Add AArch64 support in various modules

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

(Blocks: 1 bug)

50 Branch
Firefox 55
Other
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(14 attachments, 7 obsolete attachments)

2.61 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.16 KB, patch
snorp
: review+
Details | Diff | Splinter Review
1.17 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.89 KB, patch
snorp
: review+
Details | Diff | Splinter Review
3.32 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.16 KB, patch
snorp
: review+
Details | Diff | Splinter Review
1.28 KB, patch
wtc
: review+
Details | Diff | Splinter Review
12.48 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.06 KB, patch
glandium
: review+
Details | Diff | Splinter Review
3.44 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.38 KB, patch
glandium
: review+
Details | Diff | Splinter Review
894 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
2.25 KB, patch
jchen
: review+
Details | Diff | Splinter Review
1.36 KB, patch
jchen
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8862578 [details] [diff] [review]
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.
Attachment #8862578 - Flags: review?(ted)
(Assignee)

Comment 2

a year ago
Created attachment 8862579 [details] [diff] [review]
2. Fix cast warning on AArch64 in plugin code (v1)

Fix a warning on AArch64 for casting to pointer.
Attachment #8862579 - Flags: review?(snorp)
(Assignee)

Comment 3

a year ago
Created attachment 8862580 [details] [diff] [review]
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.
Attachment #8862580 - Flags: review?(mh+mozilla)
(Assignee)

Comment 4

a year ago
Created attachment 8862582 [details] [diff] [review]
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.
Attachment #8862582 - Flags: review?(mh+mozilla)
(Assignee)

Comment 5

a year ago
Created attachment 8862583 [details] [diff] [review]
5. Fix warnings in widget (v1)

* Fix warnings using putenv with string literals.

* Fix a pritnf format warning.
Attachment #8862583 - Flags: review?(snorp)
(Assignee)

Comment 6

a year ago
Created attachment 8862587 [details] [diff] [review]
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.
Attachment #8862587 - Flags: review?(nfroyd)
(Assignee)

Comment 7

a year ago
Created attachment 8862588 [details] [diff] [review]
7. Add AArch64 check to Fennec hardware checks (v1)

Add checks for "arm64-v8a" when we check for supported platforms.
Attachment #8862588 - Flags: review?(snorp)
(Assignee)

Comment 8

a year ago
Created 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]

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.
(Assignee)

Comment 9

a year ago
Created attachment 8862590 [details] [diff] [review]
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.
Attachment #8862590 - Flags: review?(mh+mozilla)
Attachment #8862587 - Flags: review?(nfroyd) → review+
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.
Attachment #8862582 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

a year ago
Attachment #8862589 - Flags: review?(wtc)
Attachment #8862579 - Flags: review?(snorp) → review+
Attachment #8862583 - Flags: review?(snorp) → review+
Attachment #8862588 - Flags: review?(snorp) → review+
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)
Attachment #8862582 - Flags: feedback-
Attachment #8862580 - Flags: review?(mh+mozilla) → review+
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).
Attachment #8862590 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 13

a year ago
Created attachment 8865002 [details] [diff] [review]
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.
Attachment #8865002 - Flags: review?(mh+mozilla)
(Assignee)

Updated

a year ago
Attachment #8862590 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Created attachment 8865587 [details] [diff] [review]
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.
Attachment #8865587 - Flags: review?(mh+mozilla)
(Assignee)

Updated

a year ago
Attachment #8862582 - Attachment is obsolete: true

Comment 15

a year ago
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.
Attachment #8862589 - Flags: review?(wtc)
Attachment #8862589 - Flags: review?(kaie)
Attachment #8862589 - Flags: review+
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))
Attachment #8865002 - Flags: review?(mh+mozilla) → feedback+
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.
Attachment #8865587 - Flags: review?(mh+mozilla) → feedback+

Updated

a year ago
Attachment #8862589 - Attachment description: 8. Don't set PR_ALTERNATE_INT64_TYPEDEF for Android in NSPR → 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]

Updated

a year ago
Depends on: 1363329
(Assignee)

Comment 18

a year ago
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.
Attachment #8865002 - Flags: review?(mh+mozilla)
(Assignee)

Comment 19

a year ago
Created attachment 8865939 [details] [diff] [review]
4a. Fix printf macro mismatches in mozglue (v3)
Attachment #8865939 - Flags: review?(mh+mozilla)
(Assignee)

Updated

a year ago
Attachment #8865587 - Attachment is obsolete: true
(Assignee)

Comment 20

a year ago
Created attachment 8865941 [details] [diff] [review]
4b. Add relocation macros for AArch64 (v3)
Attachment #8865941 - Flags: review?(mh+mozilla)
(Assignee)

Comment 21

a year ago
Created attachment 8865942 [details] [diff] [review]
4c. Add Divert case for AArch64 (v3)
Attachment #8865942 - Flags: review?(mh+mozilla)
(Assignee)

Comment 22

a year ago
Created attachment 8865943 [details] [diff] [review]
4d. Define mmap ordering for AArch64 (v3)
Attachment #8865943 - Flags: review?(mh+mozilla)
(Assignee)

Comment 23

a year ago
Created attachment 8865944 [details] [diff] [review]
4e. Fix std::min type error (v3)
Attachment #8865944 - Flags: review?(mh+mozilla)
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.
Attachment #8862589 - Flags: review?(kaie)
(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...
Attachment #8865939 - Flags: review?(mh+mozilla) → review+
Attachment #8865941 - Flags: review?(mh+mozilla) → review+
Attachment #8865942 - Flags: review?(mh+mozilla) → review+
Attachment #8865943 - Flags: review?(mh+mozilla) → review+
Attachment #8865944 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8862578 - Flags: review?(ted)
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.
Attachment #8862578 - Flags: review+
(Assignee)

Comment 28

a year ago
Created attachment 8867313 [details] [diff] [review]
9. Fix AArch64 build in jemalloc (v3)

Update jemalloc to upstream stable-4 branch to pick up the commit.
Attachment #8867313 - Flags: review?(mh+mozilla)
(Assignee)

Updated

a year ago
Attachment #8865002 - Attachment is obsolete: true
Attachment #8865002 - Flags: review?(mh+mozilla)
(Assignee)

Comment 29

a year ago
Created attachment 8867928 [details] [diff] [review]
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.
Attachment #8867928 - Flags: review+
(Assignee)

Comment 30

a year ago
Created attachment 8867929 [details] [diff] [review]
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.
Attachment #8867929 - Flags: review+
(Assignee)

Comment 31

a year ago
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.
Attachment #8867930 - Flags: review?(lsalzman)
(Assignee)

Comment 32

a year ago
Created attachment 8867931 [details] [diff] [review]
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`.
Attachment #8867931 - Flags: review?(mh+mozilla)
(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?
Flags: needinfo?(nchen)
(Assignee)

Comment 34

a year ago
Yep!
Flags: needinfo?(nchen)
(Assignee)

Updated

a year ago
Attachment #8867930 - Attachment is obsolete: true
Attachment #8867930 - Flags: review?(lsalzman)
(Assignee)

Updated

a year ago
Attachment #8867931 - Attachment is obsolete: true
Attachment #8867931 - Flags: review?(mh+mozilla)
(Assignee)

Updated

a year ago
Attachment #8867313 - Attachment is obsolete: true
Attachment #8867313 - Flags: review?(mh+mozilla)

Comment 35

a year ago
Pushed by nchen@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.