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.
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.
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.
(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.
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.
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.
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!