New issue
Advanced search Search tips

Issue 780079 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 0
Type: Bug

Blocked on:
issue 775171

Blocking:
issue 636111



Sign in to add a comment

(debug build) breakpoints not working in browser_tests

Project Member Reported by jam@chromium.org, Oct 31 2017

Issue description

I've built browser_tests with the following GN args:
is_debug = true
enable_nacl = false
target_cpu = "x86"
is_clang = true
use_goma = true
goma_dir = "E:\src\goma"
symbol_level = 2
is_win_fastlink = true

and breakpoints in various files in browser_tests aren't working. i.e. running
out\Debug\browser_tests.exe --single_process  --gtest_filter=ResourcePrefetchPredictorBrowserTest.HttpToHttpsRedirect
I couldn't get breakpoints in 
InProcessBrowserTest::SetUp()
ResourcePrefetchPredictorPrefetchingBrowserTest::SetUpCommandLine
IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, HttpToHttpsRedirect)

to be hit


This works fine with MSVS builds, i.e.:
is_debug = true
enable_nacl = false
target_cpu = "x86"
is_clang = false



Assigning to Nico to triage. I consider this P0 as I'm debugging using Visual Studio and our team (network service) is now starting to look at failing browser_tests so we'll be spending most of our time here.
 

Comment 1 by thakis@chromium.org, Oct 31 2017

Blocking: 636111
Owner: r...@chromium.org

Comment 2 by wfh@chromium.org, Oct 31 2017

out of interest - how are you setting breakpoints, with 'bp', or some other means?

Comment 3 by jam@chromium.org, Oct 31 2017

bp? this is visual studio, so i'm just hitting f9 or clicking on the left in the IDE

Comment 4 by r...@chromium.org, Oct 31 2017

Status: Assigned (was: Unconfirmed)
I reproduced this. If I manually insert __debugbreak() in the source code, the debugger stops and I can use F10 to step from there. That means our line table is at least somewhat correct.

I'm wondering if this might be another limitation of fastlink, like  http://crbug.com/756131 . I'll check what happens in VS if you use msvc+goma+fastlink.
I haven't had any problems like this with VS and fastlink, and jam@ did say that it worked with VS builds.

I've seen this work properly with clang builds so I'm surprised to see this. This is extremely serious, of course.

Comment 6 by wfh@chromium.org, Oct 31 2017

I tried debugging base_unittests built with:

is_debug = true
enable_nacl = false
target_cpu = "x86"
is_clang = true
symbol_level = 2
is_win_fastlink = true

and even by manually adding a __debugbreak() in code, it causes the debugger (windbg) to hang completely and never resolve symbols.

removing 'is_win_fastlink = true' fixes this for me, and debugger functions again.

Comment 7 by r...@chromium.org, Oct 31 2017

I seem to recall that fastlink doesn't work with windbg? I could be wrong, but that sounds like a separable issue.

I built browser_tests with msvc+goma+fastlink, and breakpoints in VS do work, so this isn't a fastlink problem, it's a clang problem. Assuming that the fix is ultimately in clang, it usually takes more than a week to find the fix, test it, commit it, and roll it into Chromium. Does that seem like a reasonable timeline?
Fastlink causes problems with iterating through symbols which causes problems with many windbg usage patterns (certain ways of setting breakpoints, for instance). Which is to say, fastlink isn't completely incompatible with windbg, but it does make for a pretty broken experience.

So, it sounds like there might be two separate problems - fastlink causing windbg hangs (is that new wfh@?) and not being able to set breakpoints in VS.

