IonMonkey: The exitCodePatch offset needs to be converted to it's final offset after assembly.

RESOLVED FIXED in Firefox 31

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

unspecified
mozilla32
ARM
All
Points:
---

Firefox Tracking Flags

(firefox30 wontfix, firefox31 fixed, firefox32 fixed)

Details

(Whiteboard: [qa-] )

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
The ARM backend requires offsets obtain during assembly to be converted to their final offset after assembly by calling actualOffset(). The exitCodePatch offset was not being converted to it's final location. It is surprising that this had not been causing failures in testing, but it does causes failures if the assembler buffer pool size is lowered.
(Assignee)

Comment 1

4 years ago
Created attachment 8433292 [details] [diff] [review]
The exitCodePatch offset needs to be converted to it's final offset after assembly.

Interestingly, the CodeLocationLabel constructor calls repoint() which calls actualOffset(), but repoint() only calls actualOffset() when passed a masm. In the patched code, actualOffset() does not get called. It would be nice to rework the code to be more intuitive, but I suggest landing this small fix now.
Assignee: nobody → dtc-moz
Attachment #8433292 - Flags: review?(jdemooij)
Comment on attachment 8433292 [details] [diff] [review]
The exitCodePatch offset needs to be converted to it's final offset after assembly.

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

Yeah this should be more intuitive, but thanks for fixing this one.

With your patch to check this stuff, will we also assert if we call actualOffset twice? That'd be good :)
Attachment #8433292 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 4

4 years ago
(In reply to Jan de Mooij [:jandem] from comment #3)
> With your patch to check this stuff, will we also assert if we call
> actualOffset twice? That'd be good :)

Yes, actualOffset will assert if passed an 'actual' offset. On the ARM all actual offsets are word aligned, so simply tagging a low bit on 'offset IDs' allows them to be distinguished.
Keywords: checkin-needed
(Assignee)

Comment 6

4 years ago
Comment on attachment 8433292 [details] [diff] [review]
The exitCodePatch offset needs to be converted to it's final offset after assembly.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is a long standing bug. The last bug to touch this was bug 750925.

User impact if declined: Crashes, although they seem rare. Probably hard to exploit.

Testing completed (on m-c, etc.): Just locally. It fixes failures in extended testing.

Risk to taking this patch (and alternatives if risky): Low. A one line fix.

String or IDL/UUID changes made by this patch: n/a
Attachment #8433292 - Flags: approval-mozilla-beta?
Attachment #8433292 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1f31c3d28c63
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → fixed
Attachment #8433292 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8433292 [details] [diff] [review]
The exitCodePatch offset needs to be converted to it's final offset after assembly.

Long-standing bug, fixed, can ride the trains from FF31 rather than crash-land to FF30 with no bake time.
Attachment #8433292 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox30: affected → wontfix
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.