615875 - Extend the protection range for constant pools around ICs.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

Attachment: Extend the protection range for constant pools around ICs.

Since bug 614323 landed I've been seeing failures on sunspider/check-date-format-xparb.js, at least with --methodjit-only.

The attached patch increases the reserved space for ICs. It also adds a couple of assertions to check that the combined space taken up by the code and the constant pool does not exceed the reserved space. (In most cases, this won't actually cause any problems, but it has the potential to do so.)

Note that I didn't tune the reservedSpace value. It will probably need to be re-tuned for PICs anyway, and it probably won't make a huge performance difference as it's small compared to the constant pool dumping frequency.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

I'm going to have to read through the underlying constant pool code more thoroughly (in a few hours). Off the cuff I don't understand why we're accounting for the size of the constant pool in the reserved space. The point of the reserved space was to ensure that the constant pool was *not* dumped into a contiguous bit of inline cache code.

Comment 2

[Doug Turner (:dougt)]

jacob, do you think this should block shipping fennec?

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

This should be a beta 4 blocker, please. I think we should be okay for beta 3, but if we can, Jacob and I will talk tonight to figure out what I'm missing.

Comment 4

[Jacob Bramley [:jbramley]]

(In reply to comment #2)
> jacob, do you think this should block shipping fennec?

Yes, as Chris said. It's statistically unlikely to happen, but it can cause crashes nonetheless.

(In reply to comment #1)
> I'm going to have to read through the underlying constant pool code more
> thoroughly (in a few hours). Off the cuff I don't understand why we're
> accounting for the size of the constant pool in the reserved space. The point
> of the reserved space was to ensure that the constant pool was *not* dumped
> into a contiguous bit of inline cache code.

Good point. New constants could end up out of range of the earliest LDRs, but this shouldn't matter as new LDRs using them will still be in range. However, I was seeing the pool dumped in the middle of SetGlobalName, and the length-check assertion hadn't fired on any other case. The generated instruction sequence may have differed in places, I suppose, and it's possible that we saw the pool dumped here out of chance.

The flushIfNoSpaceFor methods appear relatively sophisticated, and they shouldn't flush just because the earliest LDR can't reach the latest constant, but I'll review them to check that they do this properly. If the check is actually that simple, then it would explain why we saw the constant pool trouble.

Comment 5

[Jacob Bramley [:jbramley]]

The assertion trips in setgname where stubcc.rejoin is called. This in
turn asks for a label on the in-line masm, and asking for a label can
dump the pool. (Chris noted this in bug 614434.)

The actual size of the code sequence is within reservedSpace, but
generating the label causes things to go awry. A quick-fix for this is
simply to increase reservedSpace. A better fix would be to confirm that
'label()' doesn't need to reserve space, and stop it doing so. It
doesn't generate code, and a label has no (relevant) implications about
the code after it, so it seems pointless to me. Ok, it's probably useful
for performance if it's a branch target as it allows the branch to jump
directly over the constant pool, but we use labels for many other
purposes in Jaeger Monkey.

Semantically I think we should be using the ensureSpace variant that
takes a constant pool size too, but I can't see how it would actually
make a difference, so let's stick with the existing solution.

Comment 6

[Jacob Bramley [:jbramley]]

Attachment: Version 2: Assert that the pool is not flushed in critical regions, and slightly increase reservedSpace for jsop_setgname in xparb.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 494677 [details] [diff] [review]
Version 2: Assert that the pool is not flushed in critical regions, and slightly increase reservedSpace for jsop_setgname in xparb.

Good idea!

Comment 8

[Jacob Bramley [:jbramley]]

http://hg.mozilla.org/tracemonkey/rev/16a2d28ad4b4

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/16a2d28ad4b4