Extend the protection range for constant pools around ICs.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: jbramley, Assigned: jbramley)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 494383 [details] [diff] [review]
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.
Attachment #494383 - Flags: review?(cdleary)
(Assignee)

Updated

7 years ago
Blocks: 614323
(Assignee)

Updated

7 years ago
tracking-fennec: --- → ?
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

7 years ago
jacob, do you think this should block shipping fennec?
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.

Updated

7 years ago
tracking-fennec: ? → 2.0+
(Assignee)

Comment 4

7 years ago
(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.
tracking-fennec: 2.0+ → ?
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 6

7 years ago
Created 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.
Attachment #494383 - Attachment is obsolete: true
Attachment #494677 - Flags: review?(cdleary)
Attachment #494383 - Flags: review?(cdleary)
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!
Attachment #494677 - Flags: review?(cdleary) → review+
(Assignee)

Comment 8

7 years ago
http://hg.mozilla.org/tracemonkey/rev/16a2d28ad4b4
Whiteboard: fixed-in-tracemonkey

Comment 9

7 years ago
http://hg.mozilla.org/mozilla-central/rev/16a2d28ad4b4
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.