android: don't export unnecessary symbols.

android: don't export unnecessary symbols.

Android binaries were relying on -Wl,--exclude-libs=ALL to prevent
unnecessary symbols from being exported in the final shared libraries
built in Chromium, however several changes were submitted that actually
disabled this linker flag in virtually all binaries, and thus it was not
really being used and many symbols were exported by mistake.

Remove --exclude-libs=ALL entirely from the Android build, as well as
all the places which were previously having to unset it, and instead
exclude the specific libraries which incorporate visible symbols that we
don't want: the NDK libraries which we statically link into the binary,
and a number of components of chromium which contain assembly code which
has not yet been fixed to set symbol visibilities correctly.

This reduces the number of exported symbols in the shared libraries to
only the ones we intended to export, which saves ~120KB binary size.

The GN build does not build the same set of things as static libraries,
so the list is slightly different there and several more symbols are
left in the final binaries which shouldn't be there, but are not pulled
in via a static library and so no --exclude-libs flag will ever fix it;
the symbol visibilities will need to be fixed at source.

BUG=448386
TBR=ben@chromium.org

Committed: https://crrev.com/813edf0d362baf1a2b01fe54bc27ff4601d59713
Cr-Commit-Position: refs/heads/master@{#317027}

[Torne, 2015-01-19 17:42:04]

torne@chromium.org changed reviewers:
+ cjhopman@chromium.org

[Torne, 2015-01-19 17:42:05]

Hi Chris,

I'm not sure what to do about the GN build here; mojo doesn't unset
--exclude-libs=ALL there, so probably right now the GN binaries only export the
correct minimal set of symbols, but that breaks being able to compile the
WebView in future since WebView expects to be able to export things and has to
unset --exclude-libs=ALL as well (not currently implemented in the GN build).

Probably we should just exclude the same list in both? I'm not sure where to put
that in gn though.

[Torne, 2015-01-21 12:29:09]

torne@chromium.org changed reviewers:
+ primiano@chromium.org

[Torne, 2015-01-21 12:29:10]

also +primiano as we discussed this some offline

[cjhopman, 2015-01-21 19:36:15]

lgtm.

I wonder, though, what behavior we really want. Should the default for every
target be that it is added to exclude-libs and then you must manually mark ones
to not be excluded? Or should it be default exported and we just have a way to
catch regressions and manually mark it as excluded?

For GN, -Wl,--exclude-libs=ALL is set in build/config/compiler/BUILD.gn. Just
globally removing it is straightforward, but I'm not sure if it will be easy (or
even possible) to remove it from dependents of a target (removing it from a
target itself is also simple).

[Torne, 2015-01-22 10:39:46]

On 2015/01/21 19:36:15, cjhopman wrote:
> lgtm.

We definitely need to also update the gn configuration before landing this :)

> I wonder, though, what behavior we really want. Should the default for every
> target be that it is added to exclude-libs and then you must manually mark
ones
> to not be excluded? Or should it be default exported and we just have a way to
> catch regressions and manually mark it as excluded?

So the basic problem I'm trying to solve is that mojo expects to be able to
export symbols, which is not possible if --exclude-libs=ALL is set since mojo is
a static library target in gyp. So, mojo unsets that, which means we end up with
hundreds and hundreds of exported symbols from the libraries I'm trying to
exclude here. I originally proposed a patch
https://codereview.chromium.org/843103003/ which started using a linker version
script for all android targets to specify an exact whitelist of exported
symbols, but the mojo devs objected to having to go and maintain a separate list
of their exports in addition to marking their symbols' visibility in the
sources. It seems pretty reasonable that marking a symbol visible should, in
general, actually work and cause it to be visible, so I came up with the
approach in this CL instead.

Chromium passes -fvisibility=hidden when compiling all C/C++ code, so even if we
don't exclude-libs at all, the only symbols which end up being visible are 1)
symbols defined in NDK libraries we statically link, which we can trivially
exclude as in this patch as they don't change over time, and 2) symbols defined
in assembly sources, which seems to be exclusively in third_party. I've filed a
bunch of bugs to get all the current assembly symbols marked hidden, and some of
these have already been fixed - the remainder are excluded in this CL
temporarily until they get fixed in the sources.

