Closed Bug 1286436 Opened 3 years ago Closed 3 years ago

Add PC_sig for Linux/aarch64

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(1 file, 2 obsolete files)

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.
We don't have JS_CPU_<cpu arch> define for aarch64 yet.   We should define it like other platforms.
Attachment #8770373 - Flags: review?(jdemooij)
Let's define PC_sig() for Linux/aarch64 although we don't turn on aarch64 JIT without simulator
Attachment #8770375 - Flags: review?(bbouvier)
Comment on attachment 8770375 [details] [diff] [review]
Part 2. Add PC_sig define for aarch64

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

Thanks!
Attachment #8770375 - Flags: review?(bbouvier) → review+
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.
Attachment #8770373 - Flags: review?(jdemooij) → review?(mh+mozilla)
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.
Attachment #8770373 - Flags: review?(mh+mozilla)
Per glandium suggestion, we should use __aarch64__ instead of JS_CPU_ARM64
Attachment #8770373 - Attachment is obsolete: true
Attachment #8770375 - Attachment is obsolete: true
Attachment #8771268 - Flags: review?(bbouvier)
(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
Attachment #8771268 - Attachment is patch: true
Comment on attachment 8771268 [details] [diff] [review]
Add PC_sig define for aarch64. r?bbouvier

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

Cheers.
Attachment #8771268 - Flags: review?(bbouvier) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4704ec22c26
Add PC_sig define for aarch64. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/b4704ec22c26
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.