Add a postbuild action for executables built with ASan on Mac OS.

Add a postbuild action for executables built with ASan on Mac OS.
The action will copy the ASan dynamic runtime to the same dir where
the executable is.

BUG=170629
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187297

[Alexander Potapenko, 2012-12-19 12:04:25]

Please take a look

[Mark Mentovai, 2012-12-19 13:53:38]

Doing this in a postbuild once for each target sucks. Did anything come of the
idea I had to have one target to copy the dylib into the output directory (and
perhaps run install_name_tool on it), and then using a “copies” section to drag
that into bundled .app targets?

https://codereview.chromium.org/11642018/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11642018/diff/1/build/common.gypi#newcode3434
build/common.gypi:3434: # Define change_mach_o_flags in a variable ending in
_path
If you’re using this, fix the comment.

[Alexander Potapenko, 2013-01-16 13:54:05]

Just realized that the shell script must be executed for each target, because
install_name_tool must be ran for every binary (we need to fix the hardcoded
path to the dynamic library).
What if I just change the script to not copy the dynamic runtime if it's already
present (this won't save us much time, but is much cleaner)?

[Alexander Potapenko, 2013-01-16 14:01:08]

Add a postbuild action for executables built with ASan on Mac OS.
The action will copy the ASan dynamic runtime to the same dir where
the executable is.

[Mark Mentovai, 2013-01-16 14:07:16]

What’s wrong with the path?

Can you just fix the LC_ID_DYLIB stored in the dylib prior to linking anything
against it? You can use install_name_tool -id to do that if it’s already linked,
or use ld -install_name when you link it (or preferably just set the right
xcode_setting in GYP if that’s how you’re building it). Executables that link
against your dylib will use the LC_ID_DYLIB as the path to load the library
from.

For our purposes here, I think we’d be best off with an LC_ID_DYLIB that starts
with @executable_path/

[Alexander Potapenko, 2013-01-16 14:25:42]

On 2013/01/16 14:07:16, Mark Mentovai wrote:
> What’s wrong with the path?
> 
> Can you just fix the LC_ID_DYLIB stored in the dylib prior to linking anything
> against it? You can use install_name_tool -id to do that if it’s already
linked,
> or use ld -install_name when you link it (or preferably just set the right
> xcode_setting in GYP if that’s how you’re building it).
Nice idea. I think I can just fix LC_ID_DYLIB when building Clang for Chrome.
This will require the dylib to be always present in the output dir, so I don't
want to have @executable_path in the upstream Clang for now.
If so, I'll only need to add a dummy target to common.gypi (just dumped one
because I thought we won't need it!) which copies the dylib to the output dir
and to the .app targets.

[Alexander Potapenko, 2013-01-23 12:08:53]

Apparently it's not possible to make each executable depend on a specific target
in common.gypi (I guess this is due to
https://code.google.com/p/gyp/issues/detail?id=105 and is exactly the reason for
which we have a hardcoded tcmalloc dependency for each executable on Linux):

KeyError: 'build/asan.gyp:asan_dynamic_runtime'

Looks like we'll need to execute a script for each executable target :(

[Nico, 2013-01-23 15:14:43]

On Wed, Jan 23, 2013 at 4:08 AM, <glider@chromium.org> wrote:

> Apparently it's not possible to make each executable depend on a specific
> target
> in common.gypi (I guess this is due to
>
https://code.google.com/p/gyp/**issues/detail?id=105<https://code.google.com/...
is exactly the reason for
> which we have a hardcoded tcmalloc dependency for each executable on
> Linux):
>
> KeyError: 'build/asan.gyp:asan_dynamic_**runtime'
>
> Looks like we'll need to execute a script for each executable target :(
>

Can we fix gyp 105 instead, or do something else? Running a script for each
target sucks.


>
>
https://codereview.chromium.**org/11642018/<https://codereview.chromium.org/1...
>

[Alexander Potapenko, 2013-01-23 15:18:48]

I can give gyp 115 a shot by special-casing <(DEPTH) (as far as I understood
this may not be the right thing to do for other variables). Sounds feasible?

[Alexander Potapenko, 2013-01-28 14:16:31]

On 2013/01/23 15:18:48, Alexander Potapenko wrote:
> I can give gyp 115 a shot by special-casing <(DEPTH) (as far as I understood
> this may not be the right thing to do for other variables). Sounds feasible?

I've a draft patch that works around gyp 105, but it's insufficient.
Looks like it's even impossible to add a dependency with an absolute path to
target_conditions, because it isn't added to the map of dependencies.

[_com_google_glider, 2013-02-04 17:58:32]

After several attempts to define a default dependency for each executable
target I'm not that sure it is possible, even with gyp 105 fixed (see my
post in gyp-dev).
Is a (no-op most of the time) postbuild script copying the .dylib so much
different from the strip and change_macho_flags postbuild steps?
I'm happy to do this in a cleaner way, but I've ran out of ideas.
 On 2013/01/23 15:18:48, Alexander Potapenko wrote:

