[Mac/GN] Add a new linker_driver.py option to save unstripped linker output.

[Mac/GN] Add a new linker_driver.py option to save unstripped linker output.

This declares a new GN arg save_unstripped_output that can be enabled if
enable_stripping=true. If true, then an unstripped copy of the linker output
will be saved in the output directory with an ".unstripped" suffix.

BUG=628052
R=mark@chromium.org

Committed: https://crrev.com/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2
Cr-Commit-Position: refs/heads/master@{#406110}

[Robert Sesek, 2016-07-15 19:10:31]

Do you think .unstripped suffix or to place into an unstripped/ dir? Android
does the latter when they produce APKs.

[Mark Mentovai, 2016-07-15 19:24:17]

LGTM

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/BUILD.gn
File build/config/mac/BUILD.gn (right):

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/BUILD.gn#n...
build/config/mac/BUILD.gn:99: # The ldflags referenced below are handled by
//build/toolchain/mac/linker_driver.py.
This was wrapped before, why not now?

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/symbols.gni
File build/config/mac/symbols.gni (right):

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/symbols.gn...
build/config/mac/symbols.gni:25: # If enable_stripping=true, setting this to
true will save an unstripped
This leads with the less important thing and buries the important thing. I’d
reword this to say “Setting this to true will…. Unstripped copies can be useful
when enable_stripping is true.”

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/symbols.gn...
build/config/mac/symbols.gni:32: "save_unstripped_output=true requires
enable_stripping=true")
meh.

https://codereview.chromium.org/2157573002/diff/1/build/toolchain/mac/linker_...
File build/toolchain/mac/linker_driver.py (right):

https://codereview.chromium.org/2157573002/diff/1/build/toolchain/mac/linker_...
build/toolchain/mac/linker_driver.py:159: unstripped_out =
os.path.join(unstripped_path_prefix, tail + '.unstripped')
I like x.unstripped. Easier to find than unstripped/x.

https://codereview.chromium.org/2157573002/diff/1/build/toolchain/mac/linker_...
build/toolchain/mac/linker_driver.py:161: shutil.copy(linker_out,
unstripped_out)
Maybe copyfile to drop the x bits. No need to encourage people to execute the
unstripped versions.

[Robert Sesek, 2016-07-15 20:50:22]

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/BUILD.gn
File build/config/mac/BUILD.gn (right):

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/BUILD.gn#n...
build/config/mac/BUILD.gn:99: # The ldflags referenced below are handled by
//build/toolchain/mac/linker_driver.py.
On 2016/07/15 19:24:17, Mark Mentovai wrote:
> This was wrapped before, why not now?

Don't know what happened, but fixed.

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/symbols.gni
File build/config/mac/symbols.gni (right):

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/symbols.gn...
build/config/mac/symbols.gni:25: # If enable_stripping=true, setting this to
true will save an unstripped
On 2016/07/15 19:24:17, Mark Mentovai wrote:
> This leads with the less important thing and buries the important thing. I’d
> reword this to say “Setting this to true will…. Unstripped copies can be
useful
> when enable_stripping is true.”

Reworded to be in the same style as other args.

https://codereview.chromium.org/2157573002/diff/1/build/config/mac/symbols.gn...
build/config/mac/symbols.gni:32: "save_unstripped_output=true requires
enable_stripping=true")
On 2016/07/15 19:24:17, Mark Mentovai wrote:
> meh.

I think restricting the number of odd configurations is desirable.

https://codereview.chromium.org/2157573002/diff/1/build/toolchain/mac/linker_...
File build/toolchain/mac/linker_driver.py (right):

https://codereview.chromium.org/2157573002/diff/1/build/toolchain/mac/linker_...
build/toolchain/mac/linker_driver.py:159: unstripped_out =
os.path.join(unstripped_path_prefix, tail + '.unstripped')
On 2016/07/15 19:24:17, Mark Mentovai wrote:
> I like x.unstripped. Easier to find than unstripped/x.

Acknowledged.

https://codereview.chromium.org/2157573002/diff/1/build/toolchain/mac/linker_...
build/toolchain/mac/linker_driver.py:161: shutil.copy(linker_out,
unstripped_out)
On 2016/07/15 19:24:17, Mark Mentovai wrote:
> Maybe copyfile to drop the x bits. No need to encourage people to execute the
> unstripped versions.

Yeah that'll make tab-complete nicer too.

[Robert Sesek, 2016-07-15 20:52:25]

rsesek@chromium.org changed reviewers:
+ brettw@chromium.org

[Robert Sesek, 2016-07-15 20:52:26]

+brettw for OWNERS

[Mark Mentovai, 2016-07-15 21:31:52]

LGTM

[Robert Sesek, 2016-07-18 16:33:17]

brettw: Ping

[brettw, 2016-07-18 17:53:01]

https://codereview.chromium.org/2157573002/diff/20001/build/config/BUILDCONFI...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/2157573002/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:534: "//build/config/mac:save_unstripped_output",
The only reason anything needs to be in the default configs list is if some
targets need to opt-out of that feature by removing the config.

It doesn't seem like this is the case for this feature. It looks reasonable to
modify the "strip_all" config to add the flag to additionally save unstripped
output given the build args. Does that look possible to you?

[Robert Sesek, 2016-07-18 18:46:11]

https://codereview.chromium.org/2157573002/diff/20001/build/config/BUILDCONFI...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/2157573002/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:534: "//build/config/mac:save_unstripped_output",
On 2016/07/18 17:53:00, brettw (ping after 24h) wrote:
> The only reason anything needs to be in the default configs list is if some
> targets need to opt-out of that feature by removing the config.
> 
> It doesn't seem like this is the case for this feature. It looks reasonable to
> modify the "strip_all" config to add the flag to additionally save unstripped
> output given the build args. Does that look possible to you?

There are targets that opt out of strip_all because they need to customize the
arguments to strip (but the target does end up getting stripped). Those targets
will want the unstripped copy saved though, so having this be a separate config
allows them to remove strip_all but not the -Wcrl,unstripped flag.

[brettw, 2016-07-18 19:27:52]

https://codereview.chromium.org/2157573002/diff/20001/build/config/mac/BUILD.gn
File build/config/mac/BUILD.gn (right):

https://codereview.chromium.org/2157573002/diff/20001/build/config/mac/BUILD....
build/config/mac/BUILD.gn:13: config("compiler") {
If one always wants this, I'd put the new flag on the "compiler" config here.

[Robert Sesek, 2016-07-18 19:38:48]

https://codereview.chromium.org/2157573002/diff/20001/build/config/mac/BUILD.gn
File build/config/mac/BUILD.gn (right):

https://codereview.chromium.org/2157573002/diff/20001/build/config/mac/BUILD....
build/config/mac/BUILD.gn:13: config("compiler") {
On 2016/07/18 19:27:51, brettw (ping after 24h) wrote:
> If one always wants this, I'd put the new flag on the "compiler" config here.

Done.

[Robert Sesek, 2016-07-18 19:39:54]

The CQ bit was checked by rsesek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 19:40:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2016-07-18 19:52:16]

lgtm

[Robert Sesek, 2016-07-18 21:46:39]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2016-07-18 21:46:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2157573002/#ps40001
(title: "Compiler config")

[commit-bot: I haz the power, 2016-07-18 21:46:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 21:55:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-18 21:59:16]

Description was changed from

==========
[Mac/GN] Add a new linker_driver.py option to save unstripped linker output.

This declares a new GN arg save_unstripped_output that can be enabled if
enable_stripping=true. If true, then an unstripped copy of the linker output
will be saved in the output directory with an ".unstripped" suffix.

BUG=628052
R=mark@chromium.org
==========

to

==========
[Mac/GN] Add a new linker_driver.py option to save unstripped linker output.

This declares a new GN arg save_unstripped_output that can be enabled if
enable_stripping=true. If true, then an unstripped copy of the linker output
will be saved in the output directory with an ".unstripped" suffix.

BUG=628052
R=mark@chromium.org

Committed: https://crrev.com/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2
Cr-Commit-Position: refs/heads/master@{#406110}
==========

[commit-bot: I haz the power, 2016-07-18 21:59:17]

Patchset 3 (id:??) landed as
https://crrev.com/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2
Cr-Commit-Position: refs/heads/master@{#406110}