1360321 - Add AArch64 support in various modules

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 1. Update linux_syscall_support.h in Breakpad (v1)

The last update left out a couple lines that should have been taken out,
which only affected AArch64 builds.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2. Fix cast warning on AArch64 in plugin code (v1)

Fix a warning on AArch64 for casting to pointer.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 3. Don't use mmap2 on Android AArch64 in mozjemalloc (v1)

Android NDK defines SYS_mmap2 but it expands to a nonexistent symbol.
mmap2 may not be supported in any case in some AArch64 kernels, so we
should just use mmap.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4. Fix mozglue build on AArch64 Android (v1)

* Fix printf macro mismatches where, for example, `PRIxPTR` is defined
  for `long` but the ELF `Addr` type is defined as `long long`.

* Add relocation macros for AArch64.

* Fill in AArch64 implementations for code that's platform-dependent.
  Even though we're not using on-demand decompression anymore, I added the
  AArch64 cases for completeness.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 5. Fix warnings in widget (v1)

* Fix warnings using putenv with string literals.

* Fix a pritnf format warning.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 6. Add AArch64 support in xpcom (v1)

Add breakpoint support for AArch64, and fix a scanf format specifier
warning. Also fix an #if line in xptcinvoke_arm.cpp to work as intended.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 7. Add AArch64 check to Fennec hardware checks (v1)

Add checks for "arm64-v8a" when we check for supported platforms.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 8. Don't set PR_ALTERNATE_INT64_TYPEDEF for Android in NSPR [Reminder: Separate landing into NSPR tree necessary, don't land into mozilla-central/inbound/autoland]

This uses the approach suggested in bug 1139241 comment 4. Don't set
PR_ALTERNATE_INT64_TYPEDEF for Android because Android headers define
`[u]int64_t` as `[unsigned] long` when `long` is 8 bytes.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 9. Fix AArch64 build in jemalloc (v1)

This is a backport of commit ae248a2 on the jemalloc dev branch to the
4.5 branch. It adds support for the openat syscall on platforms like
AArch64 where the open syscall may not be available.

This fix will be replaced when we update to the next jemalloc release.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8862582 [details] [diff] [review]
4. Fix mozglue build on AArch64 Android (v1)

Review of attachment 8862582 [details] [diff] [review]:
-----------------------------------------------------------------

r=me since glandium is on PTO.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8862582 [details] [diff] [review]
4. Fix mozglue build on AArch64 Android (v1)

Review of attachment 8862582 [details] [diff] [review]:
-----------------------------------------------------------------

This would benefit from having each point in the commit message treated in separate patches.

::: mozglue/linker/ElfLoader.cpp
@@ +1049,5 @@
>    return true;
> +#elif defined(__aarch64__)
> +  const uint32_t trampoline[] = {
> +    0x58000050, // ldr x16, [pc, #8]
> +    0xd61f0200, // br x16

Please add a comment that x16 is a scratch register (I'm ignorant about GAS support for aarch64, but why not r16?). (per Procedure Call Standard for the 
ARM 64-bit Architecture: "The first intra-procedure-call scratch register (can be used by call veneers and PLT code); at other times may be used as a temporary register)")

@@ +1056,5 @@
> +  };
> +  const size_t len = sizeof(trampoline) + sizeof(void*);
> +  EnsureWritable w(ptr, len);
> +  memcpy(ptr, trampoline, sizeof(trampoline));
> +  *reinterpret_cast<void**>(addr + sizeof(trampoline)) = FunctionPtr(new_func);

You need a cacheflush. In fact, you'd probably be better off sharing the code with the arm implementation, just setting the different trampoline on aarch64 and then setting start to trampoline.