> I can give gyp 115 a shot by special-casing <(DEPTH) (as far as I
> understood
> this may not be the right thing to do for other variables). Sounds
> feasible?
>

I've a draft patch that works around gyp 105, but it's insufficient.
Looks like it's even impossible to add a dependency with an absolute path to
target_conditions, because it isn't added to the map of dependencies.

https://codereview.chromium.**org/11642018/<https://codereview.chromium.org/1...

[Mark Mentovai, 2013-02-04 18:05:42]

Can you make every target (not just executables) depend on your dependency,
but then have a postbuild step that’s only present in executables?

[Alexander Potapenko, 2013-02-08 14:36:33]

On 2013/02/04 18:05:42, Mark Mentovai wrote:
> Can you make every target (not just executables) depend on your dependency,
> but then have a postbuild step that’s only present in executables?

Like in Patch Set 5? (https://codereview.chromium.org/11642018/#ps22001)

I've tried to add 'copies' and 'postbuild' steps to the direct dependent
settings so that they'd appear for each target, but this didn't help; I've also
checked base_unittests.ninja and haven't even found a dependency on asan_dynamic
runtime.

[Mark Mentovai, 2013-02-08 17:45:12]

https://codereview.chromium.org/11642018/diff/22001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11642018/diff/22001/build/common.gypi#newcode...
build/common.gypi:2403: ['1==1', {
You’re not actually in target_defaults here, so…

https://codereview.chromium.org/11642018/diff/22001/build/common.gypi#newcode...
build/common.gypi:2404: 'dependencies': [
…this 'dependencies' section gets included into the root of each .gyp file, not
into its 'target_defaults' section. 'dependencies' outside of a target has no
meaning. This should be in common.gypi’s

  target_defaults
    conditions
      condition 1==1 (or, eventually, your own asan condition)

or you can leave it here at the root and put it in its own target_defaults, as
in

  conditions
    condition 1==1 (or, eventually, your own asan condition)
      target_defaults

[Alexander Potapenko, 2013-02-11 13:30:12]

> or you can leave it here at the root and put it in its own target_defaults, as
> in
> 
>   conditions
>     condition 1==1 (or, eventually, your own asan condition)
>       target_defaults

This case (Patch set 6 or
https://codereview.chromium.org/11642018/diff2/22001:26001/build/common.gypi)
leads to a cycle in the dependency graph.

[Mark Mentovai, 2013-02-11 13:51:39]

Because asan_dynamic_runtime winds up depending on itself?

[Alexander Potapenko, 2013-02-11 13:52:21]

This one:

> meaning. This should be in common.gypi’s
> 
>   target_defaults
>     conditions
>       condition 1==1 (or, eventually, your own asan condition)

doesn't add anything to base_unittests.ninja.

[Mark Mentovai, 2013-02-11 14:06:58]

You said “cycle in the dependency graph,” but I don’t see that happening with
patch set 6. Did something complain about a cycle, or is that a hypothesis?

Patch set 6 doesn’t work because you didn’t put condition inside target_defaults
(1555), you put it in a variables section (1556) inside target_defaults (1555)
AND left the target_defaults keyword in the conditioned block (1625). After
evaluating the conditional, this results in

target_defaults (1555)
  variables (1556)
    target_defaults (1625)

which is broken because target_defaults is meaningless both within a
target_defaults section and within a variables section.

Look at the conditions block at line 1651. You can add a condition there that
adds a dependency, and your conditioned block shouldn’t mention target_defaults
again since you’re already inside target_defaults scope (line 1555).

[Alexander Potapenko, 2013-02-11 14:15:34]

On 2013/02/11 14:06:58, Mark Mentovai wrote:
> You said “cycle in the dependency graph,” but I don’t see that happening with
> patch set 6. Did something complain about a cycle, or is that a hypothesis?
Yes, there was a GYP error. I've tried excluding the target itself by checking
for _type==executable, but it turned to be too early (no _type had been defined
yet). I'll try to check whether your suggestion works.

[Alexander Potapenko, 2013-02-11 14:42:32]

> Patch set 6 doesn’t work because you didn’t put condition inside
target_defaults
> (1555), you put it in a variables section (1556) inside target_defaults (1555)
> AND left the target_defaults keyword in the conditioned block (1625). After
> evaluating the conditional, this results in
> 
> target_defaults (1555)
>   variables (1556)
>     target_defaults (1625)
> 
> which is broken because target_defaults is meaningless both within a
> target_defaults section and within a variables section.
> 
> Look at the conditions block at line 1651. You can add a condition there that
> adds a dependency, and your conditioned block shouldn’t mention
target_defaults
> again since you’re already inside target_defaults scope (line 1555).

Am I understanding you right (see PS 10:
https://codereview.chromium.org/11642018/#ps22002)?
This is still a cycle, most certainly because of the self-dependency of
asan_dynamic_runtime. Is it possible to exclude it by target type or name?

[Mark Mentovai, 2013-02-11 14:48:05]

I think you can provide a variable in the asan_dynamic_runtime target that will
only be set to a specific sentinel value in that target, and you can work that
value into the condition to avoid exposing the dependency on itself to that
target.

[Alexander Potapenko, 2013-02-11 16:52:40]

On 2013/02/11 14:48:05, Mark Mentovai wrote:
> I think you can provide a variable in the asan_dynamic_runtime target that
will
> only be set to a specific sentinel value in that target, and you can work that
> value into the condition to avoid exposing the dependency on itself to that
> target.

Doesn't seem to help - still a cyclic dep (PS 13,
https://codereview.chromium.org/11642018/#ps23008)

[Mark Mentovai, 2013-02-11 17:18:19]

https://codereview.chromium.org/11642018/diff/23008/build/asan.gyp
File build/asan.gyp (right):

https://codereview.chromium.org/11642018/diff/23008/build/asan.gyp#newcode11
build/asan.gyp:11: 'asan_sentinel%': 1,
Does removing this % change anything?

https://codereview.chromium.org/11642018/diff/23008/build/asan.gyp#newcode16
build/asan.gyp:16: 'asan_sentinel%': 1,
Wasn’t expecting to see this here.

[Alexander Potapenko, 2013-02-11 17:34:17]

>https://codereview.chromium.org/11642018/diff/23008/build/asan.gyp#newcode11
> build/asan.gyp:11: 'asan_sentinel%': 1,
> Does removing this % change anything?
Nope. 
> https://codereview.chromium.org/11642018/diff/23008/build/asan.gyp#newcode16
> build/asan.gyp:16: 'asan_sentinel%': 1,
> Wasn’t expecting to see this here.

I thought this would be a dead checking. Nothing changes if I remove this.

[Alexander Potapenko, 2013-02-18 11:59:53]

To summarize, making a target on which every other target depends ends up in a
dependency cycle, which can't be resolved neither using sentinel variables (they
seem to be resolved too late), nor by checking for the target's name/type etc.
I haven't found any example of such a global dep in the Chromium GYP files - is
it supposed to work at all? Looks like the standard way to do something for all
the executables is to make a postbuild script.

[Nico, 2013-02-18 12:14:49]

Between which targets does the cycle exist?

Given a cycle-free target graph, I don't understand how adding a
dependency from all targets to a new target can lead to a cycle (if no
dependency is added from the new target itself, and the new target
doesn't depend on any of the existing targets).

On Mon, Feb 18, 2013 at 12:59 PM,  <glider@chromium.org> wrote:
> To summarize, making a target on which every other target depends ends up in
> a
> dependency cycle, which can't be resolved neither using sentinel variables
> (they
> seem to be resolved too late), nor by checking for the target's name/type
> etc.
> I haven't found any example of such a global dep in the Chromium GYP files -
> is
> it supposed to work at all? Looks like the standard way to do something for
> all
> the executables is to make a postbuild script.
>
> https://codereview.chromium.org/11642018/

[Alexander Potapenko, 2013-02-18 13:04:20]

On 2013/02/18 12:14:49, Nico wrote:
> Between which targets does the cycle exist?
GYP does not report the exact cycle location, but most certainly it's the
dependency between the new target and the new target itself.

[Nico, 2013-02-18 13:09:45]

Alright, since this is asan-only and this is apparently non-trivial to
do I suppose I could live with a postbuild for this. Mark?

On Mon, Feb 18, 2013 at 2:04 PM,  <glider@chromium.org> wrote:
> On 2013/02/18 12:14:49, Nico wrote:
>>
>> Between which targets does the cycle exist?
>
> GYP does not report the exact cycle location, but most certainly it's the
> dependency between the new target and the new target itself.
>
> https://codereview.chromium.org/11642018/

[Alexander Potapenko, 2013-02-18 16:51:05]

Updated the CL to use the postbuild script.

[_com_google_glider, 2013-02-20 05:01:32]

Mark, what do you think of the proposed solution?
On Feb 18, 2013 8:51 PM, <glider@chromium.org> wrote:

> Updated the CL to use the postbuild script.
>
>
https://codereview.chromium.**org/11642018/<https://codereview.chromium.org/1...
>

[Mark Mentovai, 2013-02-20 13:44:33]

I’m still not in love with it.

I wonder if we can enhance GYP unobtrusively to make this easier. For
example, we can make GYP look for a flag inside a target that says “if this
target depends on itself, just break that dependency, don’t fail.” I think
that would work. Are there other non-obtrusive GYP changes that might help
out here?


On Wed, Feb 20, 2013 at 12:01 AM, Alexander Potapenko <glider@google.com>wrote:

> Mark, what do you think of the proposed solution?
> On Feb 18, 2013 8:51 PM, <glider@chromium.org> wrote:
>
>> Updated the CL to use the postbuild script.
>>
>>
https://codereview.chromium.**org/11642018/<https://codereview.chromium.org/1...
>>
>

[Alexander Potapenko, 2013-02-20 15:53:06]

How about just allowing a target to depend on itself?
If you think it's less convenient, how should this flag look in a .gyp file?
Just a variable in the target's variables section?

[Nico, 2013-02-20 15:55:56]

Mark: Since the postbuild is only active in asan mode and since asan
clang is two months behind regular clang because of this by now, what
do you think about landing the postbuild first and doing the gyp
changes after that, so that that doesn't have to be rushed?

On Wed, Feb 20, 2013 at 4:53 PM,  <glider@chromium.org> wrote:
> How about just allowing a target to depend on itself?
> If you think it's less convenient, how should this flag look in a .gyp file?
> Just a variable in the target's variables section?
>
> https://codereview.chromium.org/11642018/

[Mark Mentovai, 2013-02-20 16:16:12]

What’s suffering because asan clang is behind?

[Nico, 2013-02-20 16:21:39]

As far as I know not too much (devs can't run asan locally, see
yesterday's thread on chromium-dev, but that's all I know about), but
it's a bit of a time bomb. If head clang deviates enough from asan
clang, one set might end up with broken compiles while the other set
doesn't, or if we need to pass new warning flags we need to make sure
to not pass them on the asan bots, etc etc

On Wed, Feb 20, 2013 at 5:16 PM, Mark Mentovai <mark@chromium.org> wrote:
> What’s suffering because asan clang is behind?

[_com_google_glider, 2013-02-20 16:47:46]

Devs can run asan locally with the older Clang, but unit_tests don't
work well with it.
The problem doesn't reproduce with the new Clang for me, although
Daniel hasn't confirmed that yet.

On Wed, Feb 20, 2013 at 8:21 PM, Nico Weber <thakis@chromium.org> wrote:
> As far as I know not too much (devs can't run asan locally, see
> yesterday's thread on chromium-dev, but that's all I know about), but
> it's a bit of a time bomb. If head clang deviates enough from asan
> clang, one set might end up with broken compiles while the other set
> doesn't, or if we need to pass new warning flags we need to make sure
> to not pass them on the asan bots, etc etc
>
> On Wed, Feb 20, 2013 at 5:16 PM, Mark Mentovai <mark@chromium.org> wrote:
>> What’s suffering because asan clang is behind?



--
Alexander Potapenko
Software Engineer
Google Moscow

[Alexander Potapenko, 2013-02-21 11:07:53]

message: Uploaded Patch Set 15.

I've attempted to allow self-dependencies in GYP so that it doesn't bark when
generating Ninja files: https://codereview.appspot.com/7370046

But now when I try to build a binary, I get the following error:
$ ninja -C out/Release/ base_unittests
ninja: warning: multiple rules generate true. build will not be correct;
continuing anyway
ninja: warning: multiple rules generate true. build will not be correct;
continuing anyway
ninja: warning: multiple rules generate true. build will not be correct;
continuing anyway
...
ninja: warning: multiple rules generate true. build will not be correct;
continuing anyway
ninja: error: dependency cycle: true ->
obj/build/chromium_builder_perf.actions_depends.stamp -> true

I can't understand why a target supposed to be a leaf one depends on some random
obj/build/chromium_builder_perf.actions_depends.stamp, and why this cycle hasn't
been detected by GYP.

[Alexander Potapenko, 2013-02-25 16:28:01]

Mark, can you please provide your vision of how the diff of common.gypi should
look like? There already are several devs within and outside Google who use PS
14 to run the latest ASan locally. Unless we figure out what needs to be done in
gyp we'd better commit it before it rots away.

[Mark Mentovai, 2013-02-25 16:51:25]

My idea was to do

  'target_defaults': {
    'dependencies': [
      '<(DEPTH)/build/asan.gyp:asan_dynamic_runtime',
    ],
    'prune_self_dependency': 1,
  },

And then modify GYP so that if prune_self_dependency is set in a target and
it appears to depend on itself, the self-dependency is removed.


On Mon, Feb 25, 2013 at 11:28 AM, <glider@chromium.org> wrote:

> Mark, can you please provide your vision of how the diff of common.gypi
> should
> look like? There already are several devs within and outside Google who
> use PS
> 14 to run the latest ASan locally. Unless we figure out what needs to be
> done in
> gyp we'd better commit it before it rots away.
>
>
https://codereview.chromium.**org/11642018/<https://codereview.chromium.org/1...
>

[Alexander Potapenko, 2013-02-25 17:14:13]

FTR, the reports about multiple rules generating true in
https://codereview.chromium.org/11642018/#msg37 were caused by me having put the
'copies' section of asan_dynamic_runtime into the direct dependent settings
instead of keeping it in the target itself.
With patch set 16 my GYP change (https://codereview.appspot.com/7370046) seems
to work fine, so I'm about to extend it to support 'prune_self_dependency'

[Alexander Potapenko, 2013-03-06 13:31:18]

Patch Set 18 appears to work fine with stand-alone executables (and is way
prettier than a shell script!), but it doesn't work with bundled apps, since
their executables reside in out/Release/appname.app/Contents/MacOS/, but still
reference the dynamic runtime in out/Release/. Looks like I need some postbuild
magic for this.

I guess the best thing to do is to put a copy of the runtime into the .app
bundle (should that be appname.app/Contents/MacOS/Resources/ or somewhere else?)
Can I just make an asan==1 clause to each .app target in Chrome, or there are
too many of them and I need to check for the _mac_bundle var? (I've found ~10 so
far, which is on the brink) Maybe there's an Xcode setting for that?

[Mark Mentovai, 2013-03-06 13:52:28]

How are the executables finding your dylib? Meaning: what does otool -L have to
say about the dylib’s location?

The two approaches here are to put a copy of the dylib alongside the executable
in a bundle, or to rewrite the path to the dylib in the executable.

[Alexander Potapenko, 2013-03-06 14:09:42]

The dylib's location is @executable_path/libclang_rt.asan_osx_dynamic.dylib by
default.
We can invoke install_name_tool for the .app bundles to make them reference
@executable_path/../../../libclang_rt.asan_osx_dynamic.dylib, and this will even
work. But if .app bundles are nested (like Chrome Helper.app inside Chrome.app)
it's going to be tricky to find the relative path to the runtime.

If we put the copies of the dylib into each bundle we don't have this problem
(yet we still need to run install_name_tool and fix the path, but it's going to
be the same every time), and we also get a bonus of being able to copy the
hermetic .app bundles to any other machine.

[Mark Mentovai, 2013-03-06 14:12:40]

I think that we should copy the dylibs into the bundles. If we ever release
these binaries to users, we can cut down on the size penalty by manually running
install_name_tool to point all of the executables to the same dylib by relative
path.

I would do this in a postbuild that’s inside a target_conditions that looks for
_mac_bundle. I wouldn’t want to maintain ten asan-specific configurations in ten
different app bundles, and then have to worry that future app bundles won’t also
get the correct treatment.

[Alexander Potapenko, 2013-03-06 16:04:07]

Patch Set 21 does teh job for both small bundles (crash_report_sender.app) and
Chromium.app (including the helpers).

[Mark Mentovai, 2013-03-06 17:03:19]

https://codereview.chromium.org/11642018/diff/55003/build/mac/asan.gyp
File build/mac/asan.gyp (right):

https://codereview.chromium.org/11642018/diff/55003/build/mac/asan.gyp#newcode16
build/mac/asan.gyp:16:
'../../third_party/llvm-build/Release+Asserts/lib/clang/*/lib/darwin/libclang_rt.asan_osx_dynamic.dylib',
I don’t like this at all.

How often does the clang version number change? Can you just hard-code it? Or
can we talk someone who deals with the clang rolls (like Nico?) to give us a
“current” symbolic link or something?

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
File build/mac/copy_asan_runtime_dylib.sh (right):

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:2: 
Copyright boilerplate goes here.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:9: set -x
set -ex is fine for both of these.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:14: ASAN_DYLIB=$(find \
Again, this is really hokey. Can we get a fixed path?

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:18: grep "${ASAN_DYLIB_NAME}" | \
grep -F to prevent it from being treated as a regular expression.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:20: sed 's/dylib .*/dylib/')
Or you can combine the grep, tr, and sed into one sed:

sed -Ene 's/^[[:blank:]]+(.*libclang_rt\.asan_osx_dynamic\.dylib).*$/\1/p'

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:22: if [ -z "${BUILTIN_DYLIB_PATH}" ]
1. Use [[ instead of [.

2. Put the “then” on the same line as the “if” with a semicolon.

if [[ -z "${BUILTIN_DYLIB_PATH}" ]]; then

Sme on line 30.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:24: echo "${BINARY} does not depend on the
ASan runtime library!"
Send this to stderr with >&2. Same on line 32.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:30: if [ "$(basename ${ASAN_DYLIB})" !=
"${ASAN_DYLIB_NAME}" ]
You should put quotes around ${ASAN_DYLIB} here.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:36: RESOURCES_DIR="$(dirname
"${BINARY_DIR}")/Resources"
This isn’t really a resource and does not belong in Contents/Resources. In
Chrome-land, the best precedent available is to put it in Contents/Libraries (as
we do with ffmpegsumo.so).

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:40: install_name_tool \
You might as well also change the LC_ID_DYLIB in
${RESOURCES_DIR}/${ASAN_DYLIB_NAME} to be
@executable_path/../Libraries/${ASAN_DYLIB_NAME} as well, so that everything
looks kosher. You can do that with install_name_tool -id.

[Alexander Potapenko, 2013-03-07 08:29:22]

Fixed the comments in the shell script. Will ask Nico about the symlink to
current.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
File build/mac/copy_asan_runtime_dylib.sh (right):

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:2: 
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> Copyright boilerplate goes here.

Done.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:9: set -x
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> set -ex is fine for both of these.

I actually don't need -x, it was only used for debugging.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:14: ASAN_DYLIB=$(find \
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> Again, this is really hokey. Can we get a fixed path?

I'll look into this.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:18: grep "${ASAN_DYLIB_NAME}" | \
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> grep -F to prevent it from being treated as a regular expression.

Decided to replace with sed.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:20: sed 's/dylib .*/dylib/')
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> Or you can combine the grep, tr, and sed into one sed:
> 
> sed -Ene 's/^[[:blank:]]+(.*libclang_rt\.asan_osx_dynamic\.dylib).*$/\1/p'

Done.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:22: if [ -z "${BUILTIN_DYLIB_PATH}" ]
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> 1. Use [[ instead of [.
> 
> 2. Put the “then” on the same line as the “if” with a semicolon.
> 
> if [[ -z "${BUILTIN_DYLIB_PATH}" ]]; then
> 
> Sme on line 30.

Done.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:24: echo "${BINARY} does not depend on the
ASan runtime library!"
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> Send this to stderr with >&2. Same on line 32.

Done.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:30: if [ "$(basename ${ASAN_DYLIB})" !=
"${ASAN_DYLIB_NAME}" ]
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> You should put quotes around ${ASAN_DYLIB} here.

Done.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:36: RESOURCES_DIR="$(dirname
"${BINARY_DIR}")/Resources"
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> This isn’t really a resource and does not belong in Contents/Resources. In
> Chrome-land, the best precedent available is to put it in Contents/Libraries
(as
> we do with ffmpegsumo.so).

Done.

https://codereview.chromium.org/11642018/diff/55003/build/mac/copy_asan_runti...
build/mac/copy_asan_runtime_dylib.sh:40: install_name_tool \
On 2013/03/06 17:03:19, Mark Mentovai wrote:
> You might as well also change the LC_ID_DYLIB in
> ${RESOURCES_DIR}/${ASAN_DYLIB_NAME} to be
> @executable_path/../Libraries/${ASAN_DYLIB_NAME} as well, so that everything
> looks kosher. You can do that with install_name_tool -id.
Good idea. Done.

[Mark Mentovai, 2013-03-07 17:03:20]

LGTM. Please follow up after the “current” symbolic link thing has made it into
the clang that we’re using.

[Alexander Potapenko, 2013-03-11 15:30:24]

Committed patchset #25 manually as r187297 (presubmit successful).