Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1839763006:
Add support for the Windows ASan configuration to the gn build (Closed)
|
Created:
4 years, 8 months ago by Reid Kleckner Modified:
4 years, 8 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. |
DescriptionAdd support for the Windows ASan configuration to the gn build
This is mostly just removing a check for "if (is_posix)" and some
minor cflags fixups.
R=dpranke@chromium.org,eugenis@chromium.org
BUG=598761
Committed: https://crrev.com/dce97597a49d6238e2dc041e2c80d89c3558dd1f
Cr-Commit-Position: refs/heads/master@{#384360}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Keep posix blacklist #Patch Set 3 : Rebase, revert unintended update.py changes #
Messages
Total messages: 16 (4 generated)
dpranke@chromium.org changed reviewers: + thakis@chromium.org
lgtm, though I'm no expert on the details of the sanitizer flags. adding thakis@ fyi in case he wants to take a glance also.
lgtm except for the disappearing -fsanitize-blacklist flag. https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (left): https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:172: "-fsanitize-blacklist=$asan_blacklist_path", It looks like the only reason we don't pass this in gyp is that it's difficult to express the path there (https://bugs.chromium.org/p/chromium/issues/detail?id=427202). I think we shouldn't ermove this flag on non-windows. https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:203: # clang_rt.asan_dll_thunk-i386.lib instead. Dirk: is there some easy way in gn to check "is the current target an executable()"? https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:251: # experimental flags, so only add them to non-chromecast ubsan builds. Huh? (not your code, only talking to myself)
https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:203: # clang_rt.asan_dll_thunk-i386.lib instead. On 2016/03/31 02:09:32, Nico wrote: > Dirk: is there some easy way in gn to check "is the current target an > executable()"? Nope. However, if what you want is some settings to be applied by default when linking executables, and different settings to be applied by default to shared_libraries (and so on), you can control that via the set_defaults() calls in //build/config/BUILDCONFIG.gn , so what you'd probably need to do is split this config up into smaller configs that are specific to each tool.
https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (left): https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:172: "-fsanitize-blacklist=$asan_blacklist_path", On 2016/03/31 02:09:32, Nico wrote: > It looks like the only reason we don't pass this in gyp is that it's difficult > to express the path there > (https://bugs.chromium.org/p/chromium/issues/detail?id=427202). I think we > shouldn't ermove this flag on non-windows. The blacklist file is empty, though. I think we should remove it, especially since we can easily blacklist single functions with attributes. https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:203: # clang_rt.asan_dll_thunk-i386.lib instead. On 2016/03/31 02:28:39, Dirk Pranke wrote: > On 2016/03/31 02:09:32, Nico wrote: > > Dirk: is there some easy way in gn to check "is the current target an > > executable()"? > > Nope. > > However, if what you want is some settings to be applied by default when linking > executables, and different settings to be applied by default to shared_libraries > (and so on), you can control that via the set_defaults() calls in > //build/config/BUILDCONFIG.gn , so what you'd probably need to do is split this > config up into smaller configs that are specific to each tool. > Yep, I figured the logic would have to move. Anyway, it sounds like this should be a separate change.
https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (left): https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:172: "-fsanitize-blacklist=$asan_blacklist_path", On 2016/03/31 16:39:15, Reid Kleckner wrote: > On 2016/03/31 02:09:32, Nico wrote: > > It looks like the only reason we don't pass this in gyp is that it's difficult > > to express the path there > > (https://bugs.chromium.org/p/chromium/issues/detail?id=427202). I think we > > shouldn't ermove this flag on non-windows. > > The blacklist file is empty, though. I think we should remove it, especially > since we can easily blacklist single functions with attributes. Ok, but that shouldn't be in a "asan in gn for Windows" CL then.
On 2016/03/31 16:53:54, Nico wrote: > https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... > File build/config/sanitizers/BUILD.gn (left): > > https://codereview.chromium.org/1839763006/diff/1/build/config/sanitizers/BUI... > build/config/sanitizers/BUILD.gn:172: > "-fsanitize-blacklist=$asan_blacklist_path", > On 2016/03/31 16:39:15, Reid Kleckner wrote: > > On 2016/03/31 02:09:32, Nico wrote: > > > It looks like the only reason we don't pass this in gyp is that it's > difficult > > > to express the path there > > > (https://bugs.chromium.org/p/chromium/issues/detail?id=427202). I think we > > > shouldn't ermove this flag on non-windows. > > > > The blacklist file is empty, though. I think we should remove it, especially > > since we can easily blacklist single functions with attributes. > > Ok, but that shouldn't be in a "asan in gn for Windows" CL then. The blacklist is currently disabled in gyp, though. This whole CL is about aligning gn with gyp config, so I felt it fit. Anyway, I can revert it and remove it separately. I think we'll get consensus in http://crbug.com/427202 to remove it soon.
Keep posix blacklist
Rebase, revert unintended update.py changes
The CQ bit was checked by rnk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1839763006/#ps40001 (title: "Rebase, revert unintended update.py changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839763006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839763006/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add support for the Windows ASan configuration to the gn build This is mostly just removing a check for "if (is_posix)" and some minor cflags fixups. R=dpranke@chromium.org,eugenis@chromium.org BUG=598761 ========== to ========== Add support for the Windows ASan configuration to the gn build This is mostly just removing a check for "if (is_posix)" and some minor cflags fixups. R=dpranke@chromium.org,eugenis@chromium.org BUG=598761 Committed: https://crrev.com/dce97597a49d6238e2dc041e2c80d89c3558dd1f Cr-Commit-Position: refs/heads/master@{#384360} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dce97597a49d6238e2dc041e2c80d89c3558dd1f Cr-Commit-Position: refs/heads/master@{#384360} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
