Closed Bug 858022 Opened 7 years ago Closed 7 years ago

BaselineCompiler: Crashes on hardware without SSE2 support

Categories

(Core :: JavaScript Engine, defect, critical)

23 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox22 --- unaffected
firefox23 --- verified

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, topcrash, Whiteboard: [startupcrash])

Crash Data

Attachments

(1 file, 2 obsolete files)

First EnterBaseline crash reports are in. They all seem to be on old hardware, WinXP or Linux, caused by illegal instructions. It looks like these machines don't support SSE2.

TI is disabled if masm.supportsFloatingPoint() is false and this means Ion is disabled as well. JM is not disabled but calls masm.supportsFloatingPoint() in various places (bug 712261).

We should call JSC::MacroAssembler().supportsFloatingPoint() and either:

(1) Disable Baseline completely.
(2) Don't attach FP-related stubs.

Some reports:

https://crash-stats.mozilla.com/report/index/13beb6db-9c97-4dd3-b889-5ae2d2130403

https://crash-stats.mozilla.com/report/index/5a0038f2-7fd7-41d8-a59b-1df3a2130403
Blocks: SadJit
Crash Signature: [@ EnterBaseline]
Keywords: crash
Severity: normal → critical
Keywords: regression
Whiteboard: [startupcrash]
Version: unspecified → 23 Branch
Attached patch Patch (obsolete) — Splinter Review
Disable baseline on platforms without SSE2 or ARMv6 without VFP (there are also some ARMv6 crash reports).

Let me know if you think we should support this somehow and I can file a follow-up bug, but I'd like to get this patch in first to stop the crashes and see what's left.
Attachment #733352 - Flags: review?(dvander)
Attached patch Patch (obsolete) — Splinter Review
Attachment #733352 - Attachment is obsolete: true
Attachment #733352 - Flags: review?(dvander)
Attachment #733371 - Flags: review?(dvander)
Comment on attachment 733371 [details] [diff] [review]
Patch

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

Can you file a follow-up bug for non-SSE2 support in baseline? We should fix that for this release.

::: js/src/jscntxt.h
@@ +1734,5 @@
>  
>      /* Location to stash the iteration value between JSOP_MOREITER and JSOP_ITERNEXT. */
>      js::Value           iterValue;
>  
> +    bool                 jitSupportsFloatingPoint;

This should probably go on the runtime.
Attachment #733371 - Flags: review?(dvander) → review+
Attached patch Patch v2Splinter Review
This patch:

* Adds a --no-fpu flag to the shell, atm only affects x86 debug builds. This flag makes the assembler report there's no support for SSE2+.

* Flag is added to jit_test.py to avoid breaking this configuration.

* Adds JS_ASSERT(HasSSE2()); to Ion's MacroAssembler, like JSC's MacroAssembler.

* Modifies BC to not emit instructions that require SSE2. Passes all jit-tests with --no-fpu.
Attachment #733371 - Attachment is obsolete: true
Attachment #735097 - Flags: review?(dvander)
(Forgot to mention in comment 4, patch also slightly modifies asm.js/testCall.js to avoid an over-recursion error on OS X).
Comment on attachment 735097 [details] [diff] [review]
Patch v2

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

Awesome, thank you for doing this! It'll be good to finally have this on tbpl.

::: js/src/ion/BaselineIC.cpp
@@ +4013,5 @@
>      masm.bind(&convertDoubles);
> +    if (cx->runtime->jitSupportsFloatingPoint)
> +        masm.convertInt32ValueToDouble(valueAddr, R0.scratchReg(), &convertDoublesDone);
> +    else
> +        masm.breakpoint();

Is this reachable? Should it be jump(&failure) instead?

@@ +4181,5 @@
>      masm.bind(&convertDoubles);
> +    if (cx->runtime->jitSupportsFloatingPoint)
> +        masm.convertInt32ValueToDouble(valueAddr, R0.scratchReg(), &convertDoublesDone);
> +    else
> +        masm.breakpoint();

same

::: js/src/ion/Ion.cpp
@@ +198,5 @@
>      if (!functionWrappers_ || !functionWrappers_->init())
>          return false;
>  
> +    if (cx->runtime->jitSupportsFloatingPoint) {
> +        if (!bailoutTables_.reserve(FrameSizeClass::ClassLimit().classId()))

nit: can you add a one-line comment that this stuff is ion-only?
Attachment #735097 - Flags: review?(dvander) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f10884c6a91e

(In reply to David Anderson [:dvander] from comment #6)
> Is this reachable? Should it be jump(&failure) instead?

No, as the comment explains double-arrays are only created by IonMonkey, and Ion is disabled if the CPU does not support floating-point operations.
https://hg.mozilla.org/mozilla-central/rev/f10884c6a91e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
I hit this updating an oldish build to a 4/10 build. Then updated via safe mode to 4/18 and no more startup crashes on nightly (23)
Status: RESOLVED → VERIFIED
Depends on: 865468
Depends on: 865507
See Also: → 873334
Marking status-firefox23:verified based on comment 9.
You need to log in before you can comment on or make changes to this bug.