495396 - TM: missing early return in Nativei386 when generating LEA

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andreas Gal :gal]

Jesse is the hero of the day for unearthing this.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Bad code generation bug. I don't know how we survived this without noticing.

Comment 3

[Mike Shaver (:shaver emeritus)]

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?)

Comment 4

[Edwin Smith]

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.

Comment 5

[Andreas Gal :gal]

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.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/22363f125188

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/22363f125188

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cd581b2585bc