Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2363953004:
Ensure the sanitizer runtime library is copied to app bundle if enabled. (Closed)
DescriptionEnsure the sanitizer runtime library is copied to app bundle if enabled.
The clang sanitizer runtime library needs to be copied to the application
bundle if any of the sanitizers (ASAN, MSAN, ...) is enabled. Ensure this
is the case and that the library is code signed (required with Xcode 8).
Only copy the runtime library if not using Xcode version of clang (as the
runtime library depends on clang version).
BUG=None
Committed: https://crrev.com/8f50c630cef2aefee800e3aefc2401c9a2f126e8
Cr-Commit-Position: refs/heads/master@{#421537}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Improve CL. #
Total comments: 11
Patch Set 3 : Address reviewers comments. #
Messages
Total messages: 38 (23 generated)
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdefresne@chromium.org changed reviewers: + justincohen@chromium.org, stkhapugin@chromium.org
Please take a look. This is required to build with "use_asan = true".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2363953004/diff/1/build/config/ios/codesign.py File build/config/ios/codesign.py (right): https://codereview.chromium.org/2363953004/diff/1/build/config/ios/codesign.p... build/config/ios/codesign.py:204: shutil.rmtree(dst_path) Is this the same thing Xcode does with it's build in asan? https://codereview.chromium.org/2363953004/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/2363953004/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:635: "darwin/libclang_rt.asan_iossim_dynamic.dylib", does this change if we are building with Xcode clang?
https://codereview.chromium.org/2363953004/diff/1/build/config/ios/codesign.py File build/config/ios/codesign.py (right): https://codereview.chromium.org/2363953004/diff/1/build/config/ios/codesign.p... build/config/ios/codesign.py:204: shutil.rmtree(dst_path) On 2016/09/26 15:14:02, justincohen wrote: > Is this the same thing Xcode does with it's build in asan? No, it put he asan library into ${app}.app/Frameworks but the asan library built with Chromium has an install_name of @executable_path/libclang_rt.asan_iossim_dynamic.dylib while the one from Xcode has an install_name of @executable_path/Frameworks/libclang_rt.asan_iossim_dynamic.dylib. It needs to be code signed though (and Xcode do code sign the runtime library it copies). https://codereview.chromium.org/2363953004/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/2363953004/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:635: "darwin/libclang_rt.asan_iossim_dynamic.dylib", On 2016/09/26 15:14:02, justincohen wrote: > does this change if we are building with Xcode clang? Yes, it will probably change, though we currently have no configuration supporting ASAN and Xcode clang (I'm pretty sure that even without my patch compilation will fail if you enable both). Do we want to support this configuration?
lgtm https://codereview.chromium.org/2363953004/diff/1/build/config/ios/codesign.py File build/config/ios/codesign.py (right): https://codereview.chromium.org/2363953004/diff/1/build/config/ios/codesign.p... build/config/ios/codesign.py:204: shutil.rmtree(dst_path) On 2016/09/26 15:20:43, sdefresne wrote: > On 2016/09/26 15:14:02, justincohen wrote: > > Is this the same thing Xcode does with it's build in asan? > > No, it put he asan library into ${app}.app/Frameworks but the asan library built > with Chromium has an install_name of > @executable_path/libclang_rt.asan_iossim_dynamic.dylib while the one from Xcode > has an install_name of > @executable_path/Frameworks/libclang_rt.asan_iossim_dynamic.dylib. > > It needs to be code signed though (and Xcode do code sign the runtime library it > copies). Acknowledged. https://codereview.chromium.org/2363953004/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/2363953004/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:635: "darwin/libclang_rt.asan_iossim_dynamic.dylib", On 2016/09/26 15:20:43, sdefresne wrote: > On 2016/09/26 15:14:02, justincohen wrote: > > does this change if we are building with Xcode clang? > > Yes, it will probably change, though we currently have no configuration > supporting ASAN and Xcode clang (I'm pretty sure that even without my patch > compilation will fail if you enable both). Do we want to support this > configuration? I think at least a comment explaining this won't work with Xcode's clang is good enough.
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdefresne@chromium.org changed reviewers: + dpranke@chromium.org - stkhapugin@chromium.org
justincohen: please take another look as I've made major change to the CL (should be simpler now). dpranke: could you review as OWNERS of //build?
Description was changed from ========== Ensure the sanitizer runtime library is copied to app bundle if enabled. The clang sanitizer runtime library needs to be copied to the application bundle if any of the sanitizers (ASAN, MSAN, ...) is enabled. Ensure this is the case and that the library is code signed (required with Xcode 8). BUG=None ========== to ========== Ensure the sanitizer runtime library is copied to app bundle if enabled. The clang sanitizer runtime library needs to be copied to the application bundle if any of the sanitizers (ASAN, MSAN, ...) is enabled. Ensure this is the case and that the library is code signed (required with Xcode 8). Only copy the runtime library if not using Xcode version of clang (as the runtime library depends on clang version). BUG=None ==========
lgtm https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:79: if (is_mac || (is_ios && !use_xcode_clang)) { Can we use the sanitizers w/ xcode_clang at all? Maybe using_sanitizer should be checking for that instead?
still lgtm https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... File build/config/ios/codesign.py (right): https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... build/config/ios/codesign.py:235: nit empty line? https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... build/config/ios/codesign.py:325: nit empty line https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... build/config/ios/codesign.py:368: nit empty line https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:111: action("copy_asan_runtime") { copy_and_sign_asan_runtime?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for the review. https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... File build/config/ios/codesign.py (right): https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... build/config/ios/codesign.py:235: On 2016/09/27 18:08:23, justincohen wrote: > nit empty line? Done. https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... build/config/ios/codesign.py:325: On 2016/09/27 18:08:23, justincohen wrote: > nit empty line Done. https://codereview.chromium.org/2363953004/diff/20001/build/config/ios/codesi... build/config/ios/codesign.py:368: On 2016/09/27 18:08:23, justincohen wrote: > nit empty line Done. https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:79: if (is_mac || (is_ios && !use_xcode_clang)) { On 2016/09/27 17:54:44, Dirk Pranke wrote: > Can we use the sanitizers w/ xcode_clang at all? Maybe using_sanitizer should be > checking for that instead? Yes we can use asan and clang Xcode as the version of clang shipping with Xcode does provide the necessary runtime library. This requires selecting ASAN support in Xcode for the moment as the runtime library is incompatible (it is versioned) but Xcode helpfully copy it when ASAN is enabled in Xcode. I may in the future extend this to support copying the clang runtime library from Xcode when using Xcode clang, but it will require some invasive change to ios_sdk.gni. https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:111: action("copy_asan_runtime") { On 2016/09/27 18:08:23, justincohen wrote: > copy_and_sign_asan_runtime? That would make the BUILD file unnecessarily complex (as I'll have to conditionally define a variable with the name of this target, and use the variable instead of the target name) for little benefit (having a more accurate target name on iOS). I won't do this change.
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sdefresne@chromium.org
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, justincohen@chromium.org Link to the patchset: https://codereview.chromium.org/2363953004/#ps40001 (title: "Address reviewers comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... build/config/sanitizers/BUILD.gn:79: if (is_mac || (is_ios && !use_xcode_clang)) { On 2016/09/28 14:01:46, sdefresne wrote: > On 2016/09/27 17:54:44, Dirk Pranke wrote: > > Can we use the sanitizers w/ xcode_clang at all? Maybe using_sanitizer should > be > > checking for that instead? > > Yes we can use asan and clang Xcode as the version of clang shipping with Xcode > does provide the necessary runtime library. This requires selecting ASAN support > in Xcode for the moment as the runtime library is incompatible (it is versioned) > but Xcode helpfully copy it when ASAN is enabled in Xcode. > > I may in the future extend this to support copying the clang runtime library > from Xcode when using Xcode clang, but it will require some invasive change to > ios_sdk.gni. Okay. Well, given that xcode clang isn't our first choice for compilers, I don't think we should put in any work to support that condition unless we get to a point where we're stuck building w/ xcode clang for an indefinite time, so I wouldn't put the effort in for that.
On 2016/09/28 16:24:37, Dirk Pranke wrote: > https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... > File build/config/sanitizers/BUILD.gn (right): > > https://codereview.chromium.org/2363953004/diff/20001/build/config/sanitizers... > build/config/sanitizers/BUILD.gn:79: if (is_mac || (is_ios && !use_xcode_clang)) > { > On 2016/09/28 14:01:46, sdefresne wrote: > > On 2016/09/27 17:54:44, Dirk Pranke wrote: > > > Can we use the sanitizers w/ xcode_clang at all? Maybe using_sanitizer > should > > be > > > checking for that instead? > > > > Yes we can use asan and clang Xcode as the version of clang shipping with > Xcode > > does provide the necessary runtime library. This requires selecting ASAN > support > > in Xcode for the moment as the runtime library is incompatible (it is > versioned) > > but Xcode helpfully copy it when ASAN is enabled in Xcode. > > > > I may in the future extend this to support copying the clang runtime library > > from Xcode when using Xcode clang, but it will require some invasive change to > > ios_sdk.gni. > > Okay. Well, given that xcode clang isn't our first choice for compilers, I don't > think we should put in any work to support that condition unless we get to a > point where we're stuck building w/ xcode clang for an indefinite time, so I > wouldn't put the effort in for that. Ack. For reference with Xcode 8, the file to copy is the following (but I'm not sure it is stable, I want to wait till another spin of Xcode lands before trying to generalize the path): /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/8.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib Sending to CQ as is.
The CQ bit was checked by sdefresne@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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ensure the sanitizer runtime library is copied to app bundle if enabled. The clang sanitizer runtime library needs to be copied to the application bundle if any of the sanitizers (ASAN, MSAN, ...) is enabled. Ensure this is the case and that the library is code signed (required with Xcode 8). Only copy the runtime library if not using Xcode version of clang (as the runtime library depends on clang version). BUG=None ========== to ========== Ensure the sanitizer runtime library is copied to app bundle if enabled. The clang sanitizer runtime library needs to be copied to the application bundle if any of the sanitizers (ASAN, MSAN, ...) is enabled. Ensure this is the case and that the library is code signed (required with Xcode 8). Only copy the runtime library if not using Xcode version of clang (as the runtime library depends on clang version). BUG=None Committed: https://crrev.com/8f50c630cef2aefee800e3aefc2401c9a2f126e8 Cr-Commit-Position: refs/heads/master@{#421537} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8f50c630cef2aefee800e3aefc2401c9a2f126e8 Cr-Commit-Position: refs/heads/master@{#421537} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
