android: Use gold in arm64 builds too.

android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=481855
Committed: https://crrev.com/a4baf802f3058b66976a2085278f13ab3bcd8092
Cr-Commit-Position: refs/heads/master@{#424747}

[Nico, 2016-10-11 15:49:55]

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

[commit-bot: I haz the power, 2016-10-11 15:50:07]

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

[Nico, 2016-10-11 15:50:18]

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

[Nico, 2016-10-11 15:50:18]

Since I lack arm64 hardware, I haven't actually tried running the resulting
binary.

[commit-bot: I haz the power, 2016-10-11 15:59:33]

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

[commit-bot: I haz the power, 2016-10-11 15:59:34]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Torne, 2016-10-11 16:03:50]

I can't recall why we weren't using gold for arm64 in the first place. Do you
know?

[Nico, 2016-10-11 16:10:41]

If you means me, then no. I'd guess we just forgot to add arm64 to (the gyp
equivalent of) this list during bring up.

On Oct 11, 2016 12:03 PM, <torne@chromium.org> wrote:

> I can't recall why we weren't using gold for arm64 in the first place. Do
> you
> know?
>
> https://codereview.chromium.org/2410233002/
>

-- 
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.

[Torne, 2016-10-11 16:28:39]

I am fairly sure it was intentionally disabled a long time ago, but I can't
remember why or find any reference. :/

[Primiano Tucci (use gerrit), 2016-10-11 16:29:46]

On 2016/10/11 16:10:41, Nico wrote:
> If you means me, then no. I'd guess we just forgot to add arm64 to (the gyp
> equivalent of) this list during bring up.
> 
> On Oct 11, 2016 12:03 PM, <mailto:torne@chromium.org> wrote:
> 
> > I can't recall why we weren't using gold for arm64 in the first place. Do
> > you
> > know?

When we ported chrome for android to arm64 I remember clearly there was no
support for gold on arm 64 (I had to manually surge all the broken deps that
were relying on linker gc sections).
Then at some point it was enabled, and then re-disabled by
https://codereview.chromium.org/1113473004
according to Issue 481855 this seems due to ARM Cortex-A53 Erratum #843419 and
#835769. 
But then crbug.com/522414 seems to suggest that these were fixed, and honestly I
don't fully understand if the resolution was just in the compiler (i'm rushing a
bit, but those two bugs seem relevant).

[Nico, 2016-10-11 16:35:02]

Ooooh I vaguely remember that, thanks! I'll check if the compiler passes
the right flags to the linker and whatnot.

On Tue, Oct 11, 2016 at 12:29 PM, <primiano@chromium.org> wrote:

> On 2016/10/11 16:10:41, Nico wrote:
> > If you means me, then no. I'd guess we just forgot to add arm64 to (the
> gyp
> > equivalent of) this list during bring up.
> >
> > On Oct 11, 2016 12:03 PM, <mailto:torne@chromium.org> wrote:
> >
> > > I can't recall why we weren't using gold for arm64 in the first place.
> Do
> > > you
> > > know?
>
> When we ported chrome for android to arm64 I remember clearly there was no
> support for gold on arm 64 (I had to manually surge all the broken deps
> that
> were relying on linker gc sections).
> Then at some point it was enabled, and then re-disabled by
> https://codereview.chromium.org/1113473004
> according to Issue 481855 this seems due to ARM Cortex-A53 Erratum #843419
> and
> #835769.
> But then crbug.com/522414 seems to suggest that these were fixed, and
> honestly I
> don't fully understand if the resolution was just in the compiler (i'm
> rushing a
> bit, but those two bugs seem relevant).
>
>
>
> https://codereview.chromium.org/2410233002/
>

-- 
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.

[Nico, 2016-10-11 18:45:07]

I passed -### to the libchrome.so link command for {gcc, clang} x {bfd ld,
gold}.

gcc passes --fix-cortex-a53-835769 --fix-cortex-a53-843419 to both bfd ld and
gold (via collect2). gold doesn't complain about unknown flags, and it does
complain if I pass --fix-cortex-asdfasdf. Also

 -fuse-ld=gold --version
