935791 - Add an option to disable SSE3 and SSE4 code generation on the JS shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: no-sse4.patch

This patch adds an option to disable SSE4 on the JS shell. SSE is still active, just SSE4 isn't anymore with that option, in debug builds. That could help to find some code generation bugs, so cc'ing all the fuzzing security team. Could be also useful to test in the jit-test harness.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 828361 [details] [diff] [review]
no-sse4.patch

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

Thanks for doing this. While you're here, can you also add a --no-sse3 flag for SSE3? We have some SSE3 code in CodeGeneratorX86::visitOutOfLineTruncate for instance.

We could add --no-sse3 to jit_test.py --tbpl, to test the no-SSE3/SSE4 code paths. Maybe add it to the "--ion-eager --ion-check-range-analysis" configuration?

::: js/src/assembler/assembler/MacroAssemblerX86Common.h
@@ +1404,5 @@
>          else
>              s_sseCheckState = NoSSE;
> +
> +#ifdef DEBUG
> +        if ((s_sseCheckState == HasSSE4_2 || s_sseCheckState == HasSSE4_1) && s_SSE4Disabled)

Nit: s_sseCheckState >= HasSSE4_1 (see for instance isSSE41Present)

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: --no-sse3, --no-sse4, added in tbpl

I added a --no-sse3 flag that has the same purpose but for SSE3 and above.

Also, with this patch applied, asm.js/testCaching.js and asm.js/testBullet.js don't pass locally with --no-sse3 or --no-sse4. Luke, do you have any idea of what could cause that? Does it reproduce on your machine? That seems totally unrelated to me. I suspect an instruction to have a bad SSE3 or SSE2 implementation and mess up with the registers state, but that's a wild guess.

Comment 3

[Luke Wagner [:luke]]

Oh, hah, I know: both of those tests assert a cache hit using a separate process (spawned by nestedShell()) to do the compilation that fills the cache.  However, SSE will only be turned off the original, not nested, process so the MachineID included in the serialized cache file will mismatch (as it should) and the cache won't hit.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 828609 [details] [diff] [review]
--no-sse3, --no-sse4, added in tbpl

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

Maybe the isCachingEnabled shell function should just return false with --no-sse3/--no-sse4? Nice catch btw :)

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: --no-sse3, --no-sse4, added in tbpl, propagates flags to nested shells

Carrying forward r+ from jandem.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/526ba3ace37a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/526ba3ace37a

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> so cc'ing all the fuzzing
> security team. Could be also useful to test in the jit-test harness.

Thanks for the headsup. I've added --no-sse3 to the list of flags via fuzzing rev 930b0a900fab.