SecurityTest.NewOverflow fails on clang-cl |
|||||||
Issue descriptionit is enabled at https://chromium.googlesource.com/chromium/src/+/82681d8ead6aa059caf6a64e485dbc6b210339be and start failing base_unittest on CrWinClangGoma (building with clang-cl) https://build.chromium.org/p/chromium.fyi/builders/CrWinClangGoma/builds/22705 https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2FCrWinClangGoma%2F22705%2F%2B%2Frecipes%2Fsteps%2Fbase_unittests%2F0%2Flogs%2FSecurityTest.NewOverflow%2F0 SecurityTest.NewOverflow (run #1): [ RUN ] SecurityTest.NewOverflow ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true [ FAILED ] SecurityTest.NewOverflow (0 ms) SecurityTest.NewOverflow (run #2): [ RUN ] SecurityTest.NewOverflow ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true [ FAILED ] SecurityTest.NewOverflow (0 ms) SecurityTest.NewOverflow (run #3): [ RUN ] SecurityTest.NewOverflow ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true [ FAILED ] SecurityTest.NewOverflow (0 ms) SecurityTest.NewOverflow (run #4): [ RUN ] SecurityTest.NewOverflow ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true ../../base/security_unittest.cc(85): error: Value of: overflow_detected Actual: false Expected: true [ FAILED ] SecurityTest.NewOverflow (0 ms)
,
Feb 9 2017
Are we seeing this on any of our bots?
,
Feb 9 2017
,
Feb 9 2017
I'm trying to repro locally, see if I can figure what's up here.
,
Feb 9 2017
Looks like the compiler is simply defeating the "HideValueFromCompiler" function. Not much to see here, though I don't know how defeat Clang for the purpose. Nico? See https://cs.chromium.org/chromium/src/base/security_unittest.cc?q=securitytest&l=38
,
Feb 10 2017
To answer my question from comment 2: Yes, this is broken on our bots too, see e.g. https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28shared%29%20tester/builds/9073
,
Feb 10 2017
(and it's caused by https://codereview.chromium.org/2679153002 which reenabled the test on windows)
,
Feb 10 2017
The usual way to do this would be volatile size_t kDynamicArraySize2 = kArraySize2; instead of const size_t kDynamicArraySize2 = HideValueFromCompiler(kArraySize2); Does that work?
,
Feb 10 2017
siggi: does that help?
,
Feb 13 2017
relanding with fix here: https://codereview.chromium.org/2694873002/
,
Feb 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866 commit 9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866 Author: thakis <thakis@chromium.org> Date: Mon Feb 13 21:31:17 2017 Reenable SecurityTest.NewOverflow on Windows, with clang fix Relands https://codereview.chromium.org/2679153002 with a fix for clang. BUG= 690271 Review-Url: https://codereview.chromium.org/2694873002 Cr-Commit-Position: refs/heads/master@{#450098} [modify] https://crrev.com/9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866/base/security_unittest.cc
,
Feb 13 2017
,
Feb 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/906ce688ee9742245387dd42581020c905ab0e86 commit 906ce688ee9742245387dd42581020c905ab0e86 Author: gcasto <gcasto@chromium.org> Date: Tue Feb 14 00:10:41 2017 Revert of Reenable SecurityTest.NewOverflow on Windows, with clang fix (patchset #2 id:20001 of https://codereview.chromium.org/2694873002/ ) Reason for revert: This causes failures in the Mac ASAN bots: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29 Original issue's description: > Reenable SecurityTest.NewOverflow on Windows, with clang fix > > Relands https://codereview.chromium.org/2679153002 with a fix for clang. > > BUG= 690271 > > Review-Url: https://codereview.chromium.org/2694873002 > Cr-Commit-Position: refs/heads/master@{#450098} > Committed: https://chromium.googlesource.com/chromium/src/+/9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866 TBR=siggi@chromium.org,thakis@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 690271 Review-Url: https://codereview.chromium.org/2692183002 Cr-Commit-Position: refs/heads/master@{#450166} [modify] https://crrev.com/906ce688ee9742245387dd42581020c905ab0e86/base/security_unittest.cc
,
Feb 14 2017
I added some judicious `volatile`, which made the test actually test something on the non-win platforms, which then made it fail under mac asan (the test doesn't run on linux): https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/27082 SecurityTest.NewOverflow (run #1): [ RUN ] SecurityTest.NewOverflow ==77609==WARNING: AddressSanitizer failed to allocate 0xffffffffffffffff bytes ==77609==AddressSanitizer's allocator is terminating the process instead of returning 0 ==77609==If you don't like this behavior set allocator_may_return_null=1 ==77609==AddressSanitizer CHECK failed: /b/build/slave/mac_upload_clang/build/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:221 "((0)) != (0)" (0x0, 0x0) #0 0x1102a412f in 0x0005e12f (in libclang_rt.asan_osx_dynamic.dylib) + 95 #1 0x1102b885f in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (in libclang_rt.asan_osx_dynamic.dylib) + 79 Received signal 6 [0x00010ebe0f9c] [0x00010ebe0cf5] [0x7fff8a7695aa] [0x7fff6e8b608a] [0x7fff86685b2e] [0x0001102bcff1] [0x0001102b8860] [end of stack trace] Relevant bit: ==77609==AddressSanitizer's allocator is terminating the process instead of returning 0 ==77609==If you don't like this behavior set allocator_may_return_null=1 Somewhat related: https://github.com/google/sanitizers/issues/295 Ideally we'd want a flag that sets allocator_may_return_null only for std::nothrow news. I'm not sure if such a thing exists, so I'll reland this with the test disabled on asan, I suppose.
,
Feb 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d7b56b40717cfe0d41f7f11de12b024a1dd6fca commit 4d7b56b40717cfe0d41f7f11de12b024a1dd6fca Author: thakis <thakis@chromium.org> Date: Tue Feb 14 16:21:35 2017 Reland of nable SecurityTest.NewOverflow on Windows, with clang fix (patchset #1 id:1 of https://codereview.chromium.org/2692183002/ ) Reason for revert: Reland with test disabled on ASan. Original issue's description: > Revert of Reenable SecurityTest.NewOverflow on Windows, with clang fix (patchset #2 id:20001 of https://codereview.chromium.org/2694873002/ ) > > Reason for revert: > This causes failures in the Mac ASAN bots: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29 > > Original issue's description: > > Reenable SecurityTest.NewOverflow on Windows, with clang fix > > > > Relands https://codereview.chromium.org/2679153002 with a fix for clang. > > > > BUG= 690271 > > > > Review-Url: https://codereview.chromium.org/2694873002 > > Cr-Commit-Position: refs/heads/master@{#450098} > > Committed: https://chromium.googlesource.com/chromium/src/+/9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866 > > TBR=siggi@chromium.org,thakis@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 690271 > > Review-Url: https://codereview.chromium.org/2692183002 > Cr-Commit-Position: refs/heads/master@{#450166} > Committed: https://chromium.googlesource.com/chromium/src/+/906ce688ee9742245387dd42581020c905ab0e86 TBR=siggi@chromium.org,gcasto@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 690271 Review-Url: https://codereview.chromium.org/2692313002 Cr-Commit-Position: refs/heads/master@{#450375} [modify] https://crrev.com/4d7b56b40717cfe0d41f7f11de12b024a1dd6fca/base/security_unittest.cc
,
Feb 14 2017
,
Feb 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a71e1c4d8c9bb54716c7ee7f82ce6b99730bb1b7 commit a71e1c4d8c9bb54716c7ee7f82ce6b99730bb1b7 Author: thakis <thakis@chromium.org> Date: Tue Feb 21 17:20:41 2017 Enable SecurityTest.NewOverflow on Linux now that valgrind is gone BUG= 582398 , 690271 Review-Url: https://codereview.chromium.org/2694173005 Cr-Commit-Position: refs/heads/master@{#451787} [modify] https://crrev.com/a71e1c4d8c9bb54716c7ee7f82ce6b99730bb1b7/base/security_unittest.cc |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by siggi@chromium.org
, Feb 9 2017