(debug build) breakpoints not working in browser_tests |
||||
Issue descriptionI'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.
,
Oct 31 2017
out of interest - how are you setting breakpoints, with 'bp', or some other means?
,
Oct 31 2017
bp? this is visual studio, so i'm just hitting f9 or clicking on the left in the IDE
,
Oct 31 2017
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.
,
Oct 31 2017
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.
,
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.
,
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?
,
Oct 31 2017
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.
,
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.
,
Oct 31 2017
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.
,
Nov 1 2017
+1 to no need to revert if fixes for issues like this can be rolled in quickly.
,
Nov 1 2017
@rnk: can you confirm that the fix also makes breakpoints work for the other examples I gave?
,
Nov 1 2017
,
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.
,
Nov 2 2017
Thanks for the fast fix!
,
Nov 2 2017
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.
,
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.
,
Nov 6 2017
I can verify this is fixed, thank you for the quick fix again!
,
Nov 6 2017
Thanks for the report! |
||||
►
Sign in to add a comment |
||||
Comment 1 by thakis@chromium.org
, Oct 31 2017Owner: r...@chromium.org