Closed Bug 916755 Opened 6 years ago Closed 6 years ago

visitClampVToUint8 creates unused oolTruncateDouble code and does not bind a return label.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: dougc, Assigned: dougc)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Tracked down some crashes on the ARM to debug assertion failures related
to the return branch from the OOL ToInt32 call generated in this path.

It appears that visitClampVToUint8 does not bind the rejoin label.

Further, the OOL code appears to be unused, and could perhaps be removed.
Attachment #805269 - Flags: review?(jdemooij)
Comment on attachment 805269 [details] [diff] [review]
Remove the unused ool code.

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

Thanks Douglas, Shu wrote the code though so forwarding to him :)
Attachment #805269 - Flags: review?(jdemooij) → review?(shu)
Blocks: 914899
Comment on attachment 805269 [details] [diff] [review]
Remove the unused ool code.

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

It shouldn't be removed. The path is used: see convertValueToInt, which clampValueToUint8 uses.

Good catch though, all that's needed is a |masm.bind(oolDouble->rejoin())| after the call to |masm.clampValueToUint8|.
Attachment #805269 - Flags: review?(shu)
Comment on attachment 805269 [details] [diff] [review]
Remove the unused ool code.

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

I was mistaken, and dougc is correct: the OOL path is never taken for the clamping case, only for normal truncation.
Attachment #805269 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b56a5d69f342
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 805269 [details] [diff] [review]
Remove the unused ool code.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): caused jit-test failures
User impact if declined: crashes on the Ion ARM compiler backend
Testing completed (on m-c, etc.): fixed the failing jit-tests
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch:
Attachment #805269 - Flags: approval-mozilla-aurora?
Attachment #805269 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
Assignee: general → dtc-moz
You need to log in before you can comment on or make changes to this bug.