ninja&make/mac: Only pass -x for loadable_modules.

ninja&make/mac: Only pass -x to strip for loadable_modules.

I misinterpreted what "Bundle products" meant in the "Default value" row
for STRIP_STYLE on
https://developer.apple.com/library/mac/documentation/DeveloperTools/Reference/XcodeBuildSettingRef/1-Build_Setting_Reference/build_setting_ref.html#//apple_ref/doc/uid/TP40003931-CH3-SW153

BUG=280718
R=mark@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=1730

[Mark Mentovai, 2013-09-13 16:09:05]

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:3: # Copyright (c) 2012 Google Inc. All rights
reserved.
2013

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:32: proc = subprocess.Popen(['otool', '-l',
p], stdout=subprocess.PIPE)
Compare the output of nm -aU instead? That gives you the actual symbol table,
which is a better comparison, and it’s slightly easier to use because you don’t
have to use a regex to find what you’re looking for. It protects against ninja
and Xcode stripping the same number of different symbols.

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:55: 
Blank line at EOF.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c
File test/mac/strip/main.c (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:6: void the_function() {}
Without any stripping but with dead code stripping:

mark@cougar bash$ clang++ -arch i386 -O3 -fno-exceptions -fno-rtti t.cc -o t
-Wl,-dead_strip
mark@cougar bash$ nm t
00001000 T __mh_execute_header
00001fb0 T _main
         U dyld_stub_binder

Where did the_function go? It’s been dead-stripped! OK, that’s fine, you want to
make sure that the different build environments do dead code stripping the same
way, but now you can’t be sure if you’re testing symbol stripping or dead code
stripping. So you should add a variant of the function that won’t be stripped by
dead code stripping and call it too:

__attribute__((used)) void the_used_function() {}

and now nm shows

00001fa0 T __Z17the_used_functionv
00001000 T __mh_execute_header
00001fb0 T _main
         U dyld_stub_binder

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:7: 
Let’s also add a couple of more attributed functions to the mix:

__attribute__((visibility("hidden"))) void the_hidden_function() {}
__attribute__((visibility("default"))) void the_visible_function() {}

Either add variants with and without __attribute__((used)), or just add them
with __attribute__((used)) without worrying about going crazy with permutations.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:7: 
Can we get some non-text symbols too? A file-scope “extern const int”, “int”,
and “static int” would be nice.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:8: int main() {
To make this test a little more realistic, let’s not call this “main” when we’re
not building the executable target. You can define some macro in the .gyp to be
the name of the function.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
File test/mac/strip/test-defaults.gyp (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:1: # Copyright (c) 2011 Google Inc. All rights
reserved.
2013

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:18: 'sources': [ 'file.c', ],
I don’t see file.c in your CL.

I assumed you were using main.c for everything, which is where my suggestion to
use the macro comes from. If you really have a separate file, that’s cool, just
upload it. But with the additions I’ve suggested, you may actually want to put
everything in a single file (and I guess in that case, file.c is a better name)
and use a macro to choose the name of the function.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:49: ],
Do you want to test a few targets with STRIP_STYLE set explicitly? If you wanted
to do that, I think it’d be fine to just have three executable targets.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:51: 
blank line

[Nico, 2013-09-13 20:11:07]

Good comments, found a bug, thanks!

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:3: # Copyright (c) 2012 Google Inc. All rights
reserved.
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> 2013

Time flies! Done.

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:32: proc = subprocess.Popen(['otool', '-l',
p], stdout=subprocess.PIPE)
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> Compare the output of nm -aU instead? That gives you the actual symbol table,
> which is a better comparison, and it’s slightly easier to use because you
don’t
> have to use a regex to find what you’re looking for. It protects against ninja
> and Xcode stripping the same number of different symbols.

Oh, nice. Why -U?

(Done for `nm -a`)

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:55: 
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> Blank line at EOF.

Done.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c
File test/mac/strip/main.c (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:6: void the_function() {}
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> Without any stripping but with dead code stripping:
> 
> mark@cougar bash$ clang++ -arch i386 -O3 -fno-exceptions -fno-rtti t.cc -o t
> -Wl,-dead_strip
> mark@cougar bash$ nm t
> 00001000 T __mh_execute_header
> 00001fb0 T _main
>          U dyld_stub_binder
> 
> Where did the_function go? It’s been dead-stripped! OK, that’s fine, you want
to
> make sure that the different build environments do dead code stripping the
same
> way, but now you can’t be sure if you’re testing symbol stripping or dead code
> stripping. So you should add a variant of the function that won’t be stripped
by
> dead code stripping and call it too:
> 
> __attribute__((used)) void the_used_function() {}
> 
> and now nm shows
> 
> 00001fa0 T __Z17the_used_functionv
> 00001000 T __mh_execute_header
> 00001fb0 T _main
>          U dyld_stub_binder

Oh, you mean due to inlining. Done.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:7: 
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> Can we get some non-text symbols too? A file-scope “extern const int”, “int”,
> and “static int” would be nice.

I had that at first, but removed it again since it didn't seem to add much. I
can add them back if you want.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:8: int main() {
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> To make this test a little more realistic, let’s not call this
“main” when we’re
> not building the executable target. You can define some macro in the .gyp to
be
> the name of the function.

The non-executable targets use file.c, not main.c

[Mark Mentovai, 2013-09-13 21:31:32]

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:32: proc = subprocess.Popen(['otool', '-l',
p], stdout=subprocess.PIPE)
Nico wrote:
> On 2013/09/13 16:09:05, Mark Mentovai wrote:
> > Compare the output of nm -aU instead? That gives you the actual symbol
table,
> > which is a better comparison, and it’s slightly easier to use because you
> don’t
> > have to use a regex to find what you’re looking for. It protects against
ninja
> > and Xcode stripping the same number of different symbols.
> 
> Oh, nice. Why -U?

To not show undefined symbols, just so that you can focus on your own symbols.
But I guess it doesn’t hurt to omit -U and include the undefined symbols either.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c
File test/mac/strip/main.c (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:6: void the_function() {}
Nico wrote:
> Oh, you mean due to inlining. Done.

Nope, not inlining, dead code stripping. Inlining might happen here but this
file still would have had a symbol for the_function because it defined it, and
some other file might have called it.

When the linker comes along, with dead-code stripping, it’ll see that the
function actually wasn’t called by another file, and drop it. Even before you
get to the point of stripping the symbol table, the_function would be removed.
After stripping, you won’t know if it was removed from the symbol table by
strip, or if the function text isn’t even present in the output anymore because
it was removed by dead code stripping.

Unless you say “__attribute__((used)).” Then the linker will say “oh, OK,
someone DOES use this” (maybe by loading the module and grabbing it out of the
symbol table) and leave it alone. Then, the function text is present in the
output, and if its symbol is gone it’s because strip got rid of it.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:7: 
Nico wrote:
> I had that at first, but removed it again since it didn't seem to add much. I
> can add them back if you want.

Yeah.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
File test/mac/strip/test-defaults.gyp (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:49: ],
I wrote:
> Do you want to test a few targets with STRIP_STYLE set explicitly? If you
wanted
> to do that, I think it’d be fine to just have three executable targets.

You left this comment unaddressed and unreplied-to.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:51: 
I wrote:
> blank line

This one too. And probably you didn’t see the two at the top of this file,
either.

https://codereview.chromium.org/23600042/diff/27001/pylib/gyp/xcode_emulation.py
File pylib/gyp/xcode_emulation.py (right):

https://codereview.chromium.org/23600042/diff/27001/pylib/gyp/xcode_emulation...
pylib/gyp/xcode_emulation.py:687: if self.spec['type'] == 'loadable_module' and
self._IsBundle():
Wow, that’s a pretty ridiculous behavior to have to match, but OK, if that’s how
Xcode is, that’s how it is.

https://codereview.chromium.org/23600042/diff/27001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/27001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:30: def CheckNsyms(p, o_expected):
This is not checking nsyms now, it’s checking syms.

https://codereview.chromium.org/23600042/diff/27001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:43: 00000fb3 T _the_function
The numbers here will vary depending on the compiler and linker and how they’re
running. You can’t hard-code this stuff, but if Xcode and ninja are both using
the same compiler and linker, you can compare one against the other. These here,
for example, look like 32-bit x86. Sorry, I thought that was obvious. Does that
make this harder to test?

You’d still have this problem, although to a slightly lesser degree, if you were
hard-coding specific nsyms values to match.

Although any option is going to have some amount of flake, maybe a good
compromise solution is to add -j to nm so that it won’t print addresses. That
should at least be approximately as stable as counting symbols, but it gets you
extra protection against “what if Xcode strips different symbols from ninja, but
they both strip the same number?” -j unfortunately also removes the section
value. If you wanted to keep that, you could also process the nm output to
remove the number.

https://codereview.chromium.org/23600042/diff/27001/test/mac/strip/test-defau...
File test/mac/strip/test-defaults.gyp (right):

https://codereview.chromium.org/23600042/diff/27001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:13: },
Since this tests wants to strip debug info, I wanted to make sure that ninja was
generating debug info for this test. It looks like xcode_emulation.py is
properly defaulting to GCC_GENERATE_DEBUGGING_SYMBOLS=YES and
DEBUG_INFORMATION_FORMAT=dwarf. Good. As long as there’s a test for that
somewhere else, you don’t need to worry about it here.

[Nico, 2013-09-13 21:41:06]

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c
File test/mac/strip/main.c (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:6: void the_function() {}
On 2013/09/13 21:31:32, Mark Mentovai wrote:
> Nico wrote:
> > Oh, you mean due to inlining. Done.
> 
> Nope, not inlining, dead code stripping. Inlining might happen here but this
> file still would have had a symbol for the_function because it defined it, and
> some other file might have called it.

Since it's called it's not dead code without inlining, no?

> 
> When the linker comes along, with dead-code stripping, it’ll see that the
> function actually wasn’t called by another file, and drop it. Even before you
> get to the point of stripping the symbol table, the_function would be removed.
> After stripping, you won’t know if it was removed from the symbol table by
> strip, or if the function text isn’t even present in the output anymore
because
> it was removed by dead code stripping.
> 
> Unless you say “__attribute__((used)).” Then the linker will say “oh, OK,
> someone DOES use this” (maybe by loading the module and grabbing it out of the
> symbol table) and leave it alone. Then, the function text is present in the
> output, and if its symbol is gone it’s because strip got rid of it.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:7: 
On 2013/09/13 21:31:32, Mark Mentovai wrote:
> Nico wrote:
> > I had that at first, but removed it again since it didn't seem to add much.
I
> > can add them back if you want.
> 
> Yeah.

Done.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
File test/mac/strip/test-defaults.gyp (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:1: # Copyright (c) 2011 Google Inc. All rights
reserved.
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> 2013

Done.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:18: 'sources': [ 'file.c', ],
On 2013/09/13 16:09:05, Mark Mentovai wrote:
> I don’t see file.c in your CL.
> 
> I assumed you were using main.c for everything, which is where my suggestion
to
> use the macro comes from. If you really have a separate file, that’s cool,
just
> upload it. But with the additions I’ve suggested, you may actually want to put
> everything in a single file (and I guess in that case, file.c is a better
name)
> and use a macro to choose the name of the function.

It's already there, from the test.gyp test in the same directory. It should now
be visible as I tweaked it a bit

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:49: ],
On 2013/09/13 21:31:32, Mark Mentovai wrote:
> I wrote:
> > Do you want to test a few targets with STRIP_STYLE set explicitly? If you
> wanted
> > to do that, I think it’d be fine to just have three executable targets.
> 
> You left this comment unaddressed and unreplied-to.

test.gyp in this folder already covers explicit STRIP_STYLE settings.

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/test-defau...
test/mac/strip/test-defaults.gyp:51: 
On 2013/09/13 21:31:32, Mark Mentovai wrote:
> I wrote:
> > blank line
> 
> This one too. And probably you didn’t see the two at the top of this file,
> either.

Done.

[Mark Mentovai, 2013-09-13 21:50:34]

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c
File test/mac/strip/main.c (right):

https://codereview.chromium.org/23600042/diff/10001/test/mac/strip/main.c#new...
test/mac/strip/main.c:6: void the_function() {}
Nico wrote:
> Since it's called it's not dead code without inlining, no?

Oh, that’s what you meant by inlining. Yeah.

https://codereview.chromium.org/23600042/diff/43001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/43001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:43: 00001000 S _i
You’ve still gotta deal with all of these numbers. Otherwise, this test will
break with a compiler roll.

https://codereview.chromium.org/23600042/diff/27001/test/mac/gyptest-strip-de...

Are you missing some code review comments?

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c
File test/mac/strip/file.c (right):

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c#new...
test/mac/strip/file.c:9: void the_hidden_function() {}
Indent so it’s clear that this is a continuation. Same on line 11, and in the
other file.

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c#new...
test/mac/strip/file.c:14: int i;
__attribute__((used))

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c#new...
test/mac/strip/file.c:15: static int si;
Gimme a “const int ci = something;” too, also with __attribute__((used)).

That’s more important than the “static int” here, which you can just get rid of
because any optimizer’s gonna drop it (it’s unused).

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/main.c
File test/mac/strip/main.c (right):

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/main.c#new...
test/mac/strip/main.c:16: int i;
Same.

[Nico, 2013-09-13 22:05:49]

https://codereview.chromium.org/23600042/diff/43001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/43001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:43: 00001000 S _i
On 2013/09/13 21:50:36, Mark Mentovai wrote:
> You’ve still gotta deal with all of these numbers. Otherwise, this test will
> break with a compiler roll.
> 
>
https://codereview.chromium.org/23600042/diff/27001/test/mac/gyptest-strip-de...
> 
> Are you missing some code review comments?

Done.

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c
File test/mac/strip/file.c (right):

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c#new...
test/mac/strip/file.c:9: void the_hidden_function() {}
On 2013/09/13 21:50:36, Mark Mentovai wrote:
> Indent so it’s clear that this is a continuation. Same on line 11, and in the
> other file.

Done.

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c#new...
test/mac/strip/file.c:14: int i;
On 2013/09/13 21:50:36, Mark Mentovai wrote:
> __attribute__((used))

Done.

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/file.c#new...
test/mac/strip/file.c:15: static int si;
On 2013/09/13 21:50:36, Mark Mentovai wrote:
> Gimme a “const int ci = something;” too, also with __attribute__((used)).
> 
> That’s more important than the “static int” here, which you can just get rid
of
> because any optimizer’s gonna drop it (it’s unused).

Done.

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/main.c
File test/mac/strip/main.c (right):

https://codereview.chromium.org/23600042/diff/43001/test/mac/strip/main.c#new...
test/mac/strip/main.c:16: int i;
On 2013/09/13 21:50:36, Mark Mentovai wrote:
> Same.

Done.

[Mark Mentovai, 2013-09-14 04:23:37]

LGTM

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:37: o = ''.join(filter(lambda s:
'radr://5614542' not in s, o.splitlines(True)))
If you Google that number, https://github.com/nico/lssym is the first result
(and I see you’ve just made changes to the README today). Ha!

cctools’ misc/strip.c tells you a little bit about this, and the number shows up
in a few other files in cctools for details. I see it in cctools-839 from Xcode
4.6, so it should still be in there. If you’re seeing a difference comparing
between local runs and the bots, are you running an Xcode DP locally?

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:40: o = re.sub(r'^[a-fA-F0-9]+', 'XXXXXXXX',
o, flags=re.MULTILINE)
OK, but be aware that 64-bit binaries are gonna have 16 hex digits here. This
will work, but I’m just pointing it out because you don’t NEED eight Xs here.
You could get away with one or even zero.

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:47: # The actual numbers here are not
interesting, they just need to be the same
But you’re not testing that the numbers are the same now. This comment should be
revised.

[Nico, 2013-09-14 04:30:41]

Thank you!

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
File test/mac/gyptest-strip-default.py (right):

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:37: o = ''.join(filter(lambda s:
'radr://5614542' not in s, o.splitlines(True)))
On 2013/09/14 04:23:37, Mark Mentovai wrote:
> If you Google that number, https://github.com/nico/lssym is the first result
> (and I see you’ve just made changes to the README today). Ha!

:-)

> cctools’ misc/strip.c tells you a little bit about this, and the number shows
up
> in a few other files in cctools for details. I see it in cctools-839 from
Xcode
> 4.6, so it should still be in there. If you’re seeing a difference comparing
> between local runs and the bots, are you running an Xcode DP locally?

I'm running 4.6.3 locally. Weird.

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:40: o = re.sub(r'^[a-fA-F0-9]+', 'XXXXXXXX',
o, flags=re.MULTILINE)
On 2013/09/14 04:23:37, Mark Mentovai wrote:
> OK, but be aware that 64-bit binaries are gonna have 16 hex digits here. This
> will work, but I’m just pointing it out because you don’t NEED eight Xs here.
> You could get away with one or even zero.

Done.

https://codereview.chromium.org/23600042/diff/60001/test/mac/gyptest-strip-de...
test/mac/gyptest-strip-default.py:47: # The actual numbers here are not
interesting, they just need to be the same
On 2013/09/14 04:23:37, Mark Mentovai wrote:
> But you’re not testing that the numbers are the same now. This comment should
be
> revised.

Done.

[Nico, 2013-09-14 04:31:41]

Committed patchset #14 manually as r1730 (presubmit successful).