Android: Avoid linking with --gc-sections.

Android: Avoid linking with --gc-sections.

No more --gc-sections in debug mode for test targets either.

Re-enable instrumentation of globals in ASAN builds.

BUG=159801, 159847
Committed: https://crrev.com/6ef23a61fab8467ac15f12173d29ea94b5306101
Cr-Commit-Position: refs/heads/master@{#387491}

[Lei Zhang, 2016-04-13 23:00:23]

thestig@chromium.org changed reviewers:
+ agrieve@chromium.org, eugenis@chromium.org, yfriedman@chromium.org

[Lei Zhang, 2016-04-13 23:00:24]

Follow up to https://codereview.chromium.org/1884543004

[Evgeniy Stepanov, 2016-04-13 23:05:30]

On 2016/04/13 23:00:24, Lei Zhang wrote:
> Follow up to https://codereview.chromium.org/1884543004

Very cool!
Does the browser at least start without -asan-globals=0? I think an early
startup failure may break clusterfuzz testing.

[Evgeniy Stepanov, 2016-04-13 23:05:58]

Description was changed from

==========
Android: Avoid linking with --gc-sections.

No more --gc-sections in debug mode for test targets either.

Re-enable instrumentation of globals in ASAN builds.

BUG=159801,159847
==========

to

==========
Android: Avoid linking with --gc-sections.

No more --gc-sections in debug mode for test targets either.

Re-enable instrumentation of globals in ASAN builds.

BUG=159801,159847
==========

[Evgeniy Stepanov, 2016-04-13 23:05:58]

eugenis@google.com changed reviewers:
+ inferno@chromium.org

[Lei Zhang, 2016-04-13 23:29:06]

On 2016/04/13 23:05:30, Evgeniy Stepanov wrote:
> On 2016/04/13 23:00:24, Lei Zhang wrote:
> > Follow up to https://codereview.chromium.org/1884543004
> 
> Very cool!
> Does the browser at least start without -asan-globals=0? I think an early
> startup failure may break clusterfuzz testing.

Right now I have the following GN config. The incremental build starts and
errors out, but I'm not a real Android developer, so maybe my config is no good.
Do you have advise on what config I should test with?

target_os = "android"
target_cpu = "arm"  # (default)
is_debug = false
is_component_build = true
is_clang = true
symbol_level = 1  # Faster build with fewer symbols. -g1 rather than -g2
enable_incremental_javac = true  # Much faster; experimental
use_goma = true
is_asan = false

[Lei Zhang, 2016-04-13 23:29:40]

Hmm, still got red bots. Please hold off on reviews.

[Evgeniy Stepanov, 2016-04-13 23:37:08]

http://dev.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...

[Lei Zhang, 2016-04-14 01:26:23]

On 2016/04/13 23:37:08, Evgeniy Stepanov wrote:
>
http://dev.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...

Even without this CL, I tried building r387112 with:

target_os = "android"
target_cpu = "arm"  # (default)
is_debug = false
is_asan = false
is_clang = true
use_goma = true

And ChromePublic.apk works. But if I turn on ASAN, then it does not. Is this
expected?

[agrieve, 2016-04-14 15:17:11]

On 2016/04/14 01:26:23, Lei Zhang wrote:
> On 2016/04/13 23:37:08, Evgeniy Stepanov wrote:
> >
>
http://dev.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...
> 
> Even without this CL, I tried building r387112 with:
> 
> target_os = "android"
> target_cpu = "arm"  # (default)
> is_debug = false
> is_asan = false
> is_clang = true
> use_goma = true
> 
> And ChromePublic.apk works. But if I turn on ASAN, then it does not. Is this
> expected?

I think that's normal :S. lgtm!

[Yaron, 2016-04-14 15:19:46]

On 2016/04/14 15:17:11, agrieve wrote:
> On 2016/04/14 01:26:23, Lei Zhang wrote:
> > On 2016/04/13 23:37:08, Evgeniy Stepanov wrote:
> > >
> >
>
http://dev.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...
> > 
> > Even without this CL, I tried building r387112 with:
> > 
> > target_os = "android"
> > target_cpu = "arm"  # (default)
> > is_debug = false
> > is_asan = false
> > is_clang = true
> > use_goma = true
> > 
> > And ChromePublic.apk works. But if I turn on ASAN, then it does not. Is this
> > expected?
> 
> I think that's normal :S. lgtm!

lgtm but I can't speak to the asan bits

[Evgeniy Stepanov, 2016-04-14 17:52:44]

On 2016/04/14 15:19:46, Yaron wrote:
> On 2016/04/14 15:17:11, agrieve wrote:
> > On 2016/04/14 01:26:23, Lei Zhang wrote:
> > > On 2016/04/13 23:37:08, Evgeniy Stepanov wrote:
> > > >
> > >
> >
>
http://dev.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...
> > > 
> > > Even without this CL, I tried building r387112 with:
> > > 
> > > target_os = "android"
> > > target_cpu = "arm"  # (default)
> > > is_debug = false
> > > is_asan = false
> > > is_clang = true
> > > use_goma = true
> > > 
> > > And ChromePublic.apk works. But if I turn on ASAN, then it does not. Is
this
> > > expected?

Do you mean it fails to build or fails to run on device?
This is the first time I've heard about this target, to be honest. We build
tests (most of them) and the internal apk, and that is known to work. I'll test
this today.

[Evgeniy Stepanov, 2016-04-14 21:02:56]

On 2016/04/14 17:52:44, Evgeniy Stepanov wrote:
> On 2016/04/14 15:19:46, Yaron wrote:
> > On 2016/04/14 15:17:11, agrieve wrote:
> > > On 2016/04/14 01:26:23, Lei Zhang wrote:
> > > > On 2016/04/13 23:37:08, Evgeniy Stepanov wrote:
> > > > >
> > > >
> > >
> >
>
http://dev.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...
> > > > 
> > > > Even without this CL, I tried building r387112 with:
> > > > 
> > > > target_os = "android"
> > > > target_cpu = "arm"  # (default)
> > > > is_debug = false
> > > > is_asan = false
> > > > is_clang = true
> > > > use_goma = true
> > > > 
> > > > And ChromePublic.apk works. But if I turn on ASAN, then it does not. Is
> this
> > > > expected?
> 
> Do you mean it fails to build or fails to run on device?
> This is the first time I've heard about this target, to be honest. We build
> tests (most of them) and the internal apk, and that is known to work. I'll
test
> this today.

I've done some testing. Without this change, ChromePublic.apk builds and work
both with and without ASan, if you follow the instructions.

http://dev.chromium.org/developers/testing/addresssanitizer#TOC-Building-on-A...

With this change, libchrome_public.so link is still using --gc-sections.
Anyway, it builds and works with ASan.

[Lei Zhang, 2016-04-14 21:14:05]

On 2016/04/14 17:52:44, Evgeniy Stepanov wrote:
> Do you mean it fails to build or fails to run on device?
> This is the first time I've heard about this target, to be honest. We build
> tests (most of them) and the internal apk, and that is known to work. I'll
test
> this today.

It fails to run. Chromium starts, renders the chrome and a blank page, displays
an error dialog, and quits when I press ok. This is on a freshly imaged N4 with
Android L.

[Evgeniy Stepanov, 2016-04-14 21:15:15]

On 2016/04/14 21:14:05, Lei Zhang wrote:
> On 2016/04/14 17:52:44, Evgeniy Stepanov wrote:
> > Do you mean it fails to build or fails to run on device?
> > This is the first time I've heard about this target, to be honest. We build
> > tests (most of them) and the internal apk, and that is known to work. I'll
> test
> > this today.
> 
> It fails to run. Chromium starts, renders the chrome and a blank page,
displays
> an error dialog, and quits when I press ok. This is on a freshly imaged N4
with
> Android L.

It works for me on N6.
Did you run asan_device_setup?

[Lei Zhang, 2016-04-14 21:53:47]

On 2016/04/14 21:15:15, Evgeniy Stepanov wrote:
> On 2016/04/14 21:14:05, Lei Zhang wrote:
> > On 2016/04/14 17:52:44, Evgeniy Stepanov wrote:
> > > Do you mean it fails to build or fails to run on device?
> > > This is the first time I've heard about this target, to be honest. We
build
> > > tests (most of them) and the internal apk, and that is known to work. I'll
> > test
> > > this today.
> > 
> > It fails to run. Chromium starts, renders the chrome and a blank page,
> displays
> > an error dialog, and quits when I press ok. This is on a freshly imaged N4
> with
> > Android L.
> 
> It works for me on N6.
> Did you run asan_device_setup?

No, and when I tried, I got:
failed to copy '/tmp/foo/new/libclang_rt.asan-arm-android.so' to
'/system/lib/libclang_rt.asan-arm-android.so': Read-only file system

And indeed the linking still happens with --gc-sections, because it's a release
build, and the rest of the CL only affects debug builds. Let me try taking that
out for ASAN builds.

[Lei Zhang, 2016-04-14 22:21:34]

On 2016/04/14 21:53:47, Lei Zhang wrote:
> And indeed the linking still happens with --gc-sections, because it's a
release
> build, and the rest of the CL only affects debug builds. Let me try taking
that
> out for ASAN builds.

eugenis: I removed --gc-sections for ASAN builds. Do you want to try that?

FWIW, the error I'm seeing is: "Chrome failed during startup with an unexpected
error" [ok]

[Evgeniy Stepanov, 2016-04-14 22:37:53]

On 2016/04/14 22:21:34, Lei Zhang wrote:
> On 2016/04/14 21:53:47, Lei Zhang wrote:
> > And indeed the linking still happens with --gc-sections, because it's a
> release
> > build, and the rest of the CL only affects debug builds. Let me try taking
> that
> > out for ASAN builds.
> 
> eugenis: I removed --gc-sections for ASAN builds. Do you want to try that?

Not really - if instrumentation of globals works with --gc-sections, it should
work without that flag, too. LGTM.
 
> FWIW, the error I'm seeing is: "Chrome failed during startup with an
unexpected
> error" [ok]

"Read-only file system" means that "adb root; adb remount" failed for some
reason. Is it a -user build? ASan needs at least -userdebug.

[Lei Zhang, 2016-04-14 22:47:40]

On 2016/04/14 22:37:53, Evgeniy Stepanov wrote:
> On 2016/04/14 22:21:34, Lei Zhang wrote:
> > On 2016/04/14 21:53:47, Lei Zhang wrote:
> > > And indeed the linking still happens with --gc-sections, because it's a
> > release
> > > build, and the rest of the CL only affects debug builds. Let me try taking
> > that
> > > out for ASAN builds.
> > 
> > eugenis: I removed --gc-sections for ASAN builds. Do you want to try that?
> 
> Not really - if instrumentation of globals works with --gc-sections, it should
> work without that flag, too. LGTM.

Sounds like I should go back to patch set 3 then?

> > FWIW, the error I'm seeing is: "Chrome failed during startup with an
> unexpected
> > error" [ok]
> 
> "Read-only file system" means that "adb root; adb remount" failed for some
> reason. Is it a -user build? ASan needs at least -userdebug.

Ya, reflashing...

[Evgeniy Stepanov, 2016-04-14 22:53:48]

On 2016/04/14 22:47:40, Lei Zhang wrote:
> On 2016/04/14 22:37:53, Evgeniy Stepanov wrote:
> > On 2016/04/14 22:21:34, Lei Zhang wrote:
> > > On 2016/04/14 21:53:47, Lei Zhang wrote:
> > > > And indeed the linking still happens with --gc-sections, because it's a
> > > release
> > > > build, and the rest of the CL only affects debug builds. Let me try
taking
> > > that
> > > > out for ASAN builds.
> > > 
> > > eugenis: I removed --gc-sections for ASAN builds. Do you want to try that?
> > 
> > Not really - if instrumentation of globals works with --gc-sections, it
should
> > work without that flag, too. LGTM.
> 
> Sounds like I should go back to patch set 3 then?

Yes, I guess it's better to keep --gc-sections in release builds. 

> > > FWIW, the error I'm seeing is: "Chrome failed during startup with an
> > unexpected
> > > error" [ok]
> > 
> > "Read-only file system" means that "adb root; adb remount" failed for some
> > reason. Is it a -user build? ASan needs at least -userdebug.
> 
> Ya, reflashing...

[Lei Zhang, 2016-04-14 23:20:18]

On 2016/04/14 22:47:40, Lei Zhang wrote:
> On 2016/04/14 22:37:53, Evgeniy Stepanov wrote:
> > "Read-only file system" means that "adb root; adb remount" failed for some
> > reason. Is it a -user build? ASan needs at least -userdebug.
> 
> Ya, reflashing...

And now it works with patch set 4. I will verify it works with patch set 3 and
if so, land that as patch set 5.

[Lei Zhang, 2016-04-14 23:26:33]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-04-14 23:26:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, agrieve@chromium.org, eugenis@google.com
Link to the patchset: https://codereview.chromium.org/1883723004/#ps80001
(title: "back to patch set 3")

[commit-bot: I haz the power, 2016-04-14 23:27:03]

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

[commit-bot: I haz the power, 2016-04-15 00:41:27]

Description was changed from

==========
Android: Avoid linking with --gc-sections.

No more --gc-sections in debug mode for test targets either.

Re-enable instrumentation of globals in ASAN builds.

BUG=159801,159847
==========

to

==========
Android: Avoid linking with --gc-sections.

No more --gc-sections in debug mode for test targets either.

Re-enable instrumentation of globals in ASAN builds.

BUG=159801,159847
==========

[commit-bot: I haz the power, 2016-04-15 00:41:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-15 00:42:54]

Description was changed from

==========
Android: Avoid linking with --gc-sections.

No more --gc-sections in debug mode for test targets either.

Re-enable instrumentation of globals in ASAN builds.

BUG=159801,159847
==========

to

==========
Android: Avoid linking with --gc-sections.

No more --gc-sections in debug mode for test targets either.

Re-enable instrumentation of globals in ASAN builds.

BUG=159801,159847

Committed: https://crrev.com/6ef23a61fab8467ac15f12173d29ea94b5306101
Cr-Commit-Position: refs/heads/master@{#387491}
==========

[commit-bot: I haz the power, 2016-04-15 00:42:56]

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

[Lei Zhang, 2016-04-15 06:38:01]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1889073002/ by thestig@chromium.org.

The reason for reverting is: May have caused the Android Debug (Nexus 9) GPU bot
to go red.

FAILED: python ../../build/android/gyp/finalize_apk.py ...

Exception in thread "main" java.lang.UnsupportedOperationException: Found more
than one library
Multiple libraries are not supported for APKs that use 'load_library_from_zip'..