ninja mac: Smarter dylib linking.

ninja mac: Smarter dylib linking.

This ports http://code.google.com/p/gyp/source/detail?r=1425 to mac.

Without this change, incremental (release) builds of chrome after
touching a file in:
base: 10.4s
content: 7.7s
chrome: 12.1s

With this change:
base: 1.7s
content: 5.1s
chrome: 12.1s
Committed: https://code.google.com/p/gyp/source/detail?r=1426

[Ami GONE FROM CHROMIUM, 2012-06-30 22:37:04]

http://codereview.chromium.org/10704054/diff/4001/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10704054/diff/4001/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:158: if flavor == 'win':
I almost did this in my CL but couldn't justify this change without changing the
method name and didn't have a better name...

http://codereview.chromium.org/10704054/diff/4001/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1494: 'nm -U -P ${lib} |cut -f1-2 -d\' \' |grep
\'[A-Z]$$\'; }')})
I don't know what this means, but FTR in the other thread Mark said:

For example, on the Mac, you’d want to consider the values in the LC_ID_DYLIB
load command to be part of the public API. That part is simple, but there’s also
LC_REEXPORT_DYLIB, which can get a little recursive.

http://codereview.chromium.org/10704054/diff/4001/test/ninja/solibs_avoid_rel...
File test/ninja/solibs_avoid_relinking/gyptest-solibs-avoid-relinking.py
(right):

http://codereview.chromium.org/10704054/diff/4001/test/ninja/solibs_avoid_rel...
test/ninja/solibs_avoid_relinking/gyptest-solibs-avoid-relinking.py:25: # XXX
does win do this implicitly? ask scott
I'm pretty sure the test failed on a gyp win bot before I added this, FWIW.

[Nico, 2012-07-01 01:34:13]

Scott: Do you know if this happens automatically on windows already? I remember
someone saying that shared libraries don't need to be relinked when a shared
library dependency changes, because import libraries handle that already. Is
that correct? Does winja handle this? Does the test I touch in this CL pass on
windows?

(Context: this CL adds a TOC file for every dylib that contains the public
interface of the dylib. For example for libbase.dylib this would produce a
libbase.dylib.TOC which lists all public functions in libbase.dylib and the
install name. If libbase.dylib changes but the public interface doesn't, with
this change e.g. libcontent.dylib won't have to be rebuilt.)

[Mark Mentovai, 2012-07-01 03:50:42]

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1503: '{ otool -l $lib | grep LC_ID_DYLIB -A 2; '
This should be -A 5. You want to catch the version numbers too.

Don’t worry about catching the “timestamp” in there. It’s always the same. It’s
not really a timestamp (any more).

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1504: 'nm -gP $lib |cut -f1-2 -d\' \' |grep
\'[ATDBCSI]$$\'; true; }'})
2. Why not grep -v U$? Seems less fragile in case a new section is introduced in
the future.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1504: 'nm -gP $lib |cut -f1-2 -d\' \' |grep
\'[ATDBCSI]$$\'; true; }'})
1. Does this work for Objective-C too? (There ought to be a test to cover that.
Add or remove an Objective-C function in a dylib and make sure that a dependent
is relinked.)

[Ami GONE FROM CHROMIUM, 2012-07-01 03:51:25]

