Closed
Bug 1286436
Opened 6 years ago
Closed 6 years ago
Add PC_sig for Linux/aarch64
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: m_kato, Assigned: m_kato)
Details
Attachments
(1 file, 2 obsolete files)
1.55 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
We don't have JS_CPU_<cpu arch> define for aarch64 yet. We should define it like other platforms.
Attachment #8770373 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•6 years ago
|
||
Let's define PC_sig() for Linux/aarch64 although we don't turn on aarch64 JIT without simulator
Attachment #8770375 -
Flags: review?(bbouvier)
Comment 3•6 years ago
|
||
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 4•6 years ago
|
||
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 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
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)
Assignee | ||
Comment 7•6 years ago
|
||
(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
Updated•6 years ago
|
Attachment #8771268 -
Attachment is patch: true
Comment 8•6 years ago
|
||
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
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4704ec22c26
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•