android: Hide JNI exports by default.

android: Hide JNI exports by default.

Hide JNI exported functions in Android binaries by default, unless the
target in question has explicitly set "use_native_jni_exports" to
indicate that it relies on the JVM's automatic symbol lookup mechanism.

The functions are simply demoted to hidden visibility; the code will
remain unless the linker determines that it is unreferenced and strips
it via --gc-sections.

This ensures that binaries by default actually test the explicit JNI
registration codepaths, which are required for compatibility with the
crazy linker, while still allowing binaries that do not require crazy
linker compatibility to choose to use the automatic mechanism in future.

BUG=442327
Committed: https://crrev.com/9d90d85f73ca46847047ccbda270f25b637ccc86
Cr-Commit-Position: refs/heads/master@{#316896}

[Torne, 2015-01-09 18:51:52]

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

[Torne, 2015-01-09 18:51:52]

Not finished yet, just FYI. This probably doesn't work yet as I've not done
anything to actually make the mojo symbols referred to in the mojo gyp comments
be exposed, and I haven't updated the gn files to match the gyp logic yet
either.

[Torne, 2015-01-12 16:10:56]

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

[Torne, 2015-01-12 16:10:57]

Ben, James, can either of you comment on what symbols mojo might need exported
from the main android libraries? The comments in the gyp files I've changed here
mention two symbol names that are supposed to be required, but none of our tests
fail even if I don't export them :/

We want to move to a purely whitelist-based approach to exporting symbols from
the Android libraries (in non-component builds), and thus the whitelist needs to
include any symbols mojo expects to be there :)

[Primiano Tucci (use gerrit), 2015-01-12 18:30:36]

LGTM with two notes.

1) Can you please reword a bit the commit message to explain the context? This
CL made perfect sense after some offline chat. I'm thinking about is something
as follows:

Currently on android we use -Wl,--exclude-libs=ALL to hide all symbols ...bla...
but that is not idea because ...bla.... only ones from static libraries.
exclude-libs=ALL is also problematic when a target (e.g., Mojo) wants to
deliberately export some symbols.
Removing the --exclude-libs=ALL from ldflags, in fact, is a big hammer and has
the side effect of exposing way too much symbols and unnecessarily extending the
linker gc-sections roots (read: bigger binary)

This change switch to use explicit linker scripts. This fixed the aforementioned
issue and allows to selectively whitelist individual symbols when needed.

Furthermore, a new per-target setting is introduced.... [same as current]

2) Can you take a look to the magic trickery in common.gypi to run PIEs on the
internal ICS bots using the run_pie wrapper? I think all you need, at the end,
is white-listing "main".

[jamesr, 2015-01-12 18:56:57]

jamesr@chromium.org changed reviewers:
+ qsr@chromium.org
- ben@chromium.org

[jamesr, 2015-01-12 18:58:50]

On 2015/01/12 16:10:57, Torne wrote:
> Ben, James, can either of you comment on what symbols mojo might need exported
> from the main android libraries?

What exactly are the "main android libraries"?

