Bug 140307 – If building with ASan, copy libclang_rt.asan_osx_dynamic.dylib to the build directory

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Dana Burkart]

We should copy over the ASan dylib when producing an ASan root, so that we can distribute the root to systems which don't necessarily have ASan in their toolchain. This can be accomplished by adding a Makefile to /Tools/asan, and modifying /Tools/Makefile.

Alexey had the good idea of plopping it into WebKit.framework/Versions/A/Resources so that it's automatically next to WebKitTestRunner and such.

[Dana Burkart]

Created attachment 244793 [details]
New target for DumpRenderTree which copies the dylib

This patch adds a new target to DumpRenderTree which calls a new tool to copy libclang_rt.asan_osx_dynamic.dylib to the build dir.

[Alexey Proskuryakov]

Comment on attachment 244793 [details]
New target for DumpRenderTree which copies the dylib

View in context: https://bugs.webkit.org/attachment.cgi?id=244793&action=review

I looked at the script, but I cannot meaningfully review xcodeproj changes.

> Tools/Scripts/copy-asan-dylib:23
> +if ($should_copy_dylib) {

This should probably compare to YES.

> Tools/Scripts/copy-asan-dylib:24
> +    my $xcrun_output = `xcrun --toolchain Default clang -fsanitize=address -x c - -### 2>&1`;

Can't we use a toolchain specified by asan.xcodeproj.

> Tools/Scripts/copy-asan-dylib:28
> +    $result |= system("mkdir -p $DSTROOT/System/Library/Frameworks/WebKit.framework/Versions/A/Resources");

Why does this need to use system()? Perl is very capable of creating directories and copying.

> Tools/Scripts/copy-asan-dylib:29
> +    $result |= system("ditto $1 $DSTROOT/System/Library/Frameworks/WebKit.framework/Versions/A/Resources/libclang_rt.asan_osx_dynamic.dylib") >> 8;

I think that this is the right path when building a root (i.e. when installing), but not for normal developer builds.

[Mark Rowe (bdash)]

The Resources directory within a framework isn't an appropriate place for a dylib. Putting one there can break code signing.

[Dana Burkart]

The only reason we were doing that is so that it would be next to DumpRenderTree. I guess we could put it somewhere else and add a build step to move the dylib around.

Where would you suggest putting it?

[Mark Rowe (bdash)]

Comment on attachment 244793 [details]
New target for DumpRenderTree which copies the dylib

View in context: https://bugs.webkit.org/attachment.cgi?id=244793&action=review

> Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:738
> +			buildWorkingDirectory = ../Scripts;

This violates some of our assumptions that projects don't require access content outside of their source root. DRT already violates this assumption in other places, but we should be trying to address those rather than making things worse.

> Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:1153
> +			buildSettings = {
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;

Please use .xcconfig files rather than including this gunk in the project. Please also filter out the irrelevant bits.

[David Kilzer (:ddkilzer)]

Comment on attachment 244793 [details]
New target for DumpRenderTree which copies the dylib

View in context: https://bugs.webkit.org/attachment.cgi?id=244793&action=review

r- to address Mark's and Alexey's comments.

> Tools/ChangeLog:10
> +        * DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj: Add a new target which runs copy-asan-dylib.
> +        * Scripts/copy-asan-dylib: Added. This script checks to see if we are building with asan, and
> +        if so, copies libclang_rt.asan_osx_dynamic.dylib to the build directory.

Xcode has a built-in way to copy files; can we just use a "copy files" build phase instead of a build phase script?

Or do we only need to copy this when building for ASan, so we have to include logic in the copy files build phase?

>> Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:738
>> +			buildWorkingDirectory = ../Scripts;
> 
> This violates some of our assumptions that projects don't require access content outside of their source root. DRT already violates this assumption in other places, but we should be trying to address those rather than making things worse.

In other words, please move this script into Tools/DumpRenderTree.

[David Kilzer (:ddkilzer)]

<rdar://problem/18542159>

[Dana Burkart]

(In reply to comment #6)
> Comment on attachment 244793 [details]
> New target for DumpRenderTree which copies the dylib
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244793&action=review
> 
> r- to address Mark's and Alexey's comments.
> 
> > Tools/ChangeLog:10
> > +        * DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj: Add a new target which runs copy-asan-dylib.
> > +        * Scripts/copy-asan-dylib: Added. This script checks to see if we are building with asan, and
> > +        if so, copies libclang_rt.asan_osx_dynamic.dylib to the build directory.
> 
> Xcode has a built-in way to copy files; can we just use a "copy files" build
> phase instead of a build phase script?
> 
> Or do we only need to copy this when building for ASan, so we have to
> include logic in the copy files build phase?
> 

We need to not only conditionally copy the file, we need to find it in the first place. This is not quite trivial, which is why I didn't just use Xcode's built-in way.

[Dana Burkart]

Created attachment 245568 [details]
Address concerns raised in review

[Alexey Proskuryakov]

Comment on attachment 245568 [details]
Address concerns raised in review

View in context: https://bugs.webkit.org/attachment.cgi?id=245568&action=review

> Tools/DumpRenderTree/copy-asan-dylib:7
> +#  copy-asan-dylib
> +#  asan
> +#
> +#  Created by Dana Burkart on 1/15/15.
> +#

I don't think that this is the sort of comment we us in open source tools; should be a BSD license instead.

> Tools/DumpRenderTree/copy-asan-dylib:23
> +my $asan_enabled_file = File::Spec->catfile($OBJROOT, 'ASan');

I thought that the plan was to switch to CLANG_ADDRESS_SANITIZER, and use that variable.

> Tools/DumpRenderTree/copy-asan-dylib:39
> +    $result |= system("ditto $asan_dylib_path $destination/$asan_dylib_name") >> 8;

You previously said that we needed copies or symlinks for XPC service binaries, is that the case?

I'm now thinking that we probably don't, because WebKit2 uses DYLD_INSERT_LIBRARIES when launching secondary processes.

[Mark Rowe (bdash)]

Comment on attachment 245568 [details]
Address concerns raised in review

View in context: https://bugs.webkit.org/attachment.cgi?id=245568&action=review

> Tools/DumpRenderTree/copy-asan-dylib:20
> +my $toolchain = $ENV{TOOLCHAINS_macosx} || 'default';

Why are we looking at TOOLCHAINS_macosx rather than TOOLCHAINS?

> Tools/DumpRenderTree/copy-asan-dylib:30
> +my $destination = File::Spec->catdir($DSTROOT, '/');
> +if ($ENV{RC_BUILDIT} eq "YES") {
> +    $destination = File::Spec->catdir($destination, $INSTALL_PATH);
> +}

Checking for RC_BUILDIT doesn't seem right.

This all looks like it'd result in $destination being under /tmp/ in an engineering build? Is that what we want?

I suspect that we should be using TARGET_BUILD_DIR rather than messing with DSTROOT / INSTALL_PATH ourselves. That should resolve to the appropriate location for both engineering and production builds.

> Tools/DumpRenderTree/copy-asan-dylib:33
> +    my $xcrun_output = `xcrun --toolchain $toolchain clang -fsanitize=address -x c - -### 2>&1`;

I don't think it's necessary to specify --toolchain $toolchain. xcrun defaults to the TOOLCHAINS environment variable if it's set. It will be in this case.

>> Tools/DumpRenderTree/copy-asan-dylib:39
>> +    make_path($destination) unless (-d $destination);
>> +    $result |= system("ditto $asan_dylib_path $destination/$asan_dylib_name") >> 8;
> 
> You previously said that we needed copies or symlinks for XPC service binaries, is that the case?
> 
> I'm now thinking that we probably don't, because WebKit2 uses DYLD_INSERT_LIBRARIES when launching secondary processes.

We should use the multi-argument version of system rather getting the shell involved in argument parsing.

We typically use WEXITSTATUS rather than shifting exit codes.

> Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:1178
> +				DEPLOYMENT_LOCATION = NO;

I'm not clear why you've set DEPLOYMENT_LOCATION = NO on all of these configurations. DEPLOYMENT_LOCATION is something that varies based on the type of build being performed.

[Dana Burkart]

(In reply to comment #11)
> Comment on attachment 245568 [details]
> Address concerns raised in review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245568&action=review
> 
> > Tools/DumpRenderTree/copy-asan-dylib:20
> > +my $toolchain = $ENV{TOOLCHAINS_macosx} || 'default';
> 
> Why are we looking at TOOLCHAINS_macosx rather than TOOLCHAINS?

Because that's what asan.xcconfig sets. But, per your comment below, we can just remove this line of code (by removing the --toolchain argument from the xcrun command).

> 
> > Tools/DumpRenderTree/copy-asan-dylib:30
> > +my $destination = File::Spec->catdir($DSTROOT, '/');
> > +if ($ENV{RC_BUILDIT} eq "YES") {
> > +    $destination = File::Spec->catdir($destination, $INSTALL_PATH);
> > +}
> 
> Checking for RC_BUILDIT doesn't seem right.
> 
> This all looks like it'd result in $destination being under /tmp/ in an
> engineering build? Is that what we want?

It would result in $destination being under /tmp/DumpRenderTree.dst in the case of an engineering build.

> 
> I suspect that we should be using TARGET_BUILD_DIR rather than messing with
> DSTROOT / INSTALL_PATH ourselves. That should resolve to the appropriate
> location for both engineering and production builds.
> 

If I use TARGET_BUILD_DIR, than both engineering and buildit builds will end up in the same place. Is that what we want? I was under the impression that we wanted it at the root of the build-dir in an engineering build and in the Resources folder otherwise. If that's not the case, then TARGET_BUILD_DIR would be the way to go.

> > Tools/DumpRenderTree/copy-asan-dylib:33
> > +    my $xcrun_output = `xcrun --toolchain $toolchain clang -fsanitize=address -x c - -### 2>&1`;
> 
> I don't think it's necessary to specify --toolchain $toolchain. xcrun
> defaults to the TOOLCHAINS environment variable if it's set. It will be in
> this case.
> 

Sweet!

> >> Tools/DumpRenderTree/copy-asan-dylib:39
> >> +    make_path($destination) unless (-d $destination);
> >> +    $result |= system("ditto $asan_dylib_path $destination/$asan_dylib_name") >> 8;
> > 
> > You previously said that we needed copies or symlinks for XPC service binaries, is that the case?
> > 
> > I'm now thinking that we probably don't, because WebKit2 uses DYLD_INSERT_LIBRARIES when launching secondary processes.
> 
> We should use the multi-argument version of system rather getting the shell
> involved in argument parsing.
> 
> We typically use WEXITSTATUS rather than shifting exit codes.
> 

Okay.

> > Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:1178
> > +				DEPLOYMENT_LOCATION = NO;
> 
> I'm not clear why you've set DEPLOYMENT_LOCATION = NO on all of these
> configurations. DEPLOYMENT_LOCATION is something that varies based on the
> type of build being performed.

That was a mistake (must have hit something in Xcode by accident).

[Mark Rowe (bdash)]

> > This all looks like it'd result in $destination being under /tmp/ in an
> > engineering build? Is that what we want?
> 
> It would result in $destination being under /tmp/DumpRenderTree.dst in the
> case of an engineering build.
> 
> > 
> > I suspect that we should be using TARGET_BUILD_DIR rather than messing with
> > DSTROOT / INSTALL_PATH ourselves. That should resolve to the appropriate
> > location for both engineering and production builds.
> > 
> 
> If I use TARGET_BUILD_DIR, than both engineering and buildit builds will end
> up in the same place. Is that what we want? I was under the impression that
> we wanted it at the root of the build-dir in an engineering build and in the
> Resources folder otherwise. If that's not the case, then TARGET_BUILD_DIR
> would be the way to go.

TARGET_BUILD_DIR expands to CONFIGURATION_BUILD_DIR when DEPLOYMENT_LOCATION == NO. That's the case for engineering builds, and results in TARGET_BUILD_DIR being WebKitBuild/Debug. It expands to $DSTROOT/$INSTALL_PATH when DEPLOYMENT_LOCATION == YES, which is the case for production builds. I think that maps to our desired behavior in both cases?

[Dana Burkart]

(In reply to comment #13)
> > > This all looks like it'd result in $destination being under /tmp/ in an
> > > engineering build? Is that what we want?
> > 
> > It would result in $destination being under /tmp/DumpRenderTree.dst in the
> > case of an engineering build.
> > 
> > > 
> > > I suspect that we should be using TARGET_BUILD_DIR rather than messing with
> > > DSTROOT / INSTALL_PATH ourselves. That should resolve to the appropriate
> > > location for both engineering and production builds.
> > > 
> > 
> > If I use TARGET_BUILD_DIR, than both engineering and buildit builds will end
> > up in the same place. Is that what we want? I was under the impression that
> > we wanted it at the root of the build-dir in an engineering build and in the
> > Resources folder otherwise. If that's not the case, then TARGET_BUILD_DIR
> > would be the way to go.
> 
> TARGET_BUILD_DIR expands to CONFIGURATION_BUILD_DIR when DEPLOYMENT_LOCATION
> == NO. That's the case for engineering builds, and results in
> TARGET_BUILD_DIR being WebKitBuild/Debug. It expands to
> $DSTROOT/$INSTALL_PATH when DEPLOYMENT_LOCATION == YES, which is the case
> for production builds. I think that maps to our desired behavior in both
> cases?

Okay, that makes way more sense. I'll do that.

[Dana Burkart]

Created attachment 245664 [details]
Fix issues raised by review

[Dana Burkart]

So, it turns out, if building with CLANG_ADDRESS_SANITIZER=YES instead of -fsanitize=address, this work will no longer be needed, as xcodebuild will do the copying for you. Closing.

[Andreas Kling]

Comment on attachment 245664 [details]
Fix issues raised by review

Clearing r? to get this out of the review queue.