Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2267753002:
Make lsan and tsan suppressions files overridable. (Closed)
|
Created:
4 years, 4 months ago by ehmaldonado_chromium Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake lsan and tsan suppressions files overridable.
BUG=webrtc:6236
Committed: https://crrev.com/5302bbee2f6c188ce9b3425d136afa8b1c53102c
Cr-Commit-Position: refs/heads/master@{#413723}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Move the variables to build/config/sanitizers/BUILD.gn. #
Total comments: 1
Messages
Total messages: 21 (9 generated)
ehmaldonado@chromium.org changed reviewers: + dpranke@chromium.org, kjellander@chromium.org
Description was changed from ========== In progress: Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 ========== to ========== In progress: Make lsan and tsan suppressions files overridable. See https://bugs.chromium.org/p/webrtc/issues/detail?id=6236. BUG= ==========
https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... build/config/sanitizers/sanitizers.gni:5: # See https://bugs.chromium.org/p/webrtc/issues/detail?id=6236. Remove our bug refererence. https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... build/config/sanitizers/sanitizers.gni:9: # Please add comments. https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... build/config/sanitizers/sanitizers.gni:10: lsan_suppressions_file = lsan_suppressions_file_override I found this pattern confusing, is it really needed? I understand Edward took it from https://cs.chromium.org/chromium/src/build/config/mac/mac_sdk.gni though. https://codereview.chromium.org/2267753002/diff/1/build_overrides/build.gni File build_overrides/build.gni (right): https://codereview.chromium.org/2267753002/diff/1/build_overrides/build.gni#n... build_overrides/build.gni:25: # See https://bugs.chromium.org/p/webrtc/issues/detail?id=6236. Replace bug ref with a generic comment explaining why someone might want to override them (to provide your own suppressions)
Description was changed from ========== In progress: Make lsan and tsan suppressions files overridable. See https://bugs.chromium.org/p/webrtc/issues/detail?id=6236. BUG= ========== to ========== In progress: Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 ==========
https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... build/config/sanitizers/sanitizers.gni:10: lsan_suppressions_file = lsan_suppressions_file_override On 2016/08/22 14:59:21, kjellander_chromium wrote: > I found this pattern confusing, is it really needed? I understand Edward took it > from https://cs.chromium.org/chromium/src/build/config/mac/mac_sdk.gni though. You would only need this if you need to set the value differently per-repo and *also* want to be able to change the value in an args.gn file. Most overrides only need one or the other. Given that this isn't a declare_arg() value in chromium, it seems unlikely that it needs to be a declare_arg() in webrtc as well?
https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... build/config/sanitizers/sanitizers.gni:10: lsan_suppressions_file = lsan_suppressions_file_override On 2016/08/22 17:21:50, Dirk Pranke wrote: > On 2016/08/22 14:59:21, kjellander_chromium wrote: > > I found this pattern confusing, is it really needed? I understand Edward took > it > > from https://cs.chromium.org/chromium/src/build/config/mac/mac_sdk.gni though. > > You would only need this if you need to set the value differently per-repo and > *also* want to be able to change the value in an args.gn file. > > Most overrides only need one or the other. Given that this isn't a declare_arg() > value in chromium, it seems unlikely that it needs to be a declare_arg() in > webrtc as well? Ah, now I remember that regarding the Mac SDK version. Edward: just remove the changes in this file and include import("//build_overrides/build.gni") straight from build/config/sanitizers/BUILD.gn then.
On 2016/08/22 21:00:51, kjellander_chromium wrote: > https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... > File build/config/sanitizers/sanitizers.gni (right): > > https://codereview.chromium.org/2267753002/diff/1/build/config/sanitizers/san... > build/config/sanitizers/sanitizers.gni:10: lsan_suppressions_file = > lsan_suppressions_file_override > On 2016/08/22 17:21:50, Dirk Pranke wrote: > > On 2016/08/22 14:59:21, kjellander_chromium wrote: > > > I found this pattern confusing, is it really needed? I understand Edward > took > > it > > > from https://cs.chromium.org/chromium/src/build/config/mac/mac_sdk.gni > though. > > > > You would only need this if you need to set the value differently per-repo > and > > *also* want to be able to change the value in an args.gn file. > > > > Most overrides only need one or the other. Given that this isn't a > declare_arg() > > value in chromium, it seems unlikely that it needs to be a declare_arg() in > > webrtc as well? > > Ah, now I remember that regarding the Mac SDK version. > > Edward: just remove the changes in this file and include > import("//build_overrides/build.gni") straight from > build/config/sanitizers/BUILD.gn then. PTAL
kjellander@chromium.org changed reviewers: + jochen@chromium.org
lgtm
On 2016/08/23 08:01:44, kjellander_chromium wrote: > lgtm (don't forget to update title and description)
Description was changed from ========== In progress: Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 ========== to ========== Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 ==========
lgtm
The CQ bit was checked by ehmaldonado@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 ========== to ========== Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 ========== to ========== Make lsan and tsan suppressions files overridable. BUG=webrtc:6236 Committed: https://crrev.com/5302bbee2f6c188ce9b3425d136afa8b1c53102c Cr-Commit-Position: refs/heads/master@{#413723} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5302bbee2f6c188ce9b3425d136afa8b1c53102c Cr-Commit-Position: refs/heads/master@{#413723}
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2267753002/diff/20001/build_overrides/build.gni File build_overrides/build.gni (right): https://codereview.chromium.org/2267753002/diff/20001/build_overrides/build.g... build_overrides/build.gni:25: # Allows different projects to specify their own suppressions files. Would be nicer if there were default values in case these values are not overridden. A change like this breaks downstream projects, if not also there these three values are provided. No action required this time. V8 sanitizers already broke and I'll add the values in V8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