I guess we could do something pretty crazy like: in target_defaults in
common.gypi, unless the target defines a specific variable, have it set
link_settings: ld_flags: -Wl,--exclude-libs=lib<(target_name).a
That would mean that every static_library target in the build could exclude
itself from consideration unless it explicitly wanted to be included. I think
this is kinda weird and overcomplicated, though; do you think this'd be worth
it?

What seems like the easiest approach for now is to land this patch (and try and
fix the asm symbols in their sources so that this explicit list of exclusions
can eventually be reduced to only libgcc and stlport), and then put some bot
monitoring step in place similar to the one that checks for newly added static
initialisers to prevent people from adding new exports unintentionally. I guess
this still means that if mojo want to export a new symbol they have to do
something in addition to just marking it visible, though: they'd have to
rebaseline the monitoring bot. :/

> For GN, -Wl,--exclude-libs=ALL is set in build/config/compiler/BUILD.gn. Just
> globally removing it is straightforward, but I'm not sure if it will be easy
(or
> even possible) to remove it from dependents of a target (removing it from a
> target itself is also simple).

Currently mojo works in GN by not being a static library at all but just a
srcset which gets added to the main linkable target, which means that
--exclude-libs=ALL does not apply to it. This is a problem for the future
WebView GN build, because the WebView wants to use native JNI exports, which
requires exporting symbols from all over the chromium codebase for JNI binding
purposes and thus means that --exclude-libs=ALL cannot be used. We want to be
able to build the webview and chrome simultaneously with the same gyp/gn config,
and so to allow for this we will, eventually, need to remove --exclude-libs=ALL
entirely from the GN build as well. So, the GN build can't rely on just
excluding everything, and will need to adopt the same solution that I propose
here (or some alternative).

I don't want to just land this patch as-is really, I want us (and anyone else
you can suggest who might have good points) to decide how we actually want to
solve this problem now and going forward for the Android build. We don't have to
put all of the solution in place at once (e.g. I don't really care if we don't
get around to a bot monitoring for regressions for a while, because regressions
are likely to be rare anyway), but I'd prefer not to have to change approach a
bunch of times :)

(Of course, this is not an android-specific problem in general: the other
platforms *also* build shared libraries in some cases which presumably have the
same leaking sets of exports - but android builds *everything* as a shared
library, so it seems like we are more likely to notice).

[cjhopman, 2015-01-22 19:31:24]

