1786619 - mozjs (SpiderMonkey) 102 build regression on ARMv5: selected processor does not support `dmb sy' in ARM mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P5)

Description

[Simon McVittie]

Steps to reproduce:

Attempted to build Debian's mozjs102 package (the Spidermonkey JavaScript engine as a subset of Firefox, used to build GNOME's gjs) for the 'armel' architecture (ARMv5 EABI softfloat).

Specifically, the package I was building is: https://snapshot.debian.org/package/mozjs102/102.2.0-1/

Actual results:

Lots of messages like these:

{standard input}: Assembler messages:
{standard input}: Error: selected processor does not support dmb sy' in ARM mode {standard input}: Error: selected processor does not supportldrexb
r0,[r1]' in ARM mode
{standard input}: Error: selected processor does not support `strexb
r3,r2,[r1]' in ARM mode

and the build fails.

Expected results:

We're compiling for ARMv5, so if this particular conformance level doesn't have atomic opcodes available, I had expected that we'd end up using a fallback path via libatomic or similar.

Comment 1

[Simon McVittie]

I tried editing js/src/jit/GenerateAtomicOperations.py so it would only emit the atomic operations on ARMv7 or better, falling back to the "feeling lucky" atomics implementation on ARMv5, which partially solved this but led to lots of test failures. I'll try to attach the patch that I tried.

Comment 2

[Simon McVittie]

Attachment: jit: Only use ARMv7 atomic operations on ARMv7 or higher

This works well enough to compile, but triggers a lot of test failures; but perhaps it points someone in the right direction?

Comment 3

[Simon McVittie]

mozjs 102 on ARMv5 also suffers from Bug #1786623 (as a workaround, I'm forcing use of gcc-11 and g++-11).

The version I'm trying to build already has the patch from Bug #1786621 to avoid an unrelated build failure.

Comment 4

[Nicolas B. Pierron [:nbp]]

Thanks for the investigation and the patch.

The patch looks good however, we are using phabricator to submit patches nowadays.
Follow the instructions listed below to submit your patches such that it can be integrated in SpiderMonkey / Firefox.

https://firefox-source-docs.mozilla.org/devtools/contributing/code-reviews-setup.html
https://firefox-source-docs.mozilla.org/devtools/contributing/making-prs.html

Comment 5

[Simon McVittie]

I'm not convinced the patch is good - when I tried it, I got quite a lot of unit test failures. Is ARMv5 with the fallback locking implementation something that is expected to basically work, or is it expected to be broken/incorrect/unreliable?

Comment 6

[Nicolas B. Pierron [:nbp]]

I think you might also want to filter out ARMv5 in the function which enables the JIT by default:
https://searchfox.org/mozilla-central/source/js/moz.configure#180

Comment 7

[Simon McVittie]

Thanks, that's a good idea - I'll look into that when I have a chance.

Comment 8

[Simon McVittie]

Equivalent downstream bug FYI: https://bugs.debian.org/1017961

Comment 9

[Simon McVittie]

