1279529 - Assertion failure: HasSSE2(), at js/src/jit/x86-shared/Assembler-x86-shared.h:2265

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision 3ccccf8e5036 (build with --enable-debug --enable-more-deterministic --32, run with --fuzzing-safe --no-threads --ion-eager --no-fpu):

quit();

Actually no testcase is needed:

$ ./js-dbg-32-dm-clang-darwin-3ccccf8e5036 --fuzzing-safe --no-threads --ion-eager --no-fpu -e "quit();"
Assertion failure: HasSSE2(), at /Users/skywalker/trees/mozilla-central/js/src/jit/x86-shared/Assembler-x86-shared.h:2265
Segmentation fault: 11

Backtrace:

0   js-dbg-32-dm-clang-darwin-3ccccf8e5036	0x006ae9dc js::jit::AssemblerX86Shared::vucomisd(js::jit::FloatRegister, js::jit::FloatRegister) + 172 (Assembler-x86-shared.h:2265)
1   js-dbg-32-dm-clang-darwin-3ccccf8e5036	0x006ae74e js::jit::MacroAssembler::canonicalizeDouble(js::jit::FloatRegister) + 62 (MacroAssembler-x86-shared.h:109)
2   js-dbg-32-dm-clang-darwin-3ccccf8e5036	0x006136c7 js::jit::MacroAssembler::PushRegsInMask(js::jit::LiveSet<js::jit::RegisterSet>) + 471
3   js-dbg-32-dm-clang-darwin-3ccccf8e5036	0x0048bd0e js::jit::MacroAssembler::assumeUnreachable(char const*) + 62 (RegisterSets.h:324)
4   js-dbg-32-dm-clang-darwin-3ccccf8e5036	0x00683b89 js::jit::JitRuntime::generateProfilerExitFrameTailStub(JSContext*) + 905 (Trampoline-x86.cpp:1060)
5   js-dbg-32-dm-clang-darwin-3ccccf8e5036	0x00329d55 js::jit::JitRuntime::initialize(JSContext*, js::AutoLockForExclusiveAccess&) + 341 (Ion.cpp:216)
/snip

For detailed crash information, see attachment.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2c86039b1c68
user:        Jeff Walden
date:        Mon May 23 22:49:56 2016 +0200
summary:     Bug 1245627: Canonicalize before storing a floating point value in deterministic mode; r=nbp

Waldo, is bug 1245627 a likely regressor?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1279529; Remove the --no-fpu option in the shell;

Review commit: https://reviewboard.mozilla.org/r/59134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59134/

Comment 4

[Nicolas B. Pierron [:nbp]]

https://reviewboard.mozilla.org/r/59134/#review56168

Why would this patch be sufficient?
It feels to me that many other storeDouble calls can be used and that this would just delay the fuzz-bug to another test case?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> https://reviewboard.mozilla.org/r/59134/#review56168
> 
> Why would this patch be sufficient?
> It feels to me that many other storeDouble calls can be used and that this
> would just delay the fuzz-bug to another test case?

Well, this is only happening in --enable-more-deterministic builds, which are only used by the fuzzers, so I wouldn't spend too much time thinking about it. The issue here is just that the cmpsd instruction is not available with the flags passed at runtime (--no-fpu).

There is a whole category of similar fuzz bugs indeed just waiting to happen like this, but I think spending more time with the storeDouble vs storeUncanonicalizedDouble issue (which happens only in fuzzing) is a waste of time. Anyway, if you have any better fix in mind, I'd be glad to review it.

Comment 9

[Nicolas B. Pierron [:nbp]]

Based on the current context of SSE2 support, I think an alternative would be to remove this --no-fpu flag.  AFAIK, it used to disable the FPU instruction on ARM, and also SSE2 support.

So, we might as well remove this flag and prevent the fuzzers from using it.

Comment 10

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

I am more than happy to stop fuzzing --no-fpu if we feel that it is no longer necessary.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Based on the current context of SSE2 support, I think an alternative would
> be to remove this --no-fpu flag.  AFAIK, it used to disable the FPU
> instruction on ARM, and also SSE2 support.
> 
> So, we might as well remove this flag and prevent the fuzzers from using it.

ARM hardware without a FPU is still a potential concern, even if we no longer support x86 without SSE2. Do we still support these ARM devices?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jan de Mooij [:jandem] from comment #11)
> ARM hardware without a FPU is still a potential concern, even if we no
> longer support x86 without SSE2. Do we still support these ARM devices?

For what it's worth, --no-fpu is a no-op on other platforms than x86/x64. The ARM simulator uses a different path for enabling soft/hard FP.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Stepping back a bit, it seems that SSE2 is required on windows 32 bits and mac os 32 bits, but it's unclear for linux 32 bits (64 bits intel processors all have SSE2, fwiw).

Comment 14

[Nicolas B. Pierron [:nbp]]

Do we still want support non-SSE2 linux platforms?

Comment 15

[Ralph Giles (:rillian)]

I'm not aware of an official decision on non-SSE2 x86 linux, but we're certainly moving in that direction. In practice our official builds require SSE2, but it's still possible for downstream packagers to undo that. Unless there's a reversal I expect it will become required soon, so dropping this from fuzzing sounds reasonable to me.

A number of linux distros claim to support non-SSE2, but it's not clear they have testing to ensure this for everything they build.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8762457 [details]
Bug 1279529; Remove the --no-fpu option in the shell;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59134/diff/1-2/

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

All green on try, fwiw: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93fdf5e2c851

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8762457 [details]
Bug 1279529; Remove the --no-fpu option in the shell;

https://reviewboard.mozilla.org/r/59134/#review57196

Nice that it will make jit-tests faster in automation.

Comment 19

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

> Nice that it will make jit-tests faster in automation.

\o/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4348f782dae4
Remove the --no-fpu option from the shell; r=jandem

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4348f782dae4