On 2015/01/22 10:39:46, Torne wrote:
> On 2015/01/21 19:36:15, cjhopman wrote:
> > lgtm.
> 
> We definitely need to also update the gn configuration before landing this :)
> 
> > I wonder, though, what behavior we really want. Should the default for every
> > target be that it is added to exclude-libs and then you must manually mark
> ones
> > to not be excluded? Or should it be default exported and we just have a way
to
> > catch regressions and manually mark it as excluded?
> 
> So the basic problem I'm trying to solve is that mojo expects to be able to
> export symbols, which is not possible if --exclude-libs=ALL is set since mojo
is
> a static library target in gyp. So, mojo unsets that, which means we end up
with
> hundreds and hundreds of exported symbols from the libraries I'm trying to
> exclude here. I originally proposed a patch
> https://codereview.chromium.org/843103003/ which started using a linker
version
> script for all android targets to specify an exact whitelist of exported
> symbols, but the mojo devs objected to having to go and maintain a separate
list
> of their exports in addition to marking their symbols' visibility in the
> sources. It seems pretty reasonable that marking a symbol visible should, in
> general, actually work and cause it to be visible, so I came up with the
> approach in this CL instead.
> 
> Chromium passes -fvisibility=hidden when compiling all C/C++ code, so even if
we
> don't exclude-libs at all, the only symbols which end up being visible are 1)
> symbols defined in NDK libraries we statically link, which we can trivially
> exclude as in this patch as they don't change over time, and 2) symbols
defined
> in assembly sources, which seems to be exclusively in third_party. I've filed
a
> bunch of bugs to get all the current assembly symbols marked hidden, and some
of
> these have already been fixed - the remainder are excluded in this CL
> temporarily until they get fixed in the sources.
> 
> I guess we could do something pretty crazy like: in target_defaults in
> common.gypi, unless the target defines a specific variable, have it set
> link_settings: ld_flags: -Wl,--exclude-libs=lib<(target_name).a
> That would mean that every static_library target in the build could exclude
> itself from consideration unless it explicitly wanted to be included. I think
> this is kinda weird and overcomplicated, though; do you think this'd be worth
> it?
> 
> What seems like the easiest approach for now is to land this patch (and try
and
> fix the asm symbols in their sources so that this explicit list of exclusions
> can eventually be reduced to only libgcc and stlport), and then put some bot
> monitoring step in place similar to the one that checks for newly added static
> initialisers to prevent people from adding new exports unintentionally. I
guess
> this still means that if mojo want to export a new symbol they have to do
> something in addition to just marking it visible, though: they'd have to
> rebaseline the monitoring bot. :/
> 
> > For GN, -Wl,--exclude-libs=ALL is set in build/config/compiler/BUILD.gn.
Just
> > globally removing it is straightforward, but I'm not sure if it will be easy
> (or
> > even possible) to remove it from dependents of a target (removing it from a
> > target itself is also simple).
> 
> Currently mojo works in GN by not being a static library at all but just a
> srcset which gets added to the main linkable target, which means that
> --exclude-libs=ALL does not apply to it. This is a problem for the future
> WebView GN build, because the WebView wants to use native JNI exports, which
> requires exporting symbols from all over the chromium codebase for JNI binding
> purposes and thus means that --exclude-libs=ALL cannot be used. We want to be
> able to build the webview and chrome simultaneously with the same gyp/gn
config,
> and so to allow for this we will, eventually, need to remove
--exclude-libs=ALL
> entirely from the GN build as well. So, the GN build can't rely on just
> excluding everything, and will need to adopt the same solution that I propose
> here (or some alternative).
> 
> I don't want to just land this patch as-is really, I want us (and anyone else
> you can suggest who might have good points) to decide how we actually want to
> solve this problem now and going forward for the Android build. We don't have
to
> put all of the solution in place at once (e.g. I don't really care if we don't
> get around to a bot monitoring for regressions for a while, because
regressions
> are likely to be rare anyway), but I'd prefer not to have to change approach a
> bunch of times :)
> 
> (Of course, this is not an android-specific problem in general: the other
> platforms *also* build shared libraries in some cases which presumably have
the
> same leaking sets of exports - but android builds *everything* as a shared
> library, so it seems like we are more likely to notice).

What are the symbols that mojo libraries want to export? Are these things from
the mojom files? Could mojom processing write a .defs file (aiui, these will
override --exclude-libs=ALL) for these symbols and just add it to the link
settings of the corresponding library?

[Torne, 2015-01-23 12:00:01]

On 2015/01/22 19:31:24, cjhopman wrote:
> What are the symbols that mojo libraries want to export? Are these things from
> the mojom files?

I'm not sure exactly but I don't think they are generated symbols, I think they
are just things that the separate mojo .so expects to be able to call from its
"host" binary that are fixed. Some of the comments in the mojo gyp files around
where it removes --exclude-libs=ALL mention some cases, but I'm not sure this is
exhaustive.

> Could mojom processing write a .defs file (aiui, these will
> override --exclude-libs=ALL) for these symbols and just add it to the link
> settings of the corresponding library?

Do .def files apply to ELF shared objects? I thought that was just for DLL
linkage. The only mechanisms I know of to control what is exported in our .so
are the visibilities of the individual symbols, and a linker version script
which was the approach used in the other CL.

I don't think this is really a mojo-specific problem. It happens that in most
current cases (other than the component build) our Android binaries are just a
single library and so they haven't needed to export any functions other than
JNI_OnLoad, but mojo being a separate library is one case that's now come up,
and the WebView's desire to use native JNI exports is another; I doubt we'll
have just these two cases forever.

It just seems to me like the nicest solution in terms of meeting expectations of
people making changes to chromium is for the symbol visibility flags that we
already use extensively (e.g. in component builds) to actually mean what they
say, rather than for them to be overridden weirdly by --exclude-libs in an
android-specific way that other platform developers are not really aware of
until it bites them, especially since I don't see any evidence that the problem
being solved is actually android-specific. This means we will probably have to
have something in place to stop us accidentally regressing, but I don't think
that's any harder to do than many of the things we already monitor.

