Closed
Bug 1279529
Opened 9 years ago
Closed 9 years ago
Assertion failure: HasSSE2(), at js/src/jit/x86-shared/Assembler-x86-shared.h:2265
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: gkw, Assigned: bbouvier)
References
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.
![]() |
Reporter | |
Comment 1•9 years ago
|
||
![]() |
Reporter | |
Comment 2•9 years ago
|
||
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)
Updated•9 years ago
|
Flags: needinfo?(jwalden+bmo) → needinfo?(bbouvier)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bbouvier)
Assignee | ||
Comment 3•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59134/
Attachment #8762457 -
Flags: review?(nicolas.b.pierron)
Comment 4•9 years ago
|
||
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?
Assignee | ||
Comment 6•9 years ago
|
||
(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)
Comment 9•9 years ago
|
||
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)
![]() |
Reporter | |
Comment 10•9 years ago
|
||
I am more than happy to stop fuzzing --no-fpu if we feel that it is no longer necessary.
Comment 11•9 years ago
|
||
(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?
Assignee | ||
Comment 12•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
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 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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 | ||
Comment 17•9 years ago
|
||
All green on try, fwiw: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93fdf5e2c851
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Comment 18•9 years ago
|
||
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+
![]() |
Reporter | |
Comment 19•9 years ago
|
||
> Nice that it will make jit-tests faster in automation.
\o/
Comment 20•9 years ago
|
||
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4348f782dae4
Remove the --no-fpu option from the shell; r=jandem
Comment 21•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•