collect2 version 4.9.x 20150123 (prerelease)
/usr/local/google/home/thakis/src/chrome/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold
--version
GNU gold (binutils-2.25-0666073 2.25.51.20141117) 1.11
Copyright (C) 2015 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) a later version.
This program has absolutely no warranty.

That suggests the gold should be slightly to old to have the fix from
https://sourceware.org/bugzilla/show_bug.cgi?id=18348, but

$ strings
/usr/local/google/home/thakis/src/chrome/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold
| grep cortex
x_cortex_a8
no-fix_cortex_a8
fix_cortex_a53_843419
no-fix_cortex_a53_843419
fix_cortex_a53_835769
no-fix_cortex_a53_835769
N4gold15General_options20Struct_fix_cortex_a8E
N4gold15General_options23Struct_no_fix_cortex_a8E
N4gold15General_options28Struct_fix_cortex_a53_843419E
N4gold15General_options31Struct_no_fix_cortex_a53_843419E
N4gold15General_options28Struct_fix_cortex_a53_835769E
N4gold15General_options31Struct_no_fix_cortex_a53_835769E
apply_cortex_a8_workaround
apply_cortex_a8_workaround_to_address_range
scan_span_for_cortex_a8_erratum
apply_cortex_a8_workaround
apply_cortex_a8_workaround_to_address_range
scan_span_for_cortex_a8_erratum

suggests the android folks backported it.