[Torne, 2015-01-29 13:40:42]

I'm not sure I can think of a better way to do this, and it seems like we should
probably land a version of this which:

1) actually removes -Wl,--exclude-libs=ALL from build/common.gypi to stop people
from mistakenly assuming it actually does anything useful when in reality most
of the chromium targets unset the flag anyway
2) changes the GN configuration to match, i.e. also drop -Wl,--exclude-libs=ALL
there even though it currently *is* working, and introduce the same exclusions
that I have in this patch.

This will mean that the gyp and gn builds aren't confusingly different any more,
and will allow for me to do the JNI export change I want to make in the near
future which will export additional symbols from all the places in chromium that
do JNI, which currently isn't possible in the gn build because it still excludes
all.

I haven't actually written such a patch yet, but does that seem sane for now? If
we come up with a better option in future then we can change stuff, but I think
that despite the attractiveness of a whitelist-based approach, it's just going
to cause a lot of confusion for chromium developers who are surprised that
android is behaving differently.

[Torne, 2015-01-29 18:10:20]

...although having just posted that, the exported symbols have already regressed
since I updated this patch - boringssl now includes a bunch of assembly symbols
that get incorrectly exported. :/

[Torne, 2015-02-17 10:38:19]

Updated the patch to completely remove --exclude-libs=ALL since it turns out
that it has actually been disabled for all shared libraries since July anyway,
even without the mojo change that removed it for libraries that depend on mojo,
and therefore pretending we are using it is misleading.

I still need to update this patch to bring the GN configuration in line, but
uploading now to send to non-GN trybots.

[Torne, 2015-02-17 10:39:28]

Oh, also there are still unwanted symbols exported from boringssl, but boringssl
is a component and thus can't be excluded unconditionally; rather than go to the
trouble of excluding it conditionally I'll just try and get it fixed upstream.

[Torne, 2015-02-18 10:49:28]

OK, this should be ready to go now; I've updated the GN build to match, though
"match" here is a pretty liberal description since the list of static libraries
is not the same.

This doesn't quite result in no unwanted symbols: boringssl is leaking some on
both gyp and gn which can't be trivially fixed without breaking the component
build, but agl is going to fix the code to mark them hidden. Also, on the gn
build, several things (especially opus) are not built as separate static
libraries at all, just srcsets, and so neither --exclude-libs=ALL nor
--exclude-libs=libopus.a has any effect on them, and their symbols are visible
both before and after this change. I'm bugging all the respective code owners to
fix them at source.

[Primiano Tucci (use gerrit), 2015-02-18 12:03:50]

LGTM. Torne and I had some long discussion offline.
This seem to have the very nice side effect of saving 120k of binary size (and
possibly startup time) by virtue of stop exporting unneeded libc & friends
symbols and removing GC roots for the linker GC sections.

This is the data we measured comparing a release build of libchromeshell.so for
arm32 with and without this patch.

elf section        w/ patch   origin     delta [bytes]
-----------------------------------------------
.text              25518252   25567968   -49716   
.dynstr            4356       35092      -30736   
.dynsym            6656       24416      -17760   
.hash              2724       10236      -7512    
.rel.dyn           2089816    2094224    -4408    
.ARM.extab         94220      98504      -4284    
.rodata            5469896    5472968    -3072    
.data.rel.ro       826544     828696     -2152    
.ARM.exidx         1172328    1173848    -1520    
.data.rel.ro.loca  602096     602400     -304     
.got               31512      31704      -192     
.plt               4100       4268       -168     
.rel.plt           2720       2832       -112     
.bss               185380     185460     -80      
.data              91496      91512      -16

[Torne, 2015-02-18 12:22:13]

torne@chromium.org changed reviewers:
+ ben@chromium.org

[Torne, 2015-02-18 12:22:14]

+ben for mojo/ OWNERS

[Torne, 2015-02-18 12:31:03]

On 2015/02/18 12:22:14, Torne wrote:
> +ben for mojo/ OWNERS

