Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1146863006:
[test] Sync in *san configurations from chromium. (Closed)
|
Created:
5 years, 7 months ago by Michael Achenbach Modified:
5 years, 7 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[test] Sync in *san configurations from chromium.
This configures *san in v8 just like in chromium's
common.gypi. I also addresses compilation problems with ICU
and usage of instrumented libc++.
TBR=svenpanne@chromium.org
Committed: https://crrev.com/543bcf4d5a7a3f6941e9407f10f865895f3c0f58
Cr-Commit-Position: refs/heads/master@{#28631}
Patch Set 1 : Show exception failures. #Patch Set 2 : Show PIC failures. #Patch Set 3 : Refactoring and fixes. #Patch Set 4 : Strip no-exeptions #Patch Set 5 : Rebase after https://codereview.chromium.org/1154833002 #
Total comments: 18
Patch Set 6 : Review #
Total comments: 2
Patch Set 7 : Review #Messages
Total messages: 17 (4 generated)
machenbach@chromium.org changed reviewers: + earthdok@chromium.org, glider@chromium.org
This is to demonstrate the libc++abi compile failures for msan as discussed in this CL https://codereview.chromium.org/802583003/. Statement from earthdok: "This is very strange. -fPIC should be implied by -fsanitize=memory, and we certainly don't need exceptions." Patch 1 for the exceptions and patch 2 for PIC. It is possible that our gyp definitions lack something what chromium's have...
PS#1: So, libc++abi is the only thing that requires exceptions, and the GYP file should take care of stripping this flag: https://code.google.com/p/chromium/codesearch#chromium/src/buildtools/third_p... But note that Chromium passes -fno-exceptions in cflags_cc, whereas v8 passes it in cflags. We need to patch libc++abi.gyp to strip the flag from both cflags and cflags_cc.
On 2015/05/21 15:10:10, earthdok wrote:
PS#2: The issue is with ICU, which is a C-only target. You put -fsanitize-memory
in cflags_cc, so it isn't applied to ICU. Thus, even though -fPIC is implied by
any of -fsanitize={memory,address,thread}, ICU doesn't receive -fPIC.
Takeaways:
- -fsanitize={address,memory,thread} should be in cflags, not cflags_cc. There's
no reason for C code not to have sanitizer coverage. The same applies to other
sanitizer-related flags (-fno-omit-frame-pointer, -gline-tables-only).
- the TSan config doesn't need -fPIC and -pie either,
- since you actually have C code in v8 and the distinction between cflags and
cflags_cc is meaningful, I'd say the proper fix for (PS#1) would be to not put
-fno-exceptions in cflags in the first place.
Also on the topic of standalone.gypi, is there an actual reason for using
prepending merge here? ("cflags_cc+" vs "cflags_cc"). I think it only adds
confusion.
Thanks for these comments! This might solve a few problems at once!
machenbach@chromium.org changed reviewers: + jochen@chromium.org, svenpanne@chromium.org
PTAL. This is now up-to-date with the latest configurations in common.gypi (except not using instrumented libc++ yet for asan and tsan - I don't want to change too much at once - and a few todos).
https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:291: ['OS!="mac"', { Following common.gypi, you should probably change this to "posix and not mac". As it is now, it's not obvious why Mac is special. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:310: '-Wl,--as-needed', Do not add this unless the problem manifests in standalone v8. Looking at the referenced issue, it looks like we don't even need this in Chromium any longer. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:312: }], Need to define the ADDRESS_SANITIZER symbol here, as well as in the Mac config: https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... (This is not done uniformly with other tools because the ASan symbol is defined across multiple platforms, whereas the other tools are Linux-only.) (Please ignore the "MEMORY_SANITIZER_INITIAL_SIZE" symbol in line 2808 - this is not relevant to V8.) https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:320: # TODO(earthdok): Re-enable. http://crbug.com/427202 The referenced issue does not affect standalone V8, so this TODO should look the same as the TODOs for TSan, MSan blacklists. Generally speaking, I don't think you want to have blacklists. If you do, you would have to either pull them from Chromium, or keep them in sync with the blacklists in Chromium. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:339: }], define LEAK_SANITIZER? https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:378: # Use libc++ (third_party/libc++ and third_party/libc++abi) instead of Those pathnames are no longer correct after the move to buildtools, please fix. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:379: # stdlibc++ as standard library. This is intended to use for instrumented Please fix: "intended to be used for".
All done. PTAL at patch 6. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:291: ['OS!="mac"', { On 2015/05/22 19:57:25, earthdok wrote: > Following common.gypi, you should probably change this to "posix and not mac". > As it is now, it's not obvious why Mac is special. Done. But I'm not convinced that our posix variable is meaningful. Our gyp is a bit of a mess and the posix variable seems not to be set in a nested scope and then copied out. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:310: '-Wl,--as-needed', On 2015/05/22 19:57:25, earthdok wrote: > Do not add this unless the problem manifests in standalone v8. Looking at the > referenced issue, it looks like we don't even need this in Chromium any longer. Done. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:312: }], On 2015/05/22 19:57:25, earthdok wrote: > Need to define the ADDRESS_SANITIZER symbol here, as well as in the Mac config: > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... > > (This is not done uniformly with other tools because the ASan symbol is defined > across multiple platforms, whereas the other tools are Linux-only.) > > (Please ignore the "MEMORY_SANITIZER_INITIAL_SIZE" symbol in line 2808 - this is > not relevant to V8.) Done. See above. Btw: codesearch links get outdated quite quickly in common.gypi. I try to use fixed git hashes: https://chromium.googlesource.com/chromium/src/+/11784f3a12/build/common.gypi... I assume MEMORY_TOOL_REPLACES_ALLOCATOR is also not important? https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:320: # TODO(earthdok): Re-enable. http://crbug.com/427202 On 2015/05/22 19:57:25, earthdok wrote: > The referenced issue does not affect standalone V8, so this TODO should look the > same as the TODOs for TSan, MSan blacklists. > > Generally speaking, I don't think you want to have blacklists. If you do, you > would have to either pull them from Chromium, or keep them in sync with the > blacklists in Chromium. Removing these TODOs for now. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:339: }], On 2015/05/22 19:57:25, earthdok wrote: > define LEAK_SANITIZER? Done. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:378: # Use libc++ (third_party/libc++ and third_party/libc++abi) instead of On 2015/05/22 19:57:25, earthdok wrote: > Those pathnames are no longer correct after the move to buildtools, please fix. Done. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:379: # stdlibc++ as standard library. This is intended to use for instrumented On 2015/05/22 19:57:25, earthdok wrote: > Please fix: "intended to be used for". Done.
lgtm with nit https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:291: ['OS!="mac"', { On 2015/05/26 08:26:35, Michael Achenbach wrote: > Done. But I'm not convinced that our posix variable is meaningful. Our gyp is a > bit of a mess and the posix variable seems not to be set in a nested scope and > then copied out. Try flipping the values this variable receives. My experiment shows that it is meaningful. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:312: }], On 2015/05/26 08:26:35, Michael Achenbach wrote: > I assume MEMORY_TOOL_REPLACES_ALLOCATOR is also not important? It's not referenced in V8 code, so no. It would not mind if this changed in the future, though. MEMORY_SANITIZER_INITIAL_SIZE needs to die, on the other hand. https://codereview.chromium.org/1146863006/diff/100001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1146863006/diff/100001/build/standalone.gypi#... build/standalone.gypi:238: 'ADDRESS_SANITIZER', Could you put this in each platform-specific ASan config instead? I think the way common.gypi does it is confusing (sorry, should've been more clear about that).
https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:291: ['OS!="mac"', { On 2015/05/26 13:36:54, earthdok wrote: > On 2015/05/26 08:26:35, Michael Achenbach wrote: > > Done. But I'm not convinced that our posix variable is meaningful. Our gyp is > a > > bit of a mess and the posix variable seems not to be set in a nested scope and > > then copied out. > > Try flipping the values this variable receives. My experiment shows that it is > meaningful. Indeed. https://codereview.chromium.org/1146863006/diff/80001/build/standalone.gypi#n... build/standalone.gypi:312: }], On 2015/05/26 13:36:54, earthdok wrote: > On 2015/05/26 08:26:35, Michael Achenbach wrote: > > I assume MEMORY_TOOL_REPLACES_ALLOCATOR is also not important? > > It's not referenced in V8 code, so no. It would not mind if this changed in the > future, though. MEMORY_SANITIZER_INITIAL_SIZE needs to die, on the other hand. I'd add it as soon as someone wants to reference it. https://codereview.chromium.org/1146863006/diff/100001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/1146863006/diff/100001/build/standalone.gypi#... build/standalone.gypi:238: 'ADDRESS_SANITIZER', On 2015/05/26 13:36:54, earthdok wrote: > Could you put this in each platform-specific ASan config instead? I think the > way common.gypi does it is confusing (sorry, should've been more clear about > that). Done.
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from earthdok@chromium.org Link to the patchset: https://codereview.chromium.org/1146863006/#ps120001 (title: "Review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146863006/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/543bcf4d5a7a3f6941e9407f10f865895f3c0f58 Cr-Commit-Position: refs/heads/master@{#28631} |
