1051190 - Properly copy and package ASan runtime dylib on Mac OS X

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Christian Holler (:decoder)]

On Mac OS X, ASan uses a dylib for its runtime components, rather than linking a static library to the executable. This dylib is part of the Clang build and typically resides in 

build/lib/clang/<version>/lib/darwin/libclang_rt.asan_osx_dynamic.dylib

where "build" is the LLVM build directory. It could however also be installed somewhere else.


As far as I can tell, the following steps are required in our build system:

1. We need to specify the path to the dylib in the mozconfig. I guess if we just wanted to support the case that it's in the build directory, then we could automatically find it, but as with llvm-symbolizer, it could also be installed somewhere else. I have a small patch that allows me to specify ASAN_DYLIB in the mozconfig.

2. We need to copy this dylib to dist/bin/
3. We need to rewrite the dylib paths in our binaries, e.g.

install_name_tool -change /path/to/original/libclang_rt.asan_osx_dynamic.dylib @executable_path/libclang_rt.asan_osx_dynamic.dylib firefox


ted, I'd like to hear your opinion on these points. In particular, would you prefer to automatically try and find the dylib in step 1, or is using a mozconfig variable ok?

Comment 1

[(not currently active) Ted Mielczarek]

If it's easy to automatically detect the dylib location and you don't think we'd need to override it in practice then that would be nice, but if you've already got a patch where you can just set the variable in the mozconfig then that's fine and I don't think you need to do the extra work.

Comment 2

[Christian Holler (:decoder)]

Attachment: bug1051190.patch

Here's a working patch that performs the following steps when packaging in the stage-package target, after all files have been staged:

1. This script searches the binary root (where the firefox binary and other dylibs reside), then copies the ASan dylib to that directory only.

2. It uses otool -L on all files to see if they reference the ASan dylib. If so, then it uses install_name_tool to rewrite the path to @executable_path/.

The nice thing about this is, that the configuration does not need to be told explicitly where the ASan dylib is on the system. The script will automatically learn this from the otool -L output of the first file that references the dylib. It will then copy the dylib from there automatically.

I prefer this solution over making the ASan dylib an install target, because it works automatically without further mozconfig entries, and in fact, we don't need the dylib except packaging (as long as the objdir isn't moved to another host).


Ted, what do you think of this solution?

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8472633 [details] [diff] [review]
bug1051190.patch

Review of attachment 8472633 [details] [diff] [review]:
-----------------------------------------------------------------

The approach looks fine, I have a few nits and two suggestions.

::: build/unix/rewrite_asan_dylib.py
@@ +1,1 @@
> +import sys

nit: put a MPL license block up top

@@ +21,5 @@
> +            try:
> +                otoolOut = subprocess.check_output(["otool", "-L", filename])
> +            except:
> +                # Errors are expected on non-executables, ignore them and continue
> +                continue

I think it would be nicer to just look for files that are chmod +x or whose names end with .dylib rather than waste time executing otool on non-binaries.

@@ +23,5 @@
> +            except:
> +                # Errors are expected on non-executables, ignore them and continue
> +                continue
> +
> +            for line in otoolOut.split("\n"):

.splitlines()

@@ +40,5 @@
> +                        # directory where the first binary (e.g. firefox) is
> +                        # encountered, should be considered the main. In our
> +                        # case, we will visit the toplevel binaries like firefox
> +                        # first, before we recurse into subdirectories like the
> +                        # plugin-container.

This seems fragile. Could you instead just pass the directory where it ought to wind up (whatever.app/Contents/MacOS) to this script, since we know the actual path in the Makefile? Alternately, you can get the path from the MozBuild Python class, like:

from mozbuild.base import MozbuildObject
build = MozbuildObject.from_environment()
binary=build.get_binary_path(where="staged-package")

(binary will be the full path to the firefox executable inside the bundle)

@@ +41,5 @@
> +                        # encountered, should be considered the main. In our
> +                        # case, we will visit the toplevel binaries like firefox
> +                        # first, before we recurse into subdirectories like the
> +                        # plugin-container.
> +                        subprocess.check_output(["cp", absDylibPath, root])

Use shutil.copy:
https://docs.python.org/2/library/shutil.html#shutil.copy

@@ +44,5 @@
> +                        # plugin-container.
> +                        subprocess.check_output(["cp", absDylibPath, root])
> +
> +                        # Now rewrite the library itself
> +                        subprocess.check_output(["install_name_tool", "-id", "@executable_path/" + DYLIB_NAME, os.path.join(root, DYLIB_NAME)])

You just want subprocess.check_call here and below if you're not using the output of the command.

Comment 4

[Christian Holler (:decoder)]

Attachment: Patch v2

Patch with feedback comments addressed. I'm passing in $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) to the script now which exactly resolves to the directory you requested. The logic with dylibCopied remains though because we can only copy the dylib once we have seen the first (successful) otool output so we know the location.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8479961 [details] [diff] [review]
Patch v2

Review of attachment 8479961 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for making those changes.

::: build/unix/rewrite_asan_dylib.py
@@ +13,5 @@
> +reference it with absolute paths but with @executable_path instead.
> +'''
> +
> +# This is the dylib we're looking for
> +DYLIB_NAME="libclang_rt.asan_osx_dynamic.dylib"

nit: I neglected to mention this previously, but our style is to use single quotes for Python strings.

Comment 6

[Christian Holler (:decoder)]

Thanks, I'll land it with your nit :)

Comment 7

[Christian Holler (:decoder)]

Pushed with nit addressed (for all strings in the code):

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fcf08e891fa

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3fcf08e891fa