Ben, there are also various references to --exclude-libs=ALL (other places where
it's unset, and comments talking about it) in third_party/mojo. You may want to
clean these up too on mojo's side. Assuming this patch lands successfully I
don't think we will be going back to using --exclude-libs=ALL on android ever;
it makes it far too difficult to export symbols on purpose.

[Torne, 2015-02-19 10:15:18]

TBRing ben@ as the mojo change is just a cleanup of the now-obsolete setting in
the gyp file.

[Torne, 2015-02-19 10:15:24]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2015-02-19 10:16:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/806533003/40001

[commit-bot: I haz the power, 2015-02-19 10:20:27]

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

[commit-bot: I haz the power, 2015-02-19 10:20:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Torne, 2015-02-19 10:24:07]

New patchsets have been uploaded after l-g-t-m from primiano@chromium.org

[Torne, 2015-02-19 10:24:20]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2015-02-19 10:25:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/806533003/60001

[commit-bot: I haz the power, 2015-02-19 11:21:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-19 11:22:44]

Patchset 4 (id:??) landed as
https://crrev.com/813edf0d362baf1a2b01fe54bc27ff4601d59713
Cr-Commit-Position: refs/heads/master@{#317027}

[Nico, 2015-02-24 17:12:38]

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

[Nico, 2015-02-24 17:12:38]

do you have a check for the exported symbols somewhere? on mac, we have a
whitelist of allowed symbols and this gets checked at compile time. (without
such a check, this will break again)

[Torne, 2015-02-24 17:27:15]

On 2015/02/24 17:12:38, Nico wrote:
> do you have a check for the exported symbols somewhere? on mac, we have a
> whitelist of allowed symbols and this gets checked at compile time. (without
> such a check, this will break again)

No, we don't. Having a whitelist was explicitly objected to by several people
(the original version of this patch used a whitelist at link time).

The linked bug mentions our intention to have some kind of monitoring for this
but we haven't put it in place yet or worked out exactly what it should look
like. I was imagining something like the monitoring for static initialisers
based on the count, rather than a specific whitelist, but maybe that's not the
best approach?

[Nico, 2015-02-24 17:32:43]

What were the objections against a whitelist?

On Tue, Feb 24, 2015 at 9:27 AM, <torne@chromium.org> wrote:

> On 2015/02/24 17:12:38, Nico wrote:
>
>> do you have a check for the exported symbols somewhere? on mac, we have a
>> whitelist of allowed symbols and this gets checked at compile time.
>> (without
>> such a check, this will break again)
>>
>
> No, we don't. Having a whitelist was explicitly objected to by several
> people
> (the original version of this patch used a whitelist at link time).
>
> The linked bug mentions our intention to have some kind of monitoring for
> this
> but we haven't put it in place yet or worked out exactly what it should
> look
> like. I was imagining something like the monitoring for static initialisers
> based on the count, rather than a specific whitelist, but maybe that's not
> the
> best approach?
>
> https://codereview.chromium.org/806533003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Torne, 2015-02-24 17:47:12]

On 2015/02/24 17:32:43, Nico wrote:
> What were the objections against a whitelist?

Oh, sorry, the objections were not on this CL, but on this one:
https://codereview.chromium.org/843103003/

See earlier versions of the patch (versions 1-3) and the comments before I
changed approach.

> On Tue, Feb 24, 2015 at 9:27 AM, <mailto:torne@chromium.org> wrote:
> 
> > On 2015/02/24 17:12:38, Nico wrote:
> >
> >> do you have a check for the exported symbols somewhere? on mac, we have a
> >> whitelist of allowed symbols and this gets checked at compile time.
> >> (without
> >> such a check, this will break again)
> >>
> >
> > No, we don't. Having a whitelist was explicitly objected to by several
> > people
> > (the original version of this patch used a whitelist at link time).
> >
> > The linked bug mentions our intention to have some kind of monitoring for
> > this
> > but we haven't put it in place yet or worked out exactly what it should
> > look
> > like. I was imagining something like the monitoring for static initialisers
> > based on the count, rather than a specific whitelist, but maybe that's not
> > the
> > best approach?
> >
> > https://codereview.chromium.org/806533003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.