IonMonkey (ARM): incorrect detection of the number of FP registers is causing SIGILL crashes.

RESOLVED FIXED in Firefox 33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

Trunk
mozilla34
ARM
All
Points:
---

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
The logic for the detection of the number of ARM double precision (DP) registers supported by the VFP implementation is returning 32 registers even when VFPv3 is not supported.

Without the VFPv3 capability flag set, only 16 DP registers are supported.

When the VFPv3 flags is set, there may be 16 or 32 DP registers. When there are only 16 DP registers the VFPv3D16 flag is set, and this flags is only set when VFPv3 is available.

The current code in incorrectly checking the VFPv3D16 flag without firstly checking for the VFPv3 flag, and is checking the NEON flag which is not relevant.

With recent changes from bug 991153, invalid code is generated for device that do not support 32 DP registers which causes a crash with SIGILL.
(Assignee)

Comment 1

4 years ago
Created attachment 8464504 [details] [diff] [review]
Correct the detection of the number of floating point registers.
Attachment #8464504 - Flags: review?(mrosenberg)
Comment on attachment 8464504 [details] [diff] [review]
Correct the detection of the number of floating point registers.

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

It looks like this will only affect devices without VFPv3 support?
Attachment #8464504 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 3

4 years ago
(In reply to Marty Rosenberg [:mjrosenb] from comment #2)
> Comment on attachment 8464504 [details] [diff] [review]
> Correct the detection of the number of floating point registers.
> 
> Review of attachment 8464504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like this will only affect devices without VFPv3 support?

Yes, that looks right. Neon support requires 32 DP.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=941a681dab9b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f56f74a7e562
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(Assignee)

Comment 6

4 years ago
Comment on attachment 8464504 [details] [diff] [review]
Correct the detection of the number of floating point registers.

Approval Request Comment
[Feature/regressing bug #]: Long standing issue, exposed by bug 991153 which landed on Aurora 33. The patched function is not used before this.

[User impact if declined]: Crashes, or bad code generation, on ARM devices without vfp3.

[Describe test coverage new/current, TBPL]: Local testing. TBPL try run.

[Risks and why]: Low risk. If the logic is wrong then the compiler will just use 16 rather than 32 DP registers, a performance matter.

[String/UUID change made/needed]: n/a
Attachment #8464504 - Flags: approval-mozilla-aurora?
status-firefox33: --- → affected
status-firefox34: --- → fixed
Attachment #8464504 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.