Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1582413008:
GN(android) Fix build all with is_asan (Closed)
DescriptionGN(android) Fix build all with is_asan
BUG=578198
Committed: https://crrev.com/bd83b8f344f18986f50d9be6e153fef7fe8ef386
Cr-Commit-Position: refs/heads/master@{#373409}
Patch Set 1 #
Total comments: 8
Patch Set 2 : move all ldflags #Patch Set 3 : comments & visibility #
Depends on Patchset: Messages
Total messages: 16 (6 generated)
agrieve@chromium.org changed reviewers: + brettw@chromium.org
Skirts the issue outlined in the bug since it seems the deps aren't required on Android. Tested that ninja all builds, that base_unittests pass, and that chrome_apk loads.
https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:19: ":default_sanitizer_ldflags", Doesn't this mean that if a target removes the default sanitizer flags, this will add it back? That seems... undesirable. It seems like a benefit if you remove the sanitizer config explicitly but it can't be removed, that you get an error rather than having it silently be added back for you. Or maybe I misunderstand some aspect of this. https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:99: configs = [ ":default_sanitizer_ldflags" ] This will cause these ldflags to get duplicated in the targets (since sub-configs and regular configs aren't uniquified. I think we should avoid this. https://codereview.chromium.org/1582413008/diff/1/third_party/android_protobu... File third_party/android_protobuf/BUILD.gn (right): https://codereview.chromium.org/1582413008/diff/1/third_party/android_protobu... third_party/android_protobuf/BUILD.gn:17: # Does not compile with sanitizers. Can you move this down to above the sanitizer flags in the list so it doesn't look like chromium_code is related?
I've also move the remaining ldflags from the main config to the sub-config since I think that makes more sense (now the main config controls ccflags of your target, but the ldflags config / dep sets the ldflags required if any dependency fails to exclude the ccflags. https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:19: ":default_sanitizer_ldflags", On 2016/01/19 20:39:42, brettw wrote: > Doesn't this mean that if a target removes the default sanitizer flags, this > will add it back? That seems... undesirable. It seems like a benefit if you > remove the sanitizer config explicitly but it can't be removed, that you get an > error rather than having it silently be added back for you. Or maybe I > misunderstand some aspect of this. That's correct, but I think it's desirable and how it works already. If you list this as a dep, then you get the ldflags regardless of whether your specific target removes the default_sanitizer_flags config (which mainly sets ccflags) If you don't want any sanitizer business in your app, then you should remove the config *and* not depend on this target. https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:99: configs = [ ":default_sanitizer_ldflags" ] On 2016/01/19 20:39:43, brettw wrote: > This will cause these ldflags to get duplicated in the targets (since > sub-configs and regular configs aren't uniquified. I think we should avoid this. Didn't know that, and you obviously know about this way better than I do, but I just did: ninja -n -v chrome_apk > build.log And it looks like -fsanitize=address appears only once for each gcc_solink_helper.py invokation. https://codereview.chromium.org/1582413008/diff/1/third_party/android_protobu... File third_party/android_protobuf/BUILD.gn (right): https://codereview.chromium.org/1582413008/diff/1/third_party/android_protobu... third_party/android_protobuf/BUILD.gn:17: # Does not compile with sanitizers. On 2016/01/19 20:39:43, brettw wrote: > Can you move this down to above the sanitizer flags in the list so it doesn't > look like chromium_code is related? Done.
LGTM. Can you document above this target the process for not using the sanitizer for your target and the potential gotchas (depending on a library that links to it). https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:19: ":default_sanitizer_ldflags", Okay, you convinced me. https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:99: configs = [ ":default_sanitizer_ldflags" ] Oh actually this is fine. I misunderstood this and thought this was duplicating a config in BUILDCONFIG.
On 2016/02/02 22:31:17, brettw wrote: > LGTM. Can you document above this target the process for not using the sanitizer > for your target and the potential gotchas (depending on a library that links to > it). > > https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... > File build/config/sanitizers/BUILD.gn (right): > > https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... > build/config/sanitizers/BUILD.gn:19: ":default_sanitizer_ldflags", > Okay, you convinced me. > > https://codereview.chromium.org/1582413008/diff/1/build/config/sanitizers/BUI... > build/config/sanitizers/BUILD.gn:99: configs = [ ":default_sanitizer_ldflags" ] > Oh actually this is fine. I misunderstood this and thought this was duplicating > a config in BUILDCONFIG. Added comments & also added visibility to default_sanitizer_ldflags to make it clear that targets shouldn't interact directly with it.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1582413008/#ps40001 (title: "comments & visibility")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582413008/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582413008/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582413008/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582413008/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== GN(android) Fix build all with is_asan BUG=578198 ========== to ========== GN(android) Fix build all with is_asan BUG=578198 Committed: https://crrev.com/bd83b8f344f18986f50d9be6e153fef7fe8ef386 Cr-Commit-Position: refs/heads/master@{#373409} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bd83b8f344f18986f50d9be6e153fef7fe8ef386 Cr-Commit-Position: refs/heads/master@{#373409} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
