Closed Bug 495396 Opened 12 years ago Closed 12 years ago

TM: missing early return in Nativei386 when generating LEA

Categories

(Core :: JavaScript Engine, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: gal, Assigned: gal)

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

Jesse is the hero of the day for unearthing this.
Attached patch patchSplinter Review
Assignee: general → gal
Attachment #380386 - Flags: review?(edwsmith)
Bad code generation bug. I don't know how we survived this without noticing.
Flags: blocking1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1
Attachment #380386 - Flags: review?(edwsmith) → review+
What is the actual effect of the bug?  Do we just end up generating worse code, or is the code incorrect? I share the question from comment 2!

(Related: what is the possible fallout of changing this codegen path right before release?)
I looked at this a bit more.  some notes:

* i'm convinced the return is correct and not having it was a bug.

but

* the LEA is only in the path of LIR_alloc (stack memory) + const
* falling through means the final code would be:

    <alu-op> rr, const
    lea rr, [const + EBP]

was the extra code generated modifying any other state, maybe the CC's?  I can't think of any other bad effects it could have had.

also might explain why Tamarin never saw it, we dont use LIR_ov and therefore dont depend on CC preservation between LIR instructions.

In a separate conversation with Julian S recently, the topic of cleaning up semantics around LIR_ov came up -- its too fragile the way it is now.
I completely agree. We have to model CCs. The currently approach inhibits optimization. I just added LIR_mod, which also depends on a nearby LIR_div, which also is bad style. Better solutions are definitively wanted.
http://hg.mozilla.org/tracemonkey/rev/22363f125188
Whiteboard: fixed-in-tracemonkey
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
http://hg.mozilla.org/mozilla-central/rev/22363f125188
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.