windbg seems to like to hang if you look at it sideways (wfh@ can correct me if I'm wrong) so maybe that's not important/new, but this particular bug seems serious enough that jam@'s only option is to switch to VS builds. If this experience is at all widespread then I think we need to revert to VS for all until we have a fix, rather than having each developer reverting individually. This is based on the calculation that changing compilers isn't really *that* big a deal.

Comment 9 by r...@chromium.org, Oct 31 2017

I noticed that LLVM was emitting two file checksum entries for the main file instead of one. I think this was confusing the VS debugger when it was setting breakpoints.

LLVM uses 'DIFile' to represent debug info paths, and they are uniqued by content. We had these two paths:

!DIFile(filename: "../../chrome/test/base\5Cin_process_browser_test.cc", directory: "C:\5Csrc\5Cchromium\5Csrc\5Cout\5Cclang", checksumkind: CSK_MD5, checksum: "c0e284615c2069879441c2a916cb7a69")
!DIFile(filename: "../../chrome/test/base/in_process_browser_test.cc", directory: "C:\5Csrc\5Cchromium\5Csrc\5Cout\5Cclang", checksumkind: CSK_MD5, checksum: "c0e284615c2069879441c2a916cb7a69")

Backslashes and path canonicalization strike again. =(

LLVM r317041 should fix this. I locally recompiled in_process_browser_test.cc with that revision and manually re-ran the link steps to build browser_tests.exe. After that, breakpoints worked in VS.

Personally, I'd like to try to roll clang instead of reverting. I'm told that changing the compiler back and forth causes benchmark fluctuations that automatically file performance regression bugs on both the revert and the re-land. Fixing forward seems less disruptive, and we should be able to get this done this week.
I'm not sure where the tradeoff for revert versus move forwards is, but since you've found the issue already I'm more comfortable with pushing ahead/fixing forward.

Comment 11 by jam@chromium.org, Nov 1 2017

+1 to no need to revert if fixes for issues like this can be rolled in quickly.

Comment 12 by jam@chromium.org, Nov 1 2017

@rnk: can you confirm that the fix also makes breakpoints work for the other examples I gave?

Comment 13 by h...@chromium.org, Nov 1 2017

Blockedon: 775171

Comment 14 by r...@chromium.org, Nov 2 2017

@jam: This morning I took the same steps to recompile  resource_prefetch_predictor_browsertest.cc with a fixed clang and relink browser_tests.exe and now VS stops at the two other mentioned breakpoint locations. I had to switch up the --gtest_filter to get the command line setup one to fire, but that's expected.

Looks like we'll have a new clang roll soon.  http://crbug.com/780692 , -Wdeprecated-register, is the main blocking issue, but we're making a package with the current clang revision and assuming that the patches out to fix those warnings will land soon.

I think the reason that we didn't see this breakpoint issue sooner is that it is specific to files that have dynamic initialization. We used the alternative DIFile for the source locations of the auto-generated initialization functions. Chromium is generally very strict about banning globals with constructors, so you can set breakpoints all over chrome_child.dll and content.dll and not hit this issue. However, the gtest TEST() macro uses dynamic initialization to build a registry of all tests, so any gtest file will suffer from this bug.

Comment 15 by jam@chromium.org, Nov 2 2017

Thanks for the fast fix!
Thanks for the explanation of why we don't see this problem more frequently.

Aside:  crbug.com/777943  tracks the fact that we don't seem to be able to detect static initializers in clang generated binaries, which means we now have even less protection for static initializers creeping in to the Windows binaries.

Comment 17 by r...@chromium.org, Nov 3 2017

The roll landed: https://chromium.googlesource.com/chromium/src.git/+/c4a8c7b5ff68ad06bdaff91be5170e4da89c3a9f

I think Hans is leaving the roll bug open until the roll bakes for a day or so without being reverted, but you should be able to locally verify that this bug is fixed after syncing. I guess I'll mark this fixed when the clang roll bug is closed.

Comment 18 by jam@chromium.org, Nov 6 2017

I can verify this is fixed, thank you for the quick fix again!

Comment 19 by r...@chromium.org, Nov 6 2017

Status: Verified (was: Assigned)
Thanks for the report!

Sign in to add a comment