Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1158643002:
GN: Build with a custom libc++ when using ASAN, TSAN or MSAN. (Closed)
|
Created:
5 years, 7 months ago by Sam McNally Modified:
5 years, 6 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@gn-instrumented-libraries-prebuilt Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN: Build with a custom libc++ when using ASAN, TSAN or MSAN.
Committed: https://crrev.com/296900634b5d646d19dbd5415183788a6f005633
Cr-Commit-Position: refs/heads/master@{#331937}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : rebase #
Messages
Total messages: 26 (9 generated)
Patchset #1 (id:1) has been deleted
sammc@chromium.org changed reviewers: + dpranke@chromium.org
This depends on https://codereview.chromium.org/1145333004/.
glider@chromium.org changed reviewers: + glider@chromium.org
https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:19: use_custom_libcxx = (is_asan && is_desktop_linux) || is_tsan || is_msan Is 'is_desktop_linux' in GN the same as 'OS=="linux" and chromeos==0' in GYP?
https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:19: use_custom_libcxx = (is_asan && is_desktop_linux) || is_tsan || is_msan On 2015/05/25 08:27:18, Alexander Potapenko wrote: > Is 'is_desktop_linux' in GN the same as 'OS=="linux" and chromeos==0' in GYP? Yes. It's defined in https://code.google.com/p/chromium/codesearch/#chromium/src/build/config/BUIL....
earthdok@chromium.org changed reviewers: + earthdok@chromium.org
The GYP config makes it possible to enable custom libc++ without any of the sanitizers. It looks like this patch doesn't allow to do that. It's not a configuration that is used often, but does come in handy occasionally when debugging, so we might want to keep it. Nico might have something to say about this. https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:18: if (use_custom_libcxx) { Shouldn't this be in a separate GN file? https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:18: # This is intended to use for instrumented builds. Please fix the wording here ("intended to be used"). https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:19: use_custom_libcxx = (is_asan && is_desktop_linux) || is_tsan || is_msan On 2015/05/25 08:31:22, Sam McNally wrote: > On 2015/05/25 08:27:18, Alexander Potapenko wrote: > > Is 'is_desktop_linux' in GN the same as 'OS=="linux" and chromeos==0' in GYP? > > Yes. It's defined in > https://code.google.com/p/chromium/codesearch/#chromium/src/build/config/BUIL.... This is disabled on CrOS because of GYP-related issues: http://crbug.com/396979 You should be able to enable it in GN. If you'd rather have GN mirror the GYP configuration exactly, then please put a TODO here. Once GYP is gone (or the issue is fixed), we can replace is_desktop_linux with is_linux.
earthdok@chromium.org changed reviewers: + thakis@chromium.org
(actually adding Nico)
On 2015/05/25 15:28:00, earthdok wrote: > The GYP config makes it possible to enable custom libc++ without any of the > sanitizers. It looks like this patch doesn't allow to do that. It's not a > configuration that is used often, but does come in handy occasionally when > debugging, so we might want to keep it. Nico might have something to say about > this. We'll very likely ship libc++ on OS X, but I think it's ok to make that work when the time comes. So I don't really have anything interesting to say here. (Does the libc++ stuff in gyp on OS X fix up the rpaths? Or is that not done there yet either? It'll be important for running asan tests on swarming, as third_party/llvm-build/Release+Asserts doesn't exist on swarming slaves) > > https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... > File build/config/sanitizers/BUILD.gn (right): > > https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... > build/config/sanitizers/BUILD.gn:18: if (use_custom_libcxx) { > Shouldn't this be in a separate GN file? > > https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... > File build/config/sanitizers/sanitizers.gni (right): > > https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... > build/config/sanitizers/sanitizers.gni:18: # This is intended to use for > instrumented builds. > Please fix the wording here ("intended to be used"). > > https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... > build/config/sanitizers/sanitizers.gni:19: use_custom_libcxx = (is_asan && > is_desktop_linux) || is_tsan || is_msan > On 2015/05/25 08:31:22, Sam McNally wrote: > > On 2015/05/25 08:27:18, Alexander Potapenko wrote: > > > Is 'is_desktop_linux' in GN the same as 'OS=="linux" and chromeos==0' in > GYP? > > > > Yes. It's defined in > > > https://code.google.com/p/chromium/codesearch/#chromium/src/build/config/BUIL.... > > This is disabled on CrOS because of GYP-related issues: > http://crbug.com/396979 > > You should be able to enable it in GN. > > If you'd rather have GN mirror the GYP configuration exactly, then please put a > TODO here. Once GYP is gone (or the issue is fixed), we can replace > is_desktop_linux with is_linux.
https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:18: if (use_custom_libcxx) { On 2015/05/25 15:27:59, earthdok wrote: > Shouldn't this be in a separate GN file? Are you suggesting that rather than have deps depend on libcxx_proxy, we should have other targets depend on libcxx_proxy directly? I'm not sure what the intent of your question is.
On 2015/05/25 15:28:00, earthdok wrote: > The GYP config makes it possible to enable custom libc++ without any of the > sanitizers. It looks like this patch doesn't allow to do that. It's not a > configuration that is used often, but does come in handy occasionally when > debugging, so we might want to keep it. Nico might have something to say about > this. > use_custom_libcxx doesn't require enabling sanitizers; whether sanitizers are enabled just determines the default value. Using a custom libc++ seemed sufficiently related to sanitizers to declare the arg in sanitizers.gni and use //build/config/sanitizers:deps for setting link flags and deps. https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... File build/config/sanitizers/sanitizers.gni (right): https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:18: # This is intended to use for instrumented builds. On 2015/05/25 15:28:00, earthdok wrote: > Please fix the wording here ("intended to be used"). Done. https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/sanitizers.gni:19: use_custom_libcxx = (is_asan && is_desktop_linux) || is_tsan || is_msan On 2015/05/25 15:28:00, earthdok wrote: > On 2015/05/25 08:31:22, Sam McNally wrote: > > On 2015/05/25 08:27:18, Alexander Potapenko wrote: > > > Is 'is_desktop_linux' in GN the same as 'OS=="linux" and chromeos==0' in > GYP? > > > > Yes. It's defined in > > > https://code.google.com/p/chromium/codesearch/#chromium/src/build/config/BUIL.... > > This is disabled on CrOS because of GYP-related issues: > http://crbug.com/396979 > > You should be able to enable it in GN. > > If you'd rather have GN mirror the GYP configuration exactly, then please put a > TODO here. Once GYP is gone (or the issue is fixed), we can replace > is_desktop_linux with is_linux. Done.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/1158643002/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:18: if (use_custom_libcxx) { On 2015/05/25 22:26:43, Dirk Pranke wrote: > On 2015/05/25 15:27:59, earthdok wrote: > > Shouldn't this be in a separate GN file? > > Are you suggesting that rather than have deps depend on libcxx_proxy, we should > have other targets depend on libcxx_proxy directly? I'm not sure what the intent > of your question is. Basically, yes. I was trying to say that the sanitizer config is not a good place to declare this dependency, as libc++ is not directly related to sanitizers. However, I don't feel strongly about this, especially if it would require changing a lot of GN configs. https://codereview.chromium.org/1158643002/diff/60001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (left): https://codereview.chromium.org/1158643002/diff/60001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:7: # |is_asan|, |is_lsan|, |is_tsan| and |is_msan| are false. This comment needs to be updated. By the way, where is this unconditional dependency declared? The only candidate I see is in ./build/config/BUILDCONFIG.gn, but that seems to only affect component builds.
https://codereview.chromium.org/1158643002/diff/60001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (left): https://codereview.chromium.org/1158643002/diff/60001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:7: # |is_asan|, |is_lsan|, |is_tsan| and |is_msan| are false. On 2015/05/26 16:43:49, earthdok wrote: > This comment needs to be updated. Done. > > By the way, where is this unconditional dependency declared? The only candidate > I see is in ./build/config/BUILDCONFIG.gn, but that seems to only affect > component builds. Executables and shared libraries should have a dependency on this target. See https://code.google.com/p/chromium/codesearch/#search/&q=sanitizers:deps
lgtm, thanks On 2015/05/27 00:08:15, Sam McNally wrote: > Executables and shared libraries should have a dependency on this target. See > https://code.google.com/p/chromium/codesearch/#search/&q=sanitizers:deps Could you point me to where this dep is added to the chrome executable in a non-component build? I must be missing something. Incidentally, we shouldn't really link sanitizer options/suppressions into shared libraries. Both GYP and GN configs are wrong on this. I'll file a bug.
lgtm
Patchset #4 (id:100001) has been deleted
On 2015/05/27 16:21:56, earthdok wrote: > Could you point me to where this dep is added to the chrome executable in a > non-component build? I must be missing something. I think the chrome executable target is currently missing that dep.
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, earthdok@chromium.org Link to the patchset: https://codereview.chromium.org/1158643002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158643002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/296900634b5d646d19dbd5415183788a6f005633 Cr-Commit-Position: refs/heads/master@{#331937}
Message was sent while issue was closed.
On 2015/05/29 08:27:16, Sam McNally wrote: > On 2015/05/27 16:21:56, earthdok wrote: > > Could you point me to where this dep is added to the chrome executable in a > > non-component build? I must be missing something. > > I think the chrome executable target is currently missing that dep. On 2015/05/29 08:27:16, Sam McNally wrote: > On 2015/05/27 16:21:56, earthdok wrote: > > Could you point me to where this dep is added to the chrome executable in a > > non-component build? I must be missing something. > > I think the chrome executable target is currently missing that dep. I think you're right. My concern is with what happens if a developer forgets to add this dep to their target. The effect should be some sort of error message; we should not silently disable ASan or silently ignore sanitizer_options. This more or less works as of now, because this dep is also responsible for passing ldflags=-fsanitize=... If is_asan=true is set, every target will get cflags=-fsanitize=address (through build/config/compiler), but only those targets that have this dep will get the linker flag. Those that don't will fail linking if they're executables (every executable needs to link in the ASan runtime), but link successfully if they're shared libraries. This is not perfect though: - some targets might want ASan without default options, so we'll have to decouple ldflags from default options at some point (I know nacl defines its own default options somewhere, for instance), - it's not correct to not pass ldflags=-fsanitize=... to a shared library target even if it links without errors. Even though we don't need to link shared libraries against the ASan runtime, that flag may have other side effects. - (minor issue) if the linker flag is omitted, linking a big executable takes a lot of time because of all the symbol resolution errors. If you try building non-component Chrome with ASan and goma, it appears as if the linking step hangs forever, because goma swallows all the error messages. So this is not a very good diagnostic. Can we somehow force every target that ever receives the ASan cflags to depend on either //build/config/sanitizers:deps or //build/config/sanitizers:no_default_options (or somesuch)? (I won't be around to continue this discussion, but glider@ will.) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