The comments in mojo/*gyp are about symbols that need to be exported from shared
libraries built out of the chrome tree.

[qsr, 2015-01-12 18:58:53]

Does this break android gn builds? I do not see any related change in gn
configs.

[qsr, 2015-01-12 19:01:10]

On 2015/01/12 18:58:53, qsr wrote:
> Does this break android gn builds? I do not see any related change in gn
> configs.

Hum, ok, no it will not break gn, but we will still need to use the same old
mechanism. Do you have any plan to port this to the GN build?

[Torne, 2015-01-12 19:07:30]

On 12 Jan 2015 6:58 pm, <jamesr@chromium.org> wrote:
>
> On 2015/01/12 16:10:57, Torne wrote:
>>
>> Ben, James, can either of you comment on what symbols mojo might need
exported
>> from the main android libraries?
>
>
> What exactly are the "main android libraries"?

All the various libraries built from the chromium code; including the mojo
libraries in fact. Sorry, I should have been more clear. This is a global
linker setting change which will affect every linkable binary built for
android, including shared libraries and executables, and cause them to only
export a strict whitelist of dynamic symbols.

> The comments in mojo/*gyp are about symbols that need to be exported from
shared
> libraries built out of the chrome tree.

Apparently they don't, at least as far as we have test coverage in the
chromium tree. :)
Under what cases are these symbols used?

> https://codereview.chromium.org/843103003/

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

[Torne, 2015-01-12 19:08:37]

On 12 Jan 2015 7:01 pm, <qsr@chromium.org> wrote:
>
> On 2015/01/12 18:58:53, qsr wrote:
>>
>> Does this break android gn builds? I do not see any related change in gn
>> configs.
>
>
> Hum, ok, no it will not break gn, but we will still need to use the same
old
> mechanism. Do you have any plan to port this to the GN build?

As per my first comment this CL isn't finished and I will do the GN change
in a future version. I want to get it to work correctly in gyp first so I
can just copy the end result over.

> https://codereview.chromium.org/843103003/

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

[qsr, 2015-01-12 19:09:16]

> Apparently they don't, at least as far as we have test coverage in the
> chromium tree. :)
> Under what cases are these symbols used?

 When building mojo application. html_viewer is one that will really need this.
When the network service is moved to chromium, this will be another. Those .so
defines a bunch of symbol that needs to be exported.

[Torne, 2015-01-12 19:12:57]

How do you plan to control which symbols are exported from those libraries?
The current mechanism doesn't really work at all and results in the wrong
symbols being included in basically every binary, since the linker flag
used is based on a completely arbitrary distinction between objects linked
directly into the .so and objects pulled in via some .a (which seems to be
a basically meaningless if not downright inconvenient distinction in gyp),
and then mojo turns the flag off anyway and causes hundreds of symbols to
become visible unnecessarily, creating many more GC roots and preventing
the linker from stripping dead code properly :(
On 12 Jan 2015 7:09 pm, <qsr@chromium.org> wrote:

>
>  Apparently they don't, at least as far as we have test coverage in the
>> chromium tree. :)
>> Under what cases are these symbols used?
>>
>
>  When building mojo application. html_viewer is one that will really need
> this.
> When the network service is moved to chromium, this will be another. Those
> .so
> defines a bunch of symbol that needs to be exported.
>
>
> https://codereview.chromium.org/843103003/
>

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

[jamesr, 2015-01-12 19:20:20]

On other platforms we rely on annotations to control visibility, i.e.
__attribute__((visibility("default"))).  What's different on android?

[Torne, 2015-01-12 19:30:18]

Hundreds of symbols in the build are marked visible when they shouldn't be,
and so without special linker treatment they end up visible in the final
binary. I suspect this is no different to any other platform, and the only
reason it is more noticeable on android is that almost all binaries on
android are shared libraries, whereas on the other platforms many things
are built as executables and so don't export any symbols anyway. The
visible symbols seem to mostly come from assembly code (where
-fvisibility=hidden has no effect) or bits of code that aren't built with
that flag for some reason. The android build has always depended on
--exclude-libs=ALL to avoid this, as far as I can see, so mojo unsetting
this flag has caused many symbols to reappear.
On 12 Jan 2015 7:20 pm, <jamesr@chromium.org> wrote:

> On other platforms we rely on annotations to control visibility, i.e.
> __attribute__((visibility("default"))).  What's different on android?
>
> https://codereview.chromium.org/843103003/
>

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

[Torne, 2015-01-12 19:32:09]

Also, the original reason for writing this CL is to allow final linked
binaries to decide whether they want to export JNI symbols or not without
having to compile the object files that go into those binaries twice, and
to do this we need to be able to *delete* the exported symbols generated by
the JNI binding generator to ensure that we aren't inadvertently relying on
them. I don't know how we can achieve this other than with this whitelist,
alas.
On 12 Jan 2015 7:20 pm, <jamesr@chromium.org> wrote:

> On other platforms we rely on annotations to control visibility, i.e.
> __attribute__((visibility("default"))).  What's different on android?
>
> https://codereview.chromium.org/843103003/
>

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

[jamesr, 2015-01-12 20:55:26]

I believe all the mojo-specific symbols we need to export will match the pattern
Mojo*, and it's doubtful anything else would produce symbols in this space, so
one option would be to just add that pattern to the list.  Another more flexible
solution would be to have a way to specify an export list per target, but that
would be harder.

Anyway the targets that need to export Mojo symbols are all built using GN not
gyp so it's a bit academic for this patch.

[Torne, 2015-01-13 11:19:01]

On 2015/01/12 20:55:26, jamesr wrote:
> I believe all the mojo-specific symbols we need to export will match the
pattern
> Mojo*, and it's doubtful anything else would produce symbols in this space, so
> one option would be to just add that pattern to the list.

Unless all the exported symbols are declared extern "C" then their names will be
mangled and so matching them will be trickier (depends what namespaces they
might be in, etc); I'll see what the current ones look like.

> Another more flexible
> solution would be to have a way to specify an export list per target, but that
> would be harder.

Yeah, I don't think the script is additive so we'd have to do something
complicated to build the script from gyp variables, seems gross.

> Anyway the targets that need to export Mojo symbols are all built using GN not
> gyp so it's a bit academic for this patch.

This patch is intended to address the GN build as well; I was intending to get
green builds on the gyp bots first, which I have, so if it weren't for mojo's
exported symbols I would have already updated the GN config and landed it. So,
alas, this is not academic :)

[Torne, 2015-01-13 14:19:50]

I'm going to investigate slightly different ways of achieving my aims here and
see if I can come up with something that serves my goal of dealing with JNI
exports without causing problems for mojo. Updates forthcoming :)

[Torne, 2015-01-13 15:53:08]

OK, I've filed http://crbug.com/448386 to cover the more general problem of
"android builds have too many exported symbols", which I intend to fix by
excluding specific libraries and fixing symbol visibilities in the sources, and
then probably add monitoring of this to a bot to prevent it regressing.

To address my specific JNI export issue I have a different approach, using a
version script in a blacklist fashion (to specifically remove JNI export
functions) instead of a whitelist fashion. This will avoid the need for any
special treatment of mojo, and mojo can continue to just set
visibility("default") on any symbols it wants to export without worrying about
any android-specific behaviour (and once the aforementioned bug is fixed, mojo
won't need to override --exclude-libs=ALL any more either as this won't be set
in the first place).

I assume this sounds better to everyone involved. Thanks for your comments :)

[jamesr, 2015-01-14 01:53:10]

lgtm

[Torne, 2015-01-29 14:16:45]

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

[Torne, 2015-01-29 14:22:14]

torne@chromium.org changed reviewers:
+ cjhopman@chromium.org
- jamesr@chromium.org, qsr@chromium.org

[Torne, 2015-01-29 14:22:14]

+cjhopman, rmcilroy

OK, I've uploaded a new patch that uses a blacklist approach to specifically
strip the JNI export symbols, instead of a whitelist, and retitled/described it
appropriately.

mojo people, this shouldn't have any impact on you any more since you don't use
symbols that match Java_* as far as I can see (the pattern that the JVM uses to
construct JNI native method references) - I've moved you to cc, feel free to
just opt out entirely as there may be more iterations of mail here ;)

I still haven't updated the GN files to match, which we should do before landing
this.

Chris (as the general owner of android build stuff) and Ross (as the owner of
the JNI generator), what do you think?

The goal here is to make sure that we never depend on the JVM looking up symbols
dynamically unless we are in a binary where that's explicitly okay - okay here
means "will never be loaded by the crazy linker". Thus, the default is to ban
the exports to avoid people from writing code which works when tested in, say,
content shell but not in the real Chrome with the crazy linker enabled.

Sorry for the complicated back and forth here. :)

Also, the separate patch https://codereview.chromium.org/806533003/ which gets
rid of other, non-JNI-related exported symbols that are currently exported by
mistake is still relevant and independent to this one..

[rmcilroy, 2015-02-03 13:43:56]

base/android lgtm when equivalent GN changes have been made too.

https://codereview.chromium.org/843103003/diff/60001/build/android/android_ex...
File build/android/android_exports.gyp (right):

https://codereview.chromium.org/843103003/diff/60001/build/android/android_ex...
build/android/android_exports.gyp:15: 'target_name': 'android_exports',
nit - could we rename this gyp file / target to android_webview_exports for
clarity?

https://codereview.chromium.org/843103003/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/843103003/diff/60001/build/common.gypi#newcod...
build/common.gypi:2473: # when using the crazy linker. To allow use of native
JNI exports (lazily
nit - maybe mention that binaries which set "use_chromium_linker=1" use the
crazy linker to make it obvious what gyp variables would affect this.

[cjhopman, 2015-02-03 21:04:28]

Yeah, this approach works for me.

[Torne, 2015-02-18 15:33:03]

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

[Torne, 2015-02-18 15:33:04]

+brettw

Brett, does this look okay for the GN configuration?

[brettw, 2015-02-18 21:27:26]

gn lgtm

[Torne, 2015-02-18 21:33:05]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2015-02-18 21:33:51]

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

[commit-bot: I haz the power, 2015-02-18 21:37:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-18 21:38:16]

Patchset 5 (id:??) landed as
https://crrev.com/9d90d85f73ca46847047ccbda270f25b637ccc86
Cr-Commit-Position: refs/heads/master@{#316896}