So I think this part should be good now (but wasn't in older NDKs).

clang doesn't pass these flags to either ld. It does have a built-in pass to
work around 835769 (here:
http://llvm-cs.pcc.me.uk/lib/Target/AArch64/AArch64TargetMachine.cpp#99), so the
compiler won't create these instrs, so the linker doesn't have to clean them up
(which assumes that no assembly contains bad instrs). I don't know about 843419,
but that's a clang thing, not a gold thing, and independent of this patch.

[Nico, 2016-10-11 18:56:41]

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

[commit-bot: I haz the power, 2016-10-11 18:57:11]

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

[Nico, 2016-10-11 19:06:26]

On 2016/10/11 18:45:07, Nico wrote:
> I passed -### to the libchrome.so link command for {gcc, clang} x {bfd ld,
> gold}.
> 
> gcc passes --fix-cortex-a53-835769 --fix-cortex-a53-843419 to both bfd ld and
> gold (via collect2). gold doesn't complain about unknown flags, and it does
> complain if I pass --fix-cortex-asdfasdf. Also
> 
>  -fuse-ld=gold --version
> collect2 version 4.9.x 20150123 (prerelease)
>
/usr/local/google/home/thakis/src/chrome/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold
> --version
> GNU gold (binutils-2.25-0666073 2.25.51.20141117) 1.11
> Copyright (C) 2015 Free Software Foundation, Inc.
> This program is free software; you may redistribute it under the terms of
> the GNU General Public License version 3 or (at your option) a later version.
> This program has absolutely no warranty.
> 
> That suggests the gold should be slightly to old to have the fix from
> https://sourceware.org/bugzilla/show_bug.cgi?id=18348, but
> 
> $ strings
>
/usr/local/google/home/thakis/src/chrome/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold
> | grep cortex
> x_cortex_a8
> no-fix_cortex_a8
> fix_cortex_a53_843419
> no-fix_cortex_a53_843419
> fix_cortex_a53_835769
> no-fix_cortex_a53_835769
> N4gold15General_options20Struct_fix_cortex_a8E
> N4gold15General_options23Struct_no_fix_cortex_a8E
> N4gold15General_options28Struct_fix_cortex_a53_843419E
> N4gold15General_options31Struct_no_fix_cortex_a53_843419E
> N4gold15General_options28Struct_fix_cortex_a53_835769E
> N4gold15General_options31Struct_no_fix_cortex_a53_835769E
> apply_cortex_a8_workaround
> apply_cortex_a8_workaround_to_address_range
> scan_span_for_cortex_a8_erratum
> apply_cortex_a8_workaround
> apply_cortex_a8_workaround_to_address_range
> scan_span_for_cortex_a8_erratum
> 
> suggests the android folks backported it.
> 
> So I think this part should be good now (but wasn't in older NDKs).
> 
> clang doesn't pass these flags to either ld. It does have a built-in pass to
> work around 835769 (here:
> http://llvm-cs.pcc.me.uk/lib/Target/AArch64/AArch64TargetMachine.cpp#99), so
the
> compiler won't create these instrs, so the linker doesn't have to clean them
up
> (which assumes that no assembly contains bad instrs). I don't know about
843419,
> but that's a clang thing, not a gold thing, and independent of this patch.

Slightly less barbaric:

usr/local/google/home/thakis/src/chrome/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold
--help | grep cortex
  --fix-cortex-a8             (ARM only) Fix binaries for Cortex-A8 erratum.
  --no-fix-cortex-a8          (ARM only) Do not fix binaries for Cortex-A8
erratum.
  --fix-cortex-a53-843419     (AArch64 only) Fix Cortex-A53 erratum 843419.
  --no-fix-cortex-a53-843419  (AArch64 only) Do not fix Cortex-A53 erratum
843419.
  --fix-cortex-a53-835769     (AArch64 only) Fix Cortex-A53 erratum 835769.
  --no-fix-cortex-a53-835769  (AArch64 only) Do not fix Cortex-A53 erratum
835769.

So these are there.

[Nico, 2016-10-11 19:14:21]

Description was changed from

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

BUG=none
==========

to

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=none
==========

[Primiano Tucci (use gerrit), 2016-10-11 19:29:16]

Thanks a lot for checking.
So my understanding from your comments is that the toolchain support is there. I
just didn't get if after this CL + use_clang,  clang will also pass the right
-errata args to gold or not.
If that is the case, I morally support (LGTM) this. plz re-link to BUG=481855

+pasko in case he is aware of any other unlisted reason to not switch back to
gold

[Nico, 2016-10-11 20:13:46]

On 2016/10/11 19:29:16, Primiano Tucci wrote:
> Thanks a lot for checking.
> So my understanding from your comments is that the toolchain support is there.
I
> just didn't get if after this CL + use_clang,  clang will also pass the right
> -errata args to gold or not.
> If that is the case, I morally support (LGTM) this. plz re-link to BUG=481855
> 
> +pasko in case he is aware of any other unlisted reason to not switch back to
> gold

This CL doesn't change the clang situation. Both before and after this change,
clang does not pass "linker, please work around these defect" flags to the
linker. clang _does_ make sure to not produce code with these defects in the
first place for at least one of these two defects (not sure about the second
yet), so it arguably doesn't have to pass these flags.

Added bug link.

[Nico, 2016-10-11 20:13:54]

Description was changed from

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=none
==========

to

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=481855
==========

[commit-bot: I haz the power, 2016-10-11 20:17:44]

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

[commit-bot: I haz the power, 2016-10-11 20:17:45]

Dry run: This issue passed the CQ dry run.

[Torne, 2016-10-12 12:30:02]

From back when I used to work with ARM errata and the ARM commercial toolchain,
the typical reason for the linker including these workarounds as well as the
compiler is to enable people to use closed-source static libraries that may have
been compiled with a compiler that doesn't know about the errata, which isn't a
problem that we should have. 

Assembler code might contain these sequences, though, and I haven't looked at
the errata in detail to think about how likely this is in this case.

How hard would it be to get our clang to pass the flags?

[Nico, 2016-10-12 14:37:46]

On 2016/10/12 12:30:02, Torne wrote:
> From back when I used to work with ARM errata and the ARM commercial
toolchain,
> the typical reason for the linker including these workarounds as well as the
> compiler is to enable people to use closed-source static libraries that may
have
> been compiled with a compiler that doesn't know about the errata, which isn't
a
> problem that we should have. 
> 
> Assembler code might contain these sequences, though, and I haven't looked at
> the errata in detail to think about how likely this is in this case.
> 
> How hard would it be to get our clang to pass the flags?

I asked ccoutant why these fixes are opt-in on the ld side. He wrote:

"""Errata fixes are not enabled by default (and I believe the same is
true for Gnu ld) for several reasons:

(1) They generally involve making at least one additional pass over
all the code, which slows the link down considerably.

(2) There are quite a few errata, and they are usually specific to a
particular processor or micro-architecture. We would never want to
apply them all, so we need to know which processor we're actually
targeting. (Think of the --fix* options as flags that tells the linker
what processor to "optimize" for.)

(3) As you mentioned, the compiler sometimes knows how to avoid
certain errata, and in those cases, having the linker look for them
would be wasteful. Thus, it works best to have the compiler pass the
flags for the specific errata that might still need to be fixed by the
linker.

That said, if the ARM maintainers felt it necessary to enable an
erratum fix by default, I'd probably go with their judgement.
"""

I find this fairly convincing.

If a vendor gives us a library with broken code, that's a bug in their library,
right? By a similar argument, we could also do static analysis on disassembly in
binary blobs to find other kinds of bugs, but it feels kind of out-of-scope.

Do we depend on any binary-only libraries from vendors? "Just in case" doesn't
justify slowing down our links perpetually, imho.

(In any case, this discussion is independent of this CL, which switches from ld
to gold, and gcc passes the explicit flag to both. It is relevant to switching
from gcc to clang on arm64 though.)

[Torne, 2016-10-12 15:30:25]

On 2016/10/12 14:37:46, Nico (mostly afk until Oct 23) wrote:
> I find this fairly convincing.

Sure, I think the argument about the default is pretty reasonable.

> If a vendor gives us a library with broken code, that's a bug in their
library,
> right? By a similar argument, we could also do static analysis on disassembly
in
> binary blobs to find other kinds of bugs, but it feels kind of out-of-scope.

Right, but lots of people who buy ARM's toolchain from ARM *do* use binary-only
libraries from vendors with broken code in them, so ARM's commercial linker
tends to fix up all the errata anyway to make this keep working, because the
world is terrible ;)

> Do we depend on any binary-only libraries from vendors? "Just in case" doesn't
> justify slowing down our links perpetually, imho.

We don't, so we probably don't have this issue, but at the time these errata
flags were first added to chromium several people seemed to be really concerned
about making sure that we were definitely applying the errata fixes and wanted
it to be tested to prevent regressions and other stuff; I don't think that ever
happened in the end. I tried to dig this up and couldn't find it (maybe it was
in a code review discussion and not a bug?), but it's possible that someone else
has a stronger opinion about us being really careful here :)

