1981138 - jit-test: skip-if conditions are not respected with --spectre-mitigations=on on platforms without such capabilities

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Rong Bao [:csmantle]]

On platforms without proper spectreZeroRegister capability, the JS shell crashes when running tests with // |jit-test| --spectre-mitigations=on, even if these tests are masked using skip-if: directives.

Currently, the test skipping mechanism relies on the JS shell's evaluation of an if (...) quit() statement. This happens after initializing the self-hosted code, so it never prevents the shell from crashing if no Spectre-mitigated self-hosted code can be compiled.

For example, test ion/bug1433496.js is supposed to be skipped on mips64 and riscv64 platforms since they do not have spectreZeroRegister implemented. From the run log below, however, it crashes before the feature test happens.

$ ./mach jit-test ion/bug1433496.js -- --show-cmd
/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js -p /home/mantle/workspace/mozilla-firefox-dev/js/src/jit-test/lib/prologue.js --spectre-mitigations=on -e 'const platform='"'"'linux'"'"'' -e 'const libdir='"'"'/home/mantle/workspace/mozilla-firefox-dev/js/src/jit-test/lib/'"'"'' -e 'const scriptdir='"'"'/home/mantle/workspace/mozilla-firefox-dev/js/src/jit-test/tests/ion/'"'"'' -e 'if ((getBuildConfiguration("mips64") || getBuildConfiguration("riscv64"))) quit(59)' --module-load-path /home/mantle/workspace/mozilla-firefox-dev/js/src/jit-test/modules/ -f /home/mantle/workspace/mozilla-firefox-dev/js/src/jit-test/tests/ion/bug1433496.js
[393697] Hit MOZ_CRASH(spectreZeroRegister) at /home/mantle/workspace/mozilla-firefox-dev/js/src/jit/riscv64/MacroAssembler-riscv64-inl.h:2084
#01: ???[/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js +0x2cd3668]
#02: ???[/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js +0x2df976a]
#03: ???[/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js +0x2df8dd0]
#04: JSRuntime::createJitRuntime(JSContext*)[/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js +0x204da44]
#05: JS::InitSelfHostedCode(JSContext*, mozilla::Span<unsigned char const, 18446744073709551615ul>, bool (*)(JSContext*, mozilla::Span<unsigned char const, 18446744073709551615ul>))[/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js +0x1eaab52]
#06: ???[/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js +0x1bb4b1a]
#07: ???[/usr/lib/libc.so.6 +0x27675]
#08: __libc_start_main[/usr/lib/libc.so.6 +0x27729]
#09: ???[/home/mantle/workspace/mozilla-firefox-dev/obj-x86_64-pc-linux-gnu/dist/bin/js +0x1ba92e9]
#10: ??? (???:???)
Exit code: -11
FAIL - ion/bug1433496.js
FAILURES:
    --spectre-mitigations=on ion/bug1433496.js
TIMEOUTS:

Comment 1

[Nicolas B. Pierron [:nbp]]

Sounds fun.

I guess one way might be to gather all the skip-if ahead and spawn a single process to generate the list.

This process could also be used to generate the self-hosted cache, that currently requires to process the first script run differently as well.

Comment 2

[Rong Bao [:csmantle]]

Perhaps another way is to spawn a dedicated "checker" process for each test, and then skip or continue the actual test case based on the checker's returned status code. Checker processes do not carry CLI flags when run, and only evaluate the feature test expressions.

Some thoughts:

• If the skip-if expr rely on the presence/absence of interpreter CLI flags, this method fails. Currently I haven't checked if there are any such tests yet.
• The total number of children spawned would double, but at any given time the active children process count should stay roughly the same.
• If the checker process somehow crashes unexpectedly we also need to fail the respective test. May need to distinguish checker vs tester process output when reporting errors?
• Interactions with current jit-test options. For example, should --show-cmd print cmdline for both processes? Does this hinder test debugging?

Comment 3

[Rong Bao [:csmantle]]

Hi Nicolas,

Do we really need self-hosted code caches when evaluating skip-ifs? I'm not very familiar with the inner workings of this, so I would be grateful if I could use some help and pointers.

Comment 4

[Jan de Mooij [:jandem]]

The simplest fix for this may be to ignore the --spectre-mitigations=on shell flag on platforms that don't support this (maybe just print a warning to stderr). That's consistent with how we don't expose the Spectre browser prefs on those platforms.

Comment 5

[Rong Bao [:csmantle]]

(In reply to Jan de Mooij [:jandem] from comment #4)

The simplest fix for this may be to ignore the --spectre-mitigations=on shell flag on platforms that don't support this (maybe just print a warning to stderr). That's consistent with how we don't expose the Spectre browser prefs on those platforms.

This sounds awesome to me! It's rather easy to implement and seems to be the least intrusive to the test harness.

Comment 6

[Rong Bao [:csmantle]]

Attachment: Bug 1981138 - Ignore JS shell cmdline argument "--spectre-mitigations=on" on unsupported platforms. r=jandem

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Rong Bao [:csmantle] from comment #3)

Do we really need self-hosted code caches when evaluating skip-ifs? I'm not very familiar with the inner workings of this, so I would be grateful if I could use some help and pointers.

No.
The simplest is to not have it for running the skip-if conditions.

The test suite can remain the way it is for the self-hosted code cache, and we can open follow-up bug to optimize this later.

Comment 8

[Nicolas B. Pierron [:nbp]]

For posterity:

(In reply to Rong Bao [:csmantle] from comment #2)

Perhaps another way is to spawn a dedicated "checker" process for each test, and then skip or continue the actual test case based on the checker's returned status code. Checker processes do not carry CLI flags when run, and only evaluate the feature test expressions.

Spawning an extra JS shell for each test would be costly on our CI.
A large chunk of the time comes from spawning processes, not from running the test cases, this would literally double this base time.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/605a49c1b7f9
https://hg.mozilla.org/integration/autoland/rev/04a8571d882c
Ignore JS shell cmdline argument "--spectre-mitigations=on" on unsupported platforms. r=jandem

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/04a8571d882c