[Chromecast] Include all stdlibc++/libgcc symbols in cast_shell.

[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835

Committed: https://crrev.com/fbf4f9ac36e9e121620d65ca85dd998a67745049
Cr-Commit-Position: refs/heads/master@{#364118}

[bcf, 2015-11-25 19:25:11]

Description was changed from

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast_shell.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

BUG= internal b/25566835
==========

to

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast_shell.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

BUG= internal b/25566835
==========

[bcf, 2015-11-25 19:25:11]

bcf@chromium.org changed reviewers:
+ byungchul@chromium.org, gfhuang@chromium.org, halliwell@chromium.org,
icoolidge@chromium.org, slan@chromium.org, wzhong@chromium.org

[bcf, 2015-11-25 19:27:32]

We decided that we were bound to run into more issues by running multiple copies
of stdlibc++, and in general it is much more complex to attempt to prevent
sharing between cast_shell and the shared libraries.

Thus this CL supersedes these:
https://codereview.chromium.org/1474663005/
https://eureka-internal-review.git.corp.google.com/#/c/41329/

[bcf, 2015-11-25 19:31:23]

https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:81: 'target_name': 'cast_shlib_consumer_ldflags',
This is a target any executable which uses libcast* shlibs should depend on.

I assume we'd accomplish the same thing in gn using a config, but how can we add
ldflags to the end of link line (this is important)?

gn provides `libs`, but it prepends to each option: "Expands to the list of
system libraries to link to. Each will be prefixed by the "lib_prefix".". For
example "foo" will become "-lfoo"

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

[wzhong, 2015-11-25 20:46:26]

https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:793: 'cast_shlib_consumer_ldflags',
This only covers cast_shell.

How about other exec such as update_engine and unittests?

Why not just change chromium/src/build/common.gypi, where we already have
-static-libstdc++ and -static-libgcc?

[bcf, 2015-11-25 21:19:38]

On 2015/11/25 20:46:26, wzhong wrote:
> https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp
> File chromecast/chromecast.gyp (right):
> 
>
https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp#n...
> chromecast/chromecast.gyp:793: 'cast_shlib_consumer_ldflags',
> This only covers cast_shell.
> 
> How about other exec such as update_engine and unittests?
> 
> Why not just change chromium/src/build/common.gypi, where we already have
> -static-libstdc++ and -static-libgcc?

If we add the options there, then all the shared libraries and executables which
don't use libcast* will also have the whole stdlibc++. This will result in
correct behavior, but will blow up the OTA size.

[halliwell, 2015-11-25 21:21:34]

On 2015/11/25 21:19:38, bcf wrote:
> On 2015/11/25 20:46:26, wzhong wrote:
> > https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp
> > File chromecast/chromecast.gyp (right):
> > 
> >
>
https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp#n...
> > chromecast/chromecast.gyp:793: 'cast_shlib_consumer_ldflags',
> > This only covers cast_shell.
> > 
> > How about other exec such as update_engine and unittests?
> > 
> > Why not just change chromium/src/build/common.gypi, where we already have
> > -static-libstdc++ and -static-libgcc?
> 
> If we add the options there, then all the shared libraries and executables
which
> don't use libcast* will also have the whole stdlibc++. This will result in
> correct behavior, but will blow up the OTA size.

Unit tests and mirroring definitely needs this.  What is the OTA size impact for
applying this to cast_shell and mirroring?

[bcf, 2015-11-25 21:48:03]

On 2015/11/25 21:21:34, halliwell wrote:
> On 2015/11/25 21:19:38, bcf wrote:
> > On 2015/11/25 20:46:26, wzhong wrote:
> > >
https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp
> > > File chromecast/chromecast.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp#n...
> > > chromecast/chromecast.gyp:793: 'cast_shlib_consumer_ldflags',
> > > This only covers cast_shell.
> > > 
> > > How about other exec such as update_engine and unittests?
> > > 
> > > Why not just change chromium/src/build/common.gypi, where we already have
> > > -static-libstdc++ and -static-libgcc?
> > 
> > If we add the options there, then all the shared libraries and executables
> which
> > don't use libcast* will also have the whole stdlibc++. This will result in
> > correct behavior, but will blow up the OTA size.
> 
> Unit tests and mirroring definitely needs this.  What is the OTA size impact
for
> applying this to cast_shell and mirroring?

Here's a size comparison with and without these changes applied:

No changes
$ ls -l out/target/product/chorizo/chorizo-ota-eng.bcf.zip
-rw-r----- 1 bcf eng 109064097 Nov 25 13:34
out/target/product/chorizo/chorizo-ota-eng.bcf.zip

With options applied to cast_shell and mirroring
$ ls -l out/target/product/chorizo/chorizo-ota-eng.bcf.zip
-rw-r----- 1 bcf eng 109541350 Nov 25 13:45
out/target/product/chorizo/chorizo-ota-eng.bcf.zip

[wzhong, 2015-11-25 22:05:14]

The difference is 500K.

(Bug I guess you have export for update_engine and v2mirroring yet, which you
need to do anyway).

It is not insignificant but it is worthwhile as it greatly simplifies
maintenance.

[halliwell, 2015-11-25 22:07:59]

On 2015/11/25 22:05:14, wzhong wrote:
> The difference is 500K.
> 
> (Bug I guess you have export for update_engine and v2mirroring yet, which you
> need to do anyway).
> 
> It is not insignificant but it is worthwhile as it greatly simplifies
> maintenance.

Sounds good.  Please update unit tests also.

[bcf, 2015-11-25 22:09:31]

On 2015/11/25 22:05:14, wzhong wrote:
> The difference is 500K.
> 
> (Bug I guess you have export for update_engine and v2mirroring yet, which you
> need to do anyway).
> 
> It is not insignificant but it is worthwhile as it greatly simplifies
> maintenance.

Ideally we could apply these options to any executable which depends on
cast_public_api or cast_public_api_internal. Is this possible in gyp/gn?

[bcf, 2015-11-25 23:13:52]

On 2015/11/25 22:09:31, bcf wrote:
> On 2015/11/25 22:05:14, wzhong wrote:
> > The difference is 500K.
> > 
> > (Bug I guess you have export for update_engine and v2mirroring yet, which
you
> > need to do anyway).
> > 
> > It is not insignificant but it is worthwhile as it greatly simplifies
> > maintenance.
> 
> Ideally we could apply these options to any executable which depends on
> cast_public_api or cast_public_api_internal. Is this possible in gyp/gn?

PS2 applies the options to only executables.

$ ls -l out/target/product/chorizo/chorizo-ota-eng.bcf.zip
-rw-r----- 1 bcf eng 110585240 Nov 25 15:10
out/target/product/chorizo/chorizo-ota-eng.bcf.zip

Difference is about 1.5MB which I think is acceptable.

[bcf, 2015-11-25 23:38:23]

Description was changed from

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast_shell.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

BUG= internal b/25566835
==========

to

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

BUG= internal b/25566835
==========

[wzhong, 2015-11-25 23:39:53]

lgtm

[halliwell, 2015-11-25 23:46:19]

On 2015/11/25 23:39:53, wzhong wrote:
> lgtm

lgtm - is there follow-up needed on gn with slan?

[bcf, 2015-11-26 00:02:21]

On 2015/11/25 23:46:19, halliwell wrote:
> On 2015/11/25 23:39:53, wzhong wrote:
> > lgtm
> 
> lgtm - is there follow-up needed on gn with slan?

Yes, I don't know how to add unmodified parameters to the end of the link line
for GN, slan said there was a way. Also, I think he said

'target_conditions': [
  [ '_type=="executable"', {

doesn't exist in GN, but an alternative method was possible.

[wzhong, 2015-11-26 00:22:47]

_type=="executable is used by Android and iOS in the same common.gypi.

How do they migrate to .gn? We can make a feature request if it is not available
yet.

[wzhong, 2015-11-30 17:05:05]

This CL fixes b/25767500 as well.

[slan, 2015-11-30 17:12:45]

On 2015/11/25 19:31:23, bcf wrote:
> https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp
> File chromecast/chromecast.gyp (right):
> 
>
https://codereview.chromium.org/1476923004/diff/1/chromecast/chromecast.gyp#n...
> chromecast/chromecast.gyp:81: 'target_name': 'cast_shlib_consumer_ldflags',
> This is a target any executable which uses libcast* shlibs should depend on.
> 
> I assume we'd accomplish the same thing in gn using a config, but how can we
add
> ldflags to the end of link line (this is important)?

We should be able to get the ordering right by making this a target default for
Cast builds. See 'gn help ldflags'.

> 
> gn provides `libs`, but it prepends to each option: "Expands to the list of
> system libraries to link to. Each will be prefixed by the "lib_prefix".". For
> example "foo" will become "-lfoo"

Not sure what you mean here, you would use the ldflags for this purpose, no?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

[slan, 2015-11-30 17:20:37]

On 2015/11/26 00:02:21, bcf wrote:
> On 2015/11/25 23:46:19, halliwell wrote:
> > On 2015/11/25 23:39:53, wzhong wrote:
> > > lgtm
> > 
> > lgtm - is there follow-up needed on gn with slan?
> 
> Yes, I don't know how to add unmodified parameters to the end of the link line
> for GN, slan said there was a way. Also, I think he said
> 
> 'target_conditions': [
>   [ '_type=="executable"', {
> 
> doesn't exist in GN, but an alternative method was possible.

Sorry that I'm a little late to the party.

We could do this a couple of different ways:

1) Define a "chromecast_config" with all of the right flags and apply it
explicitly to all of our executables and tests.
  Pros: Very clear and explicit about what's happening
  Cons: Maintenance

2) Apply this to the target defaults in BUILDCONFIG.gn (see
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
for example)
  Pros: Maintenance, handled in exactly one place.
  Cons: Not very clear, introduces knowledge of chromecast builds to
BUILDCONFIG.gn

I tend to lean toward #1, but our hand may be forced into exploring #2 because
of the ldflag ordering issue. bcf@, let's experiment locally today.
  

 One would be less intrusive than the other, but might require more maint

[bcf, 2015-11-30 22:11:19]

I decided to go with the BUILDCONFIG route for maintenance.

What do others think?

[slan, 2015-12-01 19:28:53]

On 2015/11/30 22:11:19, bcf wrote:
> I decided to go with the BUILDCONFIG route for maintenance.
> 
> What do others think?

+dpranke@

Hey Dirk, 

We need to add some ldflags to our executables and shlibs. I know we've tried to
avoid adding is_chromecast to BUILDCONFIG.gn. common.gypi already has
significant use of 'chromecast', but I wonder if we should hold BUILDCONFIG.gn
to a higher standard? WDYT?

[slan, 2015-12-01 19:29:31]

slan@chromium.org changed reviewers:
+ dpranke@google.com

[slan, 2015-12-01 19:29:32]

+dpranke@ for real this time.

Dirk, please see the last CL comment.

https://codereview.chromium.org/1476923004/diff/120001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1476923004/diff/120001/build/common.gypi#newc...
build/common.gypi:4154: '-Wl,--export-dynamic',
If we go this route, this needs to be echoed in GN.

[bcf, 2015-12-01 19:52:31]

https://codereview.chromium.org/1476923004/diff/120001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1476923004/diff/120001/build/common.gypi#newc...
build/common.gypi:4154: '-Wl,--export-dynamic',
On 2015/12/01 19:29:32, slan wrote:
> If we go this route, this needs to be echoed in GN.

Done.

[Dirk Pranke, 2015-12-01 20:23:23]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2015-12-01 20:23:23]

this lgtm, but I'm not 100% sure I understand the implications of these linker
flags, so thakis@ or thestig@ might be a better reviewer if you really wanted
another set of eyes, rather than just an owner.

[bcf, 2015-12-01 21:02:47]

bcf@chromium.org changed reviewers:
+ thakis@chromium.org

[bcf, 2015-12-01 21:03:03]

On 2015/12/01 20:23:23, Dirk Pranke wrote:
> this lgtm, but I'm not 100% sure I understand the implications of these linker
> flags, so thakis@ or thestig@ might be a better reviewer if you really wanted
> another set of eyes, rather than just an owner.

+ thakis@

Nico, do you have any opinions on this change?

[byungchul, 2015-12-01 21:22:48]

https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi#newc...
build/common.gypi:4154: '-Wl,--export-dynamic',
Why isn't it only for executable?

https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
File build/config/chromecast/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
build/config/chromecast/BUILD.gn:11: "-Wl,--export-dynamic",
Why isn't it only for executables?

[bcf, 2015-12-01 21:28:40]

On 2015/12/01 21:22:48, byungchul wrote:
> https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi#newc...
> build/common.gypi:4154: '-Wl,--export-dynamic',
> Why isn't it only for executable?
> 
>
https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
> File build/config/chromecast/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
> build/config/chromecast/BUILD.gn:11: "-Wl,--export-dynamic",
> Why isn't it only for executables?

We want there to be only one copy of all libstdc++ symbols at runtime, so we
must export them to ensure that they merge.

[Nico, 2015-12-01 21:32:27]

(i left a few comments on your doc. consider making docs linked on
open-source cls world-readable.)

On Tue, Dec 1, 2015 at 4:28 PM, <bcf@chromium.org> wrote:

> On 2015/12/01 21:22:48, byungchul wrote:
>
>> https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi
>> File build/common.gypi (right):
>>
>
>
>
>
https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi#newc...
>
>> build/common.gypi:4154: '-Wl,--export-dynamic',
>> Why isn't it only for executable?
>>
>
>
>
>
https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
>
>> File build/config/chromecast/BUILD.gn (right):
>>
>
>
>
>
https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
>
>> build/config/chromecast/BUILD.gn:11: "-Wl,--export-dynamic",
>> Why isn't it only for executables?
>>
>
> We want there to be only one copy of all libstdc++ symbols at runtime, so
> we
> must export them to ensure that they merge.
>
> https://codereview.chromium.org/1476923004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[byungchul, 2015-12-01 21:40:54]

On 2015/12/01 21:28:40, bcf wrote:
> On 2015/12/01 21:22:48, byungchul wrote:
> > https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1476923004/diff/140001/build/common.gypi#newc...
> > build/common.gypi:4154: '-Wl,--export-dynamic',
> > Why isn't it only for executable?
> > 
> >
>
https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
> > File build/config/chromecast/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1476923004/diff/140001/build/config/chromecas...
> > build/config/chromecast/BUILD.gn:11: "-Wl,--export-dynamic",
> > Why isn't it only for executables?
> 
> We want there to be only one copy of all libstdc++ symbols at runtime, so we
> must export them to ensure that they merge.

Actually, they aren't merged. But, linker will use first symbol to resolve
unresolved symbols. Are you sure that all reference use same symbol? Any chance
of shlibs using different symbols?

[wzhong, 2015-12-01 22:05:33]

Executable itself is always the first in symbol lookup scope at runtime. See
section 1.5.4 in http://www.akkadia.org/drepper/dsohowto.pdf.

What proposed here is:

 - Executable includes all libstdc++ symbols and export them.
 - libA.so includes symbolA and export it.
 - libB.so includes symbolB and export it.

At runtime, ld.so resolves both symbolA and symbolB to the copy inside
executable.

We believe this proposal best mimics the behavior when libstdc++.so is
dynamically loaded where all symbols are resolved to the same copy inside
libstdc++.so.

[byungchul, 2015-12-01 22:29:41]

On 2015/12/01 22:05:33, wzhong wrote:
> Executable itself is always the first in symbol lookup scope at runtime. See
> section 1.5.4 in http://www.akkadia.org/drepper/dsohowto.pdf.
> 
> What proposed here is:
> 
>  - Executable includes all libstdc++ symbols and export them.
>  - libA.so includes symbolA and export it.
>  - libB.so includes symbolB and export it.
> 
> At runtime, ld.so resolves both symbolA and symbolB to the copy inside
> executable.
> 
> We believe this proposal best mimics the behavior when libstdc++.so is
> dynamically loaded where all symbols are resolved to the same copy inside
> libstdc++.so.

To clarify, exec includes all symbols of libstdc++ including symbol A.
When libX.so refers A, it statically links libstdc++ and includes symbol A.
--export-dynamic makes symbol A reference in libX.so resolved to symbol A in
exec, but not one in libX.so.
Is it correct?
What I don't understand is what --export-dynamic does actually. Wonder if it
works same without that flag in shlibs.

[bcf, 2015-12-02 00:53:19]

On 2015/12/01 22:29:41, byungchul wrote:
> On 2015/12/01 22:05:33, wzhong wrote:
> > Executable itself is always the first in symbol lookup scope at runtime. See
> > section 1.5.4 in http://www.akkadia.org/drepper/dsohowto.pdf.
> > 
> > What proposed here is:
> > 
> >  - Executable includes all libstdc++ symbols and export them.
> >  - libA.so includes symbolA and export it.
> >  - libB.so includes symbolB and export it.
> > 
> > At runtime, ld.so resolves both symbolA and symbolB to the copy inside
> > executable.
> > 
> > We believe this proposal best mimics the behavior when libstdc++.so is
> > dynamically loaded where all symbols are resolved to the same copy inside
> > libstdc++.so.
> 
> To clarify, exec includes all symbols of libstdc++ including symbol A.
> When libX.so refers A, it statically links libstdc++ and includes symbol A.
> --export-dynamic makes symbol A reference in libX.so resolved to symbol A in
> exec, but not one in libX.so.
> Is it correct?
> What I don't understand is what --export-dynamic does actually. Wonder if it
> works same without that flag in shlibs.

Yes your explanation of the desired behavior is correct.

The docs (https://sourceware.org/binutils/docs/ld/Options.html) only refers to
executables in the section for  --export-dynamic.

I looked at the source code for both bfd and gold linkers and verified that it
has no effect on shared libraries (it's as if --export-dynamic is always on for
shared libraries).

So I removed this option from the shared library options.

[byungchul, 2015-12-02 01:03:22]

https://codereview.chromium.org/1476923004/diff/160001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1476923004/diff/160001/build/common.gypi#newc...
build/common.gypi:4169: # merge at runtime.
The right description would be, "export the libstdc++ symbols of executables so
shlibs refer them instead of ones defined within them".

[bcf, 2015-12-02 02:26:02]

https://codereview.chromium.org/1476923004/diff/160001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1476923004/diff/160001/build/common.gypi#newc...
build/common.gypi:4169: # merge at runtime.
On 2015/12/02 01:03:22, byungchul wrote:
> The right description would be, "export the libstdc++ symbols of executables
so
> shlibs refer them instead of ones defined within them".

Okay I reworded this sentence.

[bcf, 2015-12-02 02:37:24]

https://codereview.chromium.org/1476923004/diff/180001/build/config/chromecas...
File build/config/chromecast/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/180001/build/config/chromecas...
build/config/chromecast/BUILD.gn:32: configs = [ ":static_config" ]
@wzhong, we talked about how this may not be necessary for executables because
of the -l:libstdc++.a, but it seems that it is necessary otherwise cast_shell
will have a dynamic dependency on libstdc++.so.

[wzhong, 2015-12-02 02:42:52]

https://codereview.chromium.org/1476923004/diff/180001/build/config/chromecas...
File build/config/chromecast/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/180001/build/config/chromecas...
build/config/chromecast/BUILD.gn:32: configs = [ ":static_config" ]
On 2015/12/02 02:37:23, bcf wrote:
> @wzhong, we talked about how this may not be necessary for executables because
> of the -l:libstdc++.a, but it seems that it is necessary otherwise cast_shell
> will have a dynamic dependency on libstdc++.so.

Please add this to comment as this is not obvious and seems redundant at first
glance.

[bcf, 2015-12-02 03:20:23]

https://codereview.chromium.org/1476923004/diff/180001/build/config/chromecas...
File build/config/chromecast/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/180001/build/config/chromecas...
build/config/chromecast/BUILD.gn:32: configs = [ ":static_config" ]
On 2015/12/02 02:42:52, wzhong wrote:
> On 2015/12/02 02:37:23, bcf wrote:
> > @wzhong, we talked about how this may not be necessary for executables
because
> > of the -l:libstdc++.a, but it seems that it is necessary otherwise
cast_shell
> > will have a dynamic dependency on libstdc++.so.
> 
> Please add this to comment as this is not obvious and seems redundant at first
> glance. 

Done.

[wzhong, 2015-12-02 04:53:57]

lgtm

[slan, 2015-12-02 15:25:23]

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

[slan, 2015-12-02 15:25:24]

lgtm % question

+brettw@ for BUILDCONFIG.gn OWNERS

https://codereview.chromium.org/1476923004/diff/200001/build/config/chromecas...
File build/config/chromecast/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/200001/build/config/chromecas...
build/config/chromecast/BUILD.gn:5: assert(is_chromecast)
I'm surprised you don't need to import //build/config/chromecast_build.gni for
this gn arg. No error here?

[brettw, 2015-12-02 18:43:36]

https://codereview.chromium.org/1476923004/diff/200001/build/config/BUILDCONF...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/1476923004/diff/200001/build/config/BUILDCONF...
build/config/BUILDCONFIG.gn:510: } else if (is_linux || is_android ||
is_chromecast) {
You can't use is_chromecast from here. As described in my following comment,
you're seeing it being implicitly global in your build since you've set it
externally.

If we need a separate config here (I asked a question on this below), we should
refactor this so there's one "//build/config:executable_config" config and that
conditionally references the platform-specific ones based on the various flags.

https://codereview.chromium.org/1476923004/diff/200001/build/config/chromecas...
File build/config/chromecast/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/200001/build/config/chromecas...
build/config/chromecast/BUILD.gn:5: assert(is_chromecast)
On 2015/12/02 15:25:24, slan wrote:
> I'm surprised you don't need to import //build/config/chromecast_build.gni for
> this gn arg. No error here?

externally set build args are implicitly global (this is a bug I want to fix).
But this means that if you're chromecast, the variable will be set. So the
assert is "correct" but the failure case won't be an assertion failed, it will
be an undefined variable. We probably want the import here.

https://codereview.chromium.org/1476923004/diff/200001/build/config/chromecas...
build/config/chromecast/BUILD.gn:19: "-Wl,--export-dynamic",
I'd like to understand why this is so different than other systems like linux,
android, and chromeos. This looks completely different from all of them, which
seems suspicious to me.

[bcf, 2015-12-02 19:12:27]

https://codereview.chromium.org/1476923004/diff/200001/build/config/chromecas...
File build/config/chromecast/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/200001/build/config/chromecas...
build/config/chromecast/BUILD.gn:19: "-Wl,--export-dynamic",
On 2015/12/02 18:43:35, brettw wrote:
> I'd like to understand why this is so different than other systems like linux,
> android, and chromeos. This looks completely different from all of them, which
> seems suspicious to me.

The main issue is we are forced to use -static-libstdc++ due to licensing
reasons. This was previously defined in common.gypi, but is currently absent
from GN (so the ":static_config" options need to be added regardless).

We communicate with some shared libraries using C++ APIs so when both the main
executable and shlibs are compiled with -static-libstdc++, it causes subtle bugs
when there are duplicate copies of libstdc++ symbols. The goal of this CL is to
force all libstdc++/libgcc symbols to be exported by the executables to avoid
these issues.

The linked doc provides some background on this issue.
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

[bcf, 2015-12-02 23:33:01]

https://codereview.chromium.org/1476923004/diff/200001/build/config/BUILDCONF...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/1476923004/diff/200001/build/config/BUILDCONF...
build/config/BUILDCONFIG.gn:510: } else if (is_linux || is_android ||
is_chromecast) {
On 2015/12/02 18:43:35, brettw wrote:
> You can't use is_chromecast from here. As described in my following comment,
> you're seeing it being implicitly global in your build since you've set it
> externally.
> 
> If we need a separate config here (I asked a question on this below), we
should
> refactor this so there's one "//build/config:executable_config" config and
that
> conditionally references the platform-specific ones based on the various
flags.

Okay. How does something like PS12 look?

[bcf, 2015-12-04 18:52:09]

On 2015/12/02 23:33:01, bcf wrote:
>
https://codereview.chromium.org/1476923004/diff/200001/build/config/BUILDCONF...
> File build/config/BUILDCONFIG.gn (right):
> 
>
https://codereview.chromium.org/1476923004/diff/200001/build/config/BUILDCONF...
> build/config/BUILDCONFIG.gn:510: } else if (is_linux || is_android ||
> is_chromecast) {
> On 2015/12/02 18:43:35, brettw wrote:
> > You can't use is_chromecast from here. As described in my following comment,
> > you're seeing it being implicitly global in your build since you've set it
> > externally.
> > 
> > If we need a separate config here (I asked a question on this below), we
> should
> > refactor this so there's one "//build/config:executable_config" config and
> that
> > conditionally references the platform-specific ones based on the various
> flags.
> 
> Okay. How does something like PS12 look?

Any updates on this?

[slan, 2015-12-07 16:13:32]

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn#...
build/config/BUILD.gn:428: config("executable_config") {
Hmmm. Though it appears to be legal, (meaning GN throws no warning and seems to
forward things correctly) I'm not certain that having recursive configs is good
style. I will defer to Brett and Dirk on this one, but I have not seen this
pattern, and it is not explicitly called out in 'gn help config' as a legal
design.

If this is indeed invalid, then may I propose as an alternative: 
BUILDCONFIG.gn remains unchanged, except that
//builg/config/linux:executable_configs is added to the list of configs if
is_linux, where this target has the form:

config("executable_config") {
  if (is_chromecast) {
    # Ample comments and all needed properties go here directly.
    ldflags = [ ... ]
  }
}

You would then no longer need the build/config/chromecast/* targets, and
BUILDCONFIG would not change much.

However, if the design in PS12 is valid, feel free to ignore this comment :)

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn#...
build/config/BUILD.gn:438: } else if (is_linux || is_android || is_chromecast) {
is_chromecast not needed here. The OS flags should be enough.

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn#...
build/config/BUILD.gn:463: configs += [ "//build/config/chromecast:shlib_config"
]
nit: Can this be shared_library_config to match the style elsewhere?

[Dirk Pranke, 2015-12-07 21:03:42]

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn#...
build/config/BUILD.gn:428: config("executable_config") {
On 2015/12/07 16:13:32, slan wrote:
> Hmmm. Though it appears to be legal, (meaning GN throws no warning and seems
to
> forward things correctly) I'm not certain that having recursive configs is
good
> style. I will defer to Brett and Dirk on this one, but I have not seen this
> pattern, and it is not explicitly called out in 'gn help config' as a legal
> design.

This is valid and recommended in some cases.

See the discussion of "Configs on a config" in `gn help configs` (with an s).

(The feature is relatively new, so it's not used as widely as it should be).

[slan, 2015-12-07 21:07:47]

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn#...
build/config/BUILD.gn:428: config("executable_config") {
On 2015/12/07 21:03:42, Dirk Pranke wrote:
> On 2015/12/07 16:13:32, slan wrote:
> > Hmmm. Though it appears to be legal, (meaning GN throws no warning and seems
> to
> > forward things correctly) I'm not certain that having recursive configs is
> good
> > style. I will defer to Brett and Dirk on this one, but I have not seen this
> > pattern, and it is not explicitly called out in 'gn help config' as a legal
> > design.
> 
> This is valid and recommended in some cases.
> 
> See the discussion of "Configs on a config" in `gn help configs` (with an s).
> 
> (The feature is relatively new, so it's not used as widely as it should be).

Thanks for the tip! bcf@, feel free to ignore my suggestion, it sounds like this
is a good use case for "configs on a config".

[bcf, 2015-12-07 21:39:49]

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn#...
build/config/BUILD.gn:438: } else if (is_linux || is_android || is_chromecast) {
On 2015/12/07 16:13:32, slan wrote:
> is_chromecast not needed here. The OS flags should be enough.

Done.

https://codereview.chromium.org/1476923004/diff/220001/build/config/BUILD.gn#...
build/config/BUILD.gn:463: configs += [ "//build/config/chromecast:shlib_config"
]
On 2015/12/07 16:13:32, slan wrote:
> nit: Can this be shared_library_config to match the style elsewhere?

Done.

[brettw, 2015-12-08 01:08:58]

https://codereview.chromium.org/1476923004/diff/240001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/240001/build/config/BUILD.gn#...
build/config/BUILD.gn:423: "//build/config/win:console",
This won't work since targets need to override it with the "windowed" config.
Both chrome and content_shell expect to be able to remove this config from the
default list and add back their own, and I think this is common enough we need
to support doing this easily.

The same for incremental linking.

The other configs should be OK to move here. I see the mini_installer removes
"...:common_linker_setup" but it wants to manually do most stuff. In that case,
you can fix that code to remove the main "...:executable_config" and then put
back the ones you need.

[bcf, 2015-12-08 02:22:26]

https://codereview.chromium.org/1476923004/diff/240001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1476923004/diff/240001/build/config/BUILD.gn#...
build/config/BUILD.gn:423: "//build/config/win:console",
On 2015/12/08 01:08:58, brettw wrote:
> This won't work since targets need to override it with the "windowed" config.
> Both chrome and content_shell expect to be able to remove this config from the
> default list and add back their own, and I think this is common enough we need
> to support doing this easily.
> 
> The same for incremental linking.
> 
> The other configs should be OK to move here. I see the mini_installer removes
> "...:common_linker_setup" but it wants to manually do most stuff. In that
case,
> you can fix that code to remove the main "...:executable_config" and then put
> back the ones you need.

Done.

[brettw, 2015-12-08 05:01:40]

GN build LGTM

[bcf, 2015-12-08 18:38:42]

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

[commit-bot: I haz the power, 2015-12-08 18:40:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476923004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476923004/280001

[commit-bot: I haz the power, 2015-12-08 18:57:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 18:57:37]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[bcf, 2015-12-08 21:20:27]

I put "//build/config/android:hide_native_jni_exports" back in BUILDCONFIG.gn
because it needs to be removed by a target.

Decided to do this instead of changing the failing target to remove
"//build/config:shared_library_config" because the comment above
"//build/config/android:hide_native_jni_exports" implies that it may be a common
config to remove.

Thoughts?

[bcf, 2015-12-08 21:21:05]

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

[commit-bot: I haz the power, 2015-12-08 21:22:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476923004/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476923004/300001

[commit-bot: I haz the power, 2015-12-08 21:49:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 21:49:11]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[bcf, 2015-12-09 00:34:55]

https://codereview.chromium.org/1476923004/diff/320001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1476923004/diff/320001/build/common.gypi#newc...
build/common.gypi:4164: [ '_type=="executable" and OS!="android"', {
Android uses its own libstdc++ so we don't need these options.

The options are already excluded on GN because the conditional is
if (is_android) {
...
} else if (is_chromecast) ...

[bcf, 2015-12-09 00:35:06]

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

[commit-bot: I haz the power, 2015-12-09 00:46:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476923004/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476923004/320001

[commit-bot: I haz the power, 2015-12-09 02:42:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 02:42:09]

Dry run: This issue passed the CQ dry run.

[bcf, 2015-12-09 02:46:59]

On 2015/12/09 02:42:09, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Does anyone have any opinions about the changes since PS15?

[wzhong, 2015-12-09 16:51:21]

Please mention the CONFIG refactoring in commit message.

Otherwise LGTM.

[brettw, 2015-12-09 17:38:34]

I only briefly skimmed this but it looks fine.

[slan, 2015-12-09 18:08:12]

lgtm

[bcf, 2015-12-09 18:54:22]

Description was changed from

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

BUG= internal b/25566835
==========

to

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

For GN this change required refactoring the way default configs are applied to
executables and shared libraries. The targets //build/config:{executable_config,
shared_library_config} were created for this purpose.

BUG= internal b/25566835
==========

[bcf, 2015-12-09 18:57:45]

Description was changed from

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

For GN this change required refactoring the way default configs are applied to
executables and shared libraries. The targets //build/config:{executable_config,
shared_library_config} were created for this purpose.

BUG= internal b/25566835
==========

to

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835
==========

[bcf, 2015-12-09 19:07:53]

Description was changed from

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

Implementation of proposal:
https://docs.google.com/a/google.com/document/d/1ptjOlC5fH8G-DkHTEaO6GWnAXztf...

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835
==========

to

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835
==========

[bcf, 2015-12-09 19:08:25]

The CQ bit was checked by bcf@chromium.org

[bcf, 2015-12-09 19:08:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, dpranke@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1476923004/#ps320001
(title: "Disable executable options on Android.")

[commit-bot: I haz the power, 2015-12-09 19:09:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476923004/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476923004/320001

[commit-bot: I haz the power, 2015-12-09 19:16:47]

Description was changed from

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835
==========

to

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835
==========

[commit-bot: I haz the power, 2015-12-09 19:16:49]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-12-09 19:17:35]

Description was changed from

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835
==========

to

==========
[Chromecast] Include all stdlibc++/libgcc symbols in cast executables.

For GN this change required refactoring the way default configs are
applied to executables and shared libraries. The targets
//build/config:{executable_config, shared_library_config} were
created for this purpose.

BUG= internal b/25566835

Committed: https://crrev.com/fbf4f9ac36e9e121620d65ca85dd998a67745049
Cr-Commit-Position: refs/heads/master@{#364118}
==========

[commit-bot: I haz the power, 2015-12-09 19:17:36]

Patchset 17 (id:??) landed as
https://crrev.com/fbf4f9ac36e9e121620d65ca85dd998a67745049
Cr-Commit-Position: refs/heads/master@{#364118}