> (In any case, this discussion is independent of this CL, which switches from
ld
> to gold, and gcc passes the explicit flag to both. It is relevant to switching
> from gcc to clang on arm64 though.)

Yeah. This CL LGTM; sorry for being slightly offtopic here.

[Torne, 2016-10-12 15:34:44]

Oh, haha. Actually I must just have been searching very badly in crbug; the bug
I'm thinking of is crbug.com/522414 which is assigned to... me. Oh.

Anyway, it seems like pasko@ might have an opinion about what to do about the
errata when we come to look at switching to clang.

[Nico, 2016-10-12 15:38:12]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2016-10-12 15:38:25]

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

[commit-bot: I haz the power, 2016-10-12 15:44:35]

Description was changed from

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=481855
==========

to

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=481855
==========

[commit-bot: I haz the power, 2016-10-12 15:44:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-12 15:49:05]

Description was changed from

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=481855
==========

to

==========
android: Use gold in arm64 builds too.

Reduces hot-cache link time from 49s to 23s (gcc) and binary size from 72M to
67M (gcc)  -- faster links and a 5MB smaller binary.

(With clang, it's 36s to 19.5s and 75M to 69M.)

It also makes the chrome/android/arm64 config more similar to most other build
configs.

We used to not use this because of
https://sourceware.org/bugzilla/show_bug.cgi?id=18348 , but the gold binary
in NDK r12b (which we currently use) has the flags added in the patches on
that bug, so it should be fine now.

BUG=481855

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

[commit-bot: I haz the power, 2016-10-12 15:49:07]

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

[Nico, 2016-10-26 15:18:24]

To follow-up on the somewhat off-topic sidethread on this review: clang now
passes the 843419 flag as of today, as a result of the discussion started in
this bug (https://reviews.llvm.org/rL285127). It was missing that part.  For the
other erratum it already tries to not emit broken code as discussed above (which
may or may not be sufficient)