494343 - nanojit: bad assertion in LirBufWriter::ensureRoom()

Status: RESOLVED DUPLICATE of bug 494639

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch fixing assertion

Comment 8 of bug 494084 has the details.  (Sorry, I should have just started this bug rather than writing that comment in that old bug.)  Attached patch is a fix.

Comment 1

[Graydon Hoare :graydon]

Comment on attachment 379054 [details] [diff] [review]
patch fixing assertion

Obvious thinko, thanks.

Comment 2

[Graydon Hoare :graydon]

Comment on attachment 379054 [details] [diff] [review]
patch fixing assertion

Hah. As proposed, this doesn't actually work either because of the rounding up to the next LIns boundary performed in insSkip. MAX_SKIP_BYTES is 3996, insSkip rounds up to 4000 bytes (250 slots), adds 1, passes 251 to ensureRoom, which then multiplies that out to 4016 bytes required. But the limit you propose is only 4012 bytes. So it still traps.

Hilarity.

Comment 3

[Graydon Hoare :graydon]

Attachment: another stab at it

Comment 4

[Graydon Hoare :graydon]

Or .. perhaps 2*sizeof(LIns), since we don't want to land on the page-edge? God, what a mess.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 379208 [details] [diff] [review]
another stab at it

What's wrong with this sentence?

  MAX_SKIP_BYTES is 3996, insSkip rounds up to 4000 bytes (250 slots)

Er... so we round the size up to something greater than MAX_SKIP_BYTES!  That's
bad.  I think we avoid this problem if we ensure MAX_SKIP_BYTES is a multiple
of sizeof(LIns), eg. 4000;  this could be tested via an assertion in
insSkip().  I'll work up an alternative patch on Monday (it's Saturday
morning here for me now).

I consider this mistake another argument in favour of doing our accounting
entirely in bytes (bug 494357) -- I think the problem would have been easier
to see that way.  And maybe bug 494358 will help when I get to that.

Comment 6

[Nicholas Nethercote [inactive]]

I failed to say -- if MAX_SKIP_BYTES is a multiple of sizeof(LIns), I think my original patch is correct.  But I'll recheck that on Monday.

Comment 7

[Nicholas Nethercote [inactive]]

This will be easier to fix alongside various other changes, as part of bug 494639.  If we need a temporary workaround, we could just remove the assertion.

Comment 8

[Graydon Hoare :graydon]

Actually I removed this assert with the lirasm patch in bug 484142, so I think this is no longer relevant.

(Also I'll be landing your bug 494639 work later today, as a final nail in this particular coffin).

Comment 9

[Nicholas Nethercote [inactive]]

I agree it's no longer relevant.  I'll mark it as a dup of bug 494639, since that also (would have) fixed it and is more related to this than bug 484142.