Is there a straightforward way to find out from a build log whether the JIT is enabled or not? It looks as though last time we successfully compiled Spidermonkey 91 for ARMv5 (before Bug #1786623), we were building it with -DJS_CODEGEN_ARM=1, compared with the -DJS_CODEGEN_NONE=1 on an entirely unsupported architecture like riscv64. Is -DJS_CODEGEN_NONE=1 synonymous with disabling the JIT?

Does this mean we were previously enabling a JIT that actually requires post-ARMv5 CPU instructions, and none of the users who are ostensibly using ARMv5 CPUs ever noticed that error?

Or did the ARM JIT in Spidermonkey 91 support ARMv5 (without atomic operation opcodes), but the ARM JIT in Spidermonkey 102 doesn't any more?

Comment 10

[Simon McVittie]

Attachment: Enable JIT by default on ARMv7 or up, but not ARMv6

I tried this as suggested

Comment 11

[Simon McVittie]

Attachment: Enable Rust SIMD on ARMv7 or up, but not ARMv6 or below

For completeness, I also tried this at the same time

Comment 12

[Simon McVittie]

With the three attached patches (which, again, are not ready to be considered for merge), I get -DJS_CODEGEN_NONE=1 in the compiler arguments, which if I understand correctly means we are no longer enabling the JIT in this build.

However, I'm still getting the test failures similar to the ones I saw when I left the ARM JIT enabled, such as these segfaults (and more, all the Atomics/*/bigint test-cases seem to be affected):

## test262/built-ins/Atomics/xor/bigint/good-views.js: rc = -11, run time = 0.030321
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/xor/bigint/good-views.js | (args: "") [0.0 s]
## test262/built-ins/Atomics/xor/bigint/non-shared-bufferdata.js: rc = -11, run time = 0.038158
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/xor/bigint/non-shared-bufferdata.js | (args: "") [0.0 s]
## test262/built-ins/Atomics/or/bigint/good-views.js: rc = -11, run time = 0.038281
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/or/bigint/good-views.js | (args: "") [0.0 s]
## test262/built-ins/Atomics/or/bigint/non-shared-bufferdata.js: rc = -11, run time = 0.032658
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/or/bigint/non-shared-bufferdata.js | (args: "") [0.0 s]
## test262/built-ins/Atomics/sub/bigint/good-views.js: rc = -11, run time = 0.040853
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/sub/bigint/good-views.js | (args: "") [0.0 s]
## test262/built-ins/Atomics/sub/bigint/non-shared-bufferdata.js: rc = -11, run time = 0.035408
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/sub/bigint/non-shared-bufferdata.js | (args: "") [0.0 s]

and this test failure:

## test262/built-ins/Atomics/compareExchange/good-views.js: rc = 3, run time = 0.03819
/home/smcv/mozjs-armel/js/src/tests/test262/built-ins/Atomics/shell.js:188:7 uncaught exception: Test262Error: The value of view[3] is 0 Expected SameValue(«-5», «0») to be true (Testing with Int16Array.)
Stack:
  testWithTypedArrayConstructors@/home/smcv/mozjs-armel/js/src/tests/test262/built-ins/Atomics/shell.js:188:7
  @/home/smcv/mozjs-armel/js/src/tests/test262/built-ins/Atomics/compareExchange/good-views.js:16:31
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/compareExchange/good-views.js | (args: "") [0.0 s]

I don't have a good picture of what this implies for whether this mozjs build is still basically functional, or whether it will be hopelessly crashy in practical use, or somewhere in between. (Sorry, I don't use ARMv5 myself, and I'm doing all this on a remote machine.)

Comment 13

[Tom S [:evilpie]]

I would expect most code to not use Atomics, so I think you might get away with these test failures.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Simon McVittie from comment #9)

Is -DJS_CODEGEN_NONE=1 synonymous with disabling the JIT?

Yes.

Does this mean we were previously enabling a JIT that actually requires post-ARMv5 CPU instructions, and none of the users who are ostensibly using ARMv5 CPUs ever noticed that error?

Possibly yes. The set of dynamic features checked is the following:
https://searchfox.org/mozilla-central/source/js/src/jit/arm/Architecture-arm.cpp#279

Or did the ARM JIT in Spidermonkey 91 support ARMv5 (without atomic operation opcodes), but the ARM JIT in Spidermonkey 102 doesn't any more?

The Assembler / MacroAssembler was developed with ARMv6/ARMV7 in mind. It is possible that the subset we are using is safe to use on some older versions, but this was not intended.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Bug 1786619 - Use fallback atomic operations on ARM < v7.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc934c3360f4
Use fallback atomic operations on ARM < v7. r=jandem

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 9295210 [details]
Bug 1786619 - Use fallback atomic operations on ARM < v7.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Build fix for some tier3 ARM targets
• User impact if declined:
• Fix Landed on Version: 107
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): NPOTB

Comment 18

[Atila Butkovits]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9295210 [details]
Bug 1786619 - Use fallback atomic operations on ARM < v7.

NPOTB for Firefox, but useful for embedders. Approved for 102.4esr.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/470147864827

Comment 21

[michal]

I am still having an issue with 102.4.0 and -march=armv6 -mfpu=vfp -mfloat-abi=hard

{standard input}: Assembler messages:
{standard input}:15529: Error: selected processor does not support `isb' in ARM mode