516107 - TM: partial merge causes bad ARM codegen [nanojit]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

We ended up taking only part of this patch, breaking code generation on arm.

http://hg.mozilla.org/tamarin-redux/diff/8f05ae1c7a08/nanojit/NativeARM.cpp

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixed merge

Problem (we think) is that mSlot was being pre-incremented here, which was the case with the previous semantics, whereas now it should be post incremented.  We only call underrunProtect(8), but we actually wrote 12 bytes -- 4 bytes for a slot that we just skipped over, 4 bytes to write the slot itself, and then 4 bytes for the instruction.

There may be other theories of what went wrong here, but that seems to be the most likely one.

Comment 3

[Andreas Gal :gal]

Comment on attachment 400222 [details] [diff] [review]
fixed merge

Matches adobe's code now.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/17dddefd1501
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b1b11642a45
http://hg.mozilla.org/tracemonkey/rev/28a3eef2aed6

Comment 5

[Graydon Hoare :graydon]

I'm very glad you managed to work out the right way to do this, if it is now working! At the time I found that "taking Adobe's version verbatim" was causing it to crash, presumably due to conflicts with other bits of not-yet-resolved ARM or Assembler assumptions I was unable to track down. So I tried to get something that "wasn't as crashy" by adjusting with the assumed boundary condition. As Jacob pointed out, what I wound up committing didn't really make a lot of sense, and was likely working by accident. It just happened to crash less :(

If that's all fixed now, hurrah!