Closed Bug 1019414 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: dougc, Assigned: dougc)

References

Details

(Whiteboard: [qa-] )

Attachments

(1 file)

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.
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+
(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
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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-
You need to log in before you can comment on or make changes to this bug.