chrome_elf_unittests failing on CrWinAsanCov, CrWinAsan |
||||||
Issue descriptionStarted here: https://build.chromium.org/p/chromium.fyi/builders/CrWinAsanCov%20tester/builds/980 (clang 301299, last good 301180) https://build.chromium.org/p/chromium.fyi/builders/CrWinAsan%20tester/builds/1717 (clang 301285, last good 301198) [ RUN ] ELFImportsTest.ChromeElfLoadSanityTest ../../chrome_elf/elf_imports_unittest.cc(131): error: Failed Couldn't find OK ] ELFImportsTest.DISABLED_ChromeElfLoadSanityTestImpl in output Note: Google Test filter = ELFImportsTest.DISABLED_ChromeElfLoadSanityTestImpl [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ELFImportsTest [ RUN ] ELFImportsTest.DISABLED_ChromeElfLoadSanityTestImpl ../../chrome_elf/elf_imports_unittest.cc(154): error: Value of: ::GetModuleHandleW(L"user32.dll") Actual: 76F80000 Expected: nullptr Which is: NULL [ FAILED ] ELFImportsTest.DISABLED_ChromeElfLoadSanityTestImpl (6 ms) [ RUN ] ELFImportsTest.ChromeElfSanityCheck ../../chrome_elf/elf_imports_unittest.cc(106): error: Value of: match Actual: false Expected: true Illegal import in chrome_elf.dll: USER32.dll [ FAILED ] ELFImportsTest.ChromeElfSanityCheck (0 ms)
,
Apr 25 2017
Hm no, https://build.chromium.org/p/chromium.fyi/builders/CrWinAsanCov%20tester/builds/980 was before that change made it in.
,
Apr 25 2017
robertshield, are you aware of any recent changes to chrome_elf?
,
Apr 25 2017
eugenis, could http://llvm.org/viewvc/llvm-project?rev=301225&view=rev cause this?
,
Apr 25 2017
,
Apr 25 2017
Could be. That change should not have any effect unless building with -fno-data-sections.
,
Apr 25 2017
https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?type=cs&l=1329 Apparently, we are building w/o data-sections. Since r301225, clang respects this setting for asan globals, too. This could potentially result in linking more stuff than before, and maybe pull the dependency to the user32.dll thing as well. Am I making any sense?
,
Apr 26 2017
@thakis: I don't see any recent changes that would break elf, and it would surprise me that something non-very-clang-specific would break this test without also breaking it on the msvc builders. @eugenis: what you describe sounds plausible. I don't know much of anything about -fno-data-sections and its side effects on linking, but the test is designed to fail if dependencies creep in.
,
Apr 26 2017
+Penny FYI
,
Apr 26 2017
robertshield: yes, I think this is caused by eugenis's change, thanks. (I hadn't seen that yet when I asked you.) eugenis: I'm not sure if we still need that /Gw tweak for asan on win -- if so, your change breaks us and blocks clang rolls in chromium.
,
Apr 26 2017
I'm not sure what the ODR checker bit is about. Let's revert for now. I've uploaded https://reviews.llvm.org/D32514.
,
Apr 26 2017
Please land. Rnk is out today and I'm on a phone without phab access :+)
,
Apr 26 2017
LLVM r301374
,
Apr 26 2017
I looked closely at the implementation of the ODR checker with eugenis and found a potential bug: after we rebuild a global [0] we do not copy the comdat from the original global. That may have caused duplicate symbol link errors with the ODR checker enabled. [0] http://llvm-cs.pcc.me.uk/lib/Transforms/Instrumentation/AddressSanitizer.cpp#1830
,
Apr 26 2017
The bots from #0 are green again. So this seems fixed for the moment.
,
Apr 26 2017
Do we want a bug for figuring out if /Gw is still needed and for investigating what pcc and eugenis found in comment 14 and for possibly relanding eugenis's change for coff after that's addressed?
,
Apr 27 2017
Filed issue 716193 for the follow-up suggested in comment 16. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by thakis@chromium.org
, Apr 25 2017