BaselineCompiler: Crashes on hardware without SSE2 support

VERIFIED FIXED in Firefox 23

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug, {crash, regression, topcrash})

23 Branch
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22 unaffected, firefox23 verified)

Details

(Whiteboard: [startupcrash], crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: SadJit

Updated

6 years ago
Crash Signature: [@ EnterBaseline]
Keywords: crash

Updated

6 years ago
Severity: normal → critical
Keywords: regression
Whiteboard: [startupcrash]
Version: unspecified → 23 Branch
(Assignee)

Comment 1

6 years ago
Posted 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)
(Assignee)

Comment 2

6 years ago
Posted 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+
(Assignee)

Comment 4

6 years ago
Posted 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)
(Assignee)

Comment 5

6 years ago
(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+
(Assignee)

Comment 7

6 years ago
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
Last Resolved: 6 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

Updated

6 years ago
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.