Closed Bug 1279529 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: gkw, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

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.
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?
Blocks: 1245627
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jwalden+bmo) → needinfo?(bbouvier)
Flags: needinfo?(bbouvier)
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?
(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.
Flags: needinfo?(nicolas.b.pierron)
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.
Flags: needinfo?(nicolas.b.pierron)
I am more than happy to stop fuzzing --no-fpu if we feel that it is no longer necessary.
(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?
(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.
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).
Do we still want support non-SSE2 linux platforms?
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles)
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/
Attachment #8762457 - Attachment description: Bug 1279529: Use storeUncanonicalized in PushRegistersr; → Bug 1279529; Remove the --no-fpu option in the shell;
Attachment #8762457 - Flags: review?(nicolas.b.pierron) → review?(jdemooij)
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
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.
Attachment #8762457 - Flags: review?(jdemooij) → review+
> Nice that it will make jit-tests faster in automation.

\o/
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4348f782dae4
Remove the --no-fpu option from the shell; r=jandem
https://hg.mozilla.org/mozilla-central/rev/4348f782dae4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.