::: mozglue/linker/XZStream.cpp
@@ +17,5 @@
>  {
>    if (!aBufSize) {
>      return 0;
>    }
> +  aBufSize = std::min(size_t(9u), aBufSize);

size_t(9)

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8862590 [details] [diff] [review]
9. Fix AArch64 build in jemalloc (v1)

Review of attachment 8862590 [details] [diff] [review]:
-----------------------------------------------------------------

Running memory/jemalloc/update.sh should produce the same code as is current in the tree. So you can't patch jemalloc code without making that work somehow. In the past, we've been using patch files for that (and we do that for many other third party code bases), and more recently, I've been using a forked repository with the right patches and changing the url and version in upstream.info (see bug 1201738).

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 9. Fix AArch64 build in jemalloc (v2)

This patch changes upstream.info to a revision in my forked repo. Feel free to
push that rev to your jemalloc repo if you want to use your repo in
upstream.info. Looks like the new rev also picked up a configure change from
upstream, but that looks like a pretty safe change.

For the next jemalloc release, we can go back to the upstream repo, since this
fix is already merged to the upstream dev branch.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4. Fix mozglue build on AArch64 Android (v2)

(In reply to Mike Hommey [:glandium] from comment #11)
> Comment on attachment 8862582 [details] [diff] [review]
> 4. Fix mozglue build on AArch64 Android (v1)
> 
> I'm ignorant about GAS support for aarch64, but why not r16?

I think when used as a 64-bit register it's called `x16`.

> @@ +1056,5 @@
> You need a cacheflush. In fact, you'd probably be better off sharing the
> code with the arm implementation, just setting the different trampoline on
> aarch64 and then setting start to trampoline.

This patch merges the ARM and AArch64 code.

Looks like there's no `cacheflush` for AArch64, but there is
`__builtin___clear_cache`, which is supposed to replace `cacheflush`. I tested
that `__builtin___clear_cache` works with both GCC and Clang, for both ARM and
AArch64.

Comment 15

[Wan-Teh Chang]

Comment on attachment 8862589 [details] [diff] [review]
8. Don't set PR_ALTERNATE_INT64_TYPEDEF for Android in NSPR [Reminder: Separate landing into NSPR tree necessary, don't land into mozilla-central/inbound/autoland]

Review of attachment 8862589 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Kai, could you review this and check this in? Thanks.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8865002 [details] [diff] [review]
9. Fix AArch64 build in jemalloc (v2)

Review of attachment 8865002 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/jemalloc/src/include/jemalloc/internal/jemalloc_internal_decls.h
@@ +15,5 @@
>  #      define SYS_write __NR_write
>  #    endif
> +#    if defined(SYS_open) && defined(__aarch64__)
> +       /* Android headers may define SYS_open to __NR_open even though
> +        * __NR_open may not exist on AArch64 (superseded by __NR_openat). */

Not that it matters much, but you wouldn't need this if you just made SYS_openat the default and SYS_open the fallback (I don't really see a reason not to use SYS_openat everywhere it's available)

::: memory/jemalloc/upstream.info
@@ +1,1 @@
> +UPSTREAM_REPO=https://github.com/darchons/jemalloc

Can you maybe get upstream to apply this to the stable-4 branch (which would be good to update to the tip of, since it has a fix for issue #766 which sounds like it could affect us (we are using custom hooks))

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8865587 [details] [diff] [review]
4. Fix mozglue build on AArch64 Android (v2)

Review of attachment 8865587 [details] [diff] [review]:
-----------------------------------------------------------------

The first sentence in comment 11 still applies.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8865002 [details] [diff] [review]
9. Fix AArch64 build in jemalloc (v2)

(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8865002 [details] [diff] [review]
> 9. Fix AArch64 build in jemalloc (v2)
> 
> Review of attachment 8865002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/jemalloc/src/include/jemalloc/internal/jemalloc_internal_decls.h
> @@ +15,5 @@
> >  #      define SYS_write __NR_write
> >  #    endif
> > +#    if defined(SYS_open) && defined(__aarch64__)
> > +       /* Android headers may define SYS_open to __NR_open even though
> > +        * __NR_open may not exist on AArch64 (superseded by __NR_openat). */
> 
> Not that it matters much, but you wouldn't need this if you just made
> SYS_openat the default and SYS_open the fallback (I don't really see a
> reason not to use SYS_openat everywhere it's available)

I wasn't sure if we need to support running on kernels without openat support. Besides, this change was already accepted in upstream trunk so I think it'd be a good idea to keep it here.

> ::: memory/jemalloc/upstream.info
> @@ +1,1 @@
> > +UPSTREAM_REPO=https://github.com/darchons/jemalloc
> 
> Can you maybe get upstream to apply this to the stable-4 branch (which would
> be good to update to the tip of, since it has a fix for issue #766 which
> sounds like it could affect us (we are using custom hooks))

In another bug? I don't think issue #766 is directly related to supporting AArch64.

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4a. Fix printf macro mismatches in mozglue (v3)

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4b. Add relocation macros for AArch64 (v3)

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4c. Add Divert case for AArch64 (v3)

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4d. Define mmap ordering for AArch64 (v3)

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4e. Fix std::min type error (v3)

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 8862589 [details] [diff] [review]
8. Don't set PR_ALTERNATE_INT64_TYPEDEF for Android in NSPR [Reminder: Separate landing into NSPR tree necessary, don't land into mozilla-central/inbound/autoland]

I've filed bug 1363329 to track the landing of this patch into the NSPR tree (already done).
Wan-Teh's review is sufficient.
Uplifting this change into mozilla-central as part of a new NSPR beta/release snapshot is tracked in bug 1350291.

Comment 25

[Mike Hommey [:glandium]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #18)
> Comment on attachment 8865002 [details] [diff] [review]
> 9. Fix AArch64 build in jemalloc (v2)
> 
> (In reply to Mike Hommey [:glandium] from comment #16)
> > Comment on attachment 8865002 [details] [diff] [review]
> > 9. Fix AArch64 build in jemalloc (v2)
> > 
> > Review of attachment 8865002 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: memory/jemalloc/src/include/jemalloc/internal/jemalloc_internal_decls.h
> > @@ +15,5 @@
> > >  #      define SYS_write __NR_write
> > >  #    endif
> > > +#    if defined(SYS_open) && defined(__aarch64__)
> > > +       /* Android headers may define SYS_open to __NR_open even though
> > > +        * __NR_open may not exist on AArch64 (superseded by __NR_openat). */
> > 
> > Not that it matters much, but you wouldn't need this if you just made
> > SYS_openat the default and SYS_open the fallback (I don't really see a
> > reason not to use SYS_openat everywhere it's available)
> 
> I wasn't sure if we need to support running on kernels without openat
> support. Besides, this change was already accepted in upstream trunk so I
> think it'd be a good idea to keep it here.

That was just a suggestion about what could be further changed upstream. openat was added in Linux 2.6.16, which is 11 years old.
 
> > ::: memory/jemalloc/upstream.info
> > @@ +1,1 @@
> > > +UPSTREAM_REPO=https://github.com/darchons/jemalloc
> > 
> > Can you maybe get upstream to apply this to the stable-4 branch (which would
> > be good to update to the tip of, since it has a fix for issue #766 which
> > sounds like it could affect us (we are using custom hooks))
> 
> In another bug? I don't think issue #766 is directly related to supporting
> AArch64.

It's not, but I'd rather do a "bulk" update of jemalloc if possible. Although, at this point, we might as well decide to remove it from the tree entirely...

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 8862578 [details] [diff] [review]
1. Update linux_syscall_support.h in Breakpad (v1)

Review of attachment 8862578 [details] [diff] [review]:
-----------------------------------------------------------------

This file comes from this upstream repo: https://chromium.googlesource.com/linux-syscall-support/+/master

If you don't patch upstream we'll just overwrite your patch the next time we update Breakpad. If you get your change landed in LSS you also need to update the revision of it that Breakpad pulls via gclient here: https://chromium.googlesource.com/breakpad/breakpad/+/master/DEPS#56

I don't really know enough about AArch64 to otherwise comment on this patch.

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 8862578 [details] [diff] [review]
1. Update linux_syscall_support.h in Breakpad (v1)

Jim pointed out that this is merely syncing up with upstream, some changes were left out in the last sync in bug 1264367.

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 9. Fix AArch64 build in jemalloc (v3)

Update jemalloc to upstream stable-4 branch to pick up the commit.

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 10. Fix opt build warnings in VIXL (v1)

Fix an unused variable warning for `visitor` because it's only used in
the assertion macro.

Fix several no-return-value errors because the compiler cannot assume
the VIXL_UNREACHABLE() macro is actually unreachable.

r=me for trivial patch.

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 11. Include AArch64 in nsWrapperCache 64-bit check (v1)

Include AArch64 in the 64-bit architecture check in nsWrapperCache.h.
Otherwise AArch64 builds fail with a build error. r=me for trivial
patch.

Comment 31

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 12a. Add missing crc32 dependency for Skia on AArch64 (v1)

Add an arm64 target cpu to generate_mozbuild.py so we can include the
"arm64" and "crc32" options, which are required for building Skia on
AArch64.

Comment 32

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 12b. Add asm/hwcap.h and sys/auxv.h as system headers (v1)

asm/hwcap.h and sys/auxv.h are used by Skia to check system capabilities
through `getauxval`.

Comment 33

[Lee Salzman [:lsalzman]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #31)
> Created attachment 8867930 [details] [diff] [review]
> 12a. Add missing crc32 dependency for Skia on AArch64 (v1)
> 
> Add an arm64 target cpu to generate_mozbuild.py so we can include the
> "arm64" and "crc32" options, which are required for building Skia on
> AArch64.

This looks like a dup of changes already made in bug 1364840?

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

Yep!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f37ecadee386
1. Update linux_syscall_support.h in Breakpad; r=ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ed1c66e7f7
2. Fix cast warning on AArch64 in plugin code; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/af763004c710
3. Don't use mmap2 on Android AArch64 in mozjemalloc; r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbdb04c9cb11
4a. Fix printf macro mismatches in mozglue; r=froydnj r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fca0781227
4b. Add relocation macros for AArch64; r=froydnj r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/44495073921f
4c. Add Divert case for AArch64; r=froydnj r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/aac0549dc030
4d. Define mmap ordering for AArch64; r=froydnj r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/339a792a67ab
4e. Fix std::min type error; r=froydnj r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/00d7a8c4978e
5. Fix warnings in widget; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f005adcc78
6. Add AArch64 support in xpcom; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/f173c1bfd2b3
7. Add AArch64 check to Fennec hardware checks; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec77f129df1
10. Fix opt build warnings in VIXL; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/d30ef114da50
11. Include AArch64 in nsWrapperCache 64-bit check; r=me

Comment 36

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/f37ecadee386
https://hg.mozilla.org/mozilla-central/rev/f5ed1c66e7f7
https://hg.mozilla.org/mozilla-central/rev/af763004c710
https://hg.mozilla.org/mozilla-central/rev/dbdb04c9cb11
https://hg.mozilla.org/mozilla-central/rev/f2fca0781227
https://hg.mozilla.org/mozilla-central/rev/44495073921f
https://hg.mozilla.org/mozilla-central/rev/aac0549dc030
https://hg.mozilla.org/mozilla-central/rev/339a792a67ab
https://hg.mozilla.org/mozilla-central/rev/00d7a8c4978e
https://hg.mozilla.org/mozilla-central/rev/38f005adcc78
https://hg.mozilla.org/mozilla-central/rev/f173c1bfd2b3
https://hg.mozilla.org/mozilla-central/rev/5ec77f129df1
https://hg.mozilla.org/mozilla-central/rev/d30ef114da50