LGTM % commentary nits and knowing nothing about os/x tooling; leaving that for
Mark to review.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:156: def WriteToc(self, flavor):
s/WriteToc/UsesToc/ ?

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:157: """Return if the target should produce a
restat rule based on a TOC file."""
s/if/true if/ to match the other method in this class (or use "whether" to keep
on a single line)

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1488: mtime_preserving_solink_base = (
IWBN to dedup this with the posix version above.  I think the only diffs are the
conditional (otool) and extract_toc; might be worth unifying.  Or not.  Your
call.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1490: # Always force dependent libraries to relink
if this library
s/libraries/targets/

[Ami GONE FROM CHROMIUM, 2012-07-01 03:51:59]

Also, yay!

[Nico, 2012-07-01 16:33:22]

Thanks!

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:156: def WriteToc(self, flavor):
On 2012/07/01 03:51:25, Ami Fischman wrote:
> s/WriteToc/UsesToc/ ?

Done.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:157: """Return if the target should produce a
restat rule based on a TOC file."""
On 2012/07/01 03:51:25, Ami Fischman wrote:
> s/if/true if/ to match the other method in this class (or use "whether" to
keep
> on a single line)

Done.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1488: mtime_preserving_solink_base = (
On 2012/07/01 03:51:25, Ami Fischman wrote:
> IWBN to dedup this with the posix version above.  I think the only diffs are
the
> conditional (otool) and extract_toc; might be worth unifying.  Or not.  Your
> call.

It's likely I'll move this into mac-tool soon (once I looked at the todo in line
1506), so I'd like to keep this duplicated for now.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1490: # Always force dependent libraries to relink
if this library
On 2012/07/01 03:51:25, Ami Fischman wrote:
> s/libraries/targets/

Done.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1503: '{ otool -l $lib | grep LC_ID_DYLIB -A 2; '
On 2012/07/01 03:50:42, Mark Mentovai wrote:
> This should be -A 5. You want to catch the version numbers too.
> 
> Don’t worry about catching the “timestamp” in there. It’s always the same.
It’s
> not really a timestamp (any more).

Done.

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1504: 'nm -gP $lib |cut -f1-2 -d\' \' |grep
\'[ATDBCSI]$$\'; true; }'})
On 2012/07/01 03:50:42, Mark Mentovai wrote:
> 2. Why not grep -v U$? Seems less fragile in case a new section is introduced
in
> the future.

Done. This is a leftover from me using -UP earlier, where I also needed to
filter out [atdbcsi-].

http://codereview.chromium.org/10704054/diff/9007/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1504: 'nm -gP $lib |cut -f1-2 -d\' \' |grep
\'[ATDBCSI]$$\'; true; }'})
On 2012/07/01 03:50:42, Mark Mentovai wrote:
> 1. Does this work for Objective-C too? (There ought to be a test to cover
that.
> Add or remove an Objective-C function in a dylib and make sure that a
dependent
> is relinked.)

As far as I understand, Objective-C functions always have private visibility and
the linker doesn't do anything with them -- Objective-C function linking is done
by the runtime at program start (or dylib load). This means this won't show
functions (class and metaclass symbols are public and do show up), but as far as
I understand it's not necessary either.

Sample:
Nicos-MacBook-Pro:src thakis$ cat test.mm
#import <Foundation/Foundation.h>
@interface A : NSObject - (void)f; @end
@implementation A - (void)f {} @end
Nicos-MacBook-Pro:src thakis$ clang -c test.mm
Nicos-MacBook-Pro:src thakis$ clang -shared -o libtest.dylib test.o -framework
Foundation
Nicos-MacBook-Pro:src thakis$ nm libtest.dylib 
0000000000000f50 t -[A f]
00000000000010e8 S  _OBJC_METACLASS_$_A
                 U _OBJC_CLASS_$_NSObject
00000000000010c0 S _OBJC_METACLASS_$_A
                 U _OBJC_METACLASS_$_NSObject
                 U __objc_empty_cache
                 U __objc_empty_vtable
                 U dyld_stub_binder

(Note that -[A f] is t, but  _OBJC_METACLASS_$_A is S)

[Mark Mentovai, 2012-07-01 18:32:30]

LGTM. Thanks for the Objective-C investigation.

http://codereview.chromium.org/10704054/diff/4005/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10704054/diff/4005/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1505: 'nm -gP $lib |cut -f1-2 -d\' \' |grep -v U$$;
true; }'})
I’d be consistent with the preceding line and put spaces on both sides of the |
pipes on this line.

[Nico, 2012-07-01 19:44:32]

Thanks!

http://codereview.chromium.org/10704054/diff/4005/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/10704054/diff/4005/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:1505: 'nm -gP $lib |cut -f1-2 -d\' \' |grep -v U$$;
true; }'})
On 2012/07/01 18:32:30, Mark Mentovai wrote:
> I’d be consistent with the preceding line and put spaces on both sides of the
|
> pipes on this line.

Done.

[Mark Mentovai, 2012-07-02 01:36:56]

LGTM