1286436 - Add PC_sig for Linux/aarch64

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Makoto Kato [:m_kato]]

Now, we don't turn on native arm64 JIT.  When turning on it (define JS_CODEGEN_ARM64 without simulator), No PC_sig defines on WasmSignalHnadlers.cpp.  We should define. it.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Part 1. Add JS_CPU_ARM64 define for aarch64

We don't have JS_CPU_<cpu arch> define for aarch64 yet.   We should define it like other platforms.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Part 2. Add PC_sig define for aarch64

Let's define PC_sig() for Linux/aarch64 although we don't turn on aarch64 JIT without simulator

Comment 3

[Benjamin Bouvier [:bbouvier] (afk until August 17th)]

Comment on attachment 8770375 [details] [diff] [review]
Part 2. Add PC_sig define for aarch64

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

Thanks!

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8770373 [details] [diff] [review]
Part 1. Add JS_CPU_ARM64 define for aarch64

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

Seems reasonable but this needs review from a build system peer.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8770373 [details] [diff] [review]
Part 1. Add JS_CPU_ARM64 define for aarch64

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

::: js/src/old-configure.in
@@ +1169,5 @@
>      ENABLE_ION=1
>      AC_DEFINE(JS_CPU_ARM)
>      ;;
> +aarch64*-*)
> +    AC_DEFINE(JS_CPU_ARM64)

the only thing JS_CPU_ARM is used for is https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/WasmSignalHandlers.cpp#349 and https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/WasmSignalHandlers.cpp#854.
The latter is darwin-only, and AFAICT from some quick googling, it looks like the code would work equally on arm64.
The former doesn't work on arm64, but I question the use of the JS_CPU macros there.

So for your use case, I'd say you should just use __aarch64__ in WasmSignalHandlers.cpp, and not rely on adding a new define. And a followup should be filed to remove the JS_CPU_* defines. They aren't used that much, and could easily be replaced by the right compiler-defined macros.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Add PC_sig define for aarch64. r?bbouvier

Per glandium suggestion, we should use __aarch64__ instead of JS_CPU_ARM64

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Mike Hommey [:glandium] from comment #5)

> WasmSignalHandlers.cpp, and not rely on adding a new define. And a followup
> should be filed to remove the JS_CPU_* defines. They aren't used that much,
> and could easily be replaced by the right compiler-defined macros.

filed as bug 1287048

Comment 8

[Benjamin Bouvier [:bbouvier] (afk until August 17th)]

Comment on attachment 8771268 [details] [diff] [review]
Add PC_sig define for aarch64. r?bbouvier

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

Cheers.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4704ec22c26
Add PC_sig define for aarch64. r=bbouvier

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4704ec22c26