ARM: making labels can dump the constant pool!

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
8 years ago
8 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

unspecified
ARM
All
Points:
---

Firefox Tracking Flags

(fennec2.0+)

Details

This is pretty much the death of repatching, unless you reserve all the space up front -- there may be some latent bugs in tip due to this. jbramley, any particular reason it has to be like this?
Latent bugs could be, for example, in call ICs -- we either have a) make it not this way or b) audit the existing code and remember for code that goes in, of which (a) seem the most reasonable. Nominating for blocking Fennec final.
tracking-fennec: --- → ?
I don't know why a label has to reserve space. A label generates no code so it doesn't make sense to dump the constant pool here. However, if generating a label dumps the constant pool, generating any code after the label will surely do the same. The ARMAssembler::label() method calls AssemblerBufferWithConstantPool::size(), which in turn calls (after expansions) 'flushIfNoSpaceFor(4,8)'. That ensures that there's space for an 8-byte constant and a 4-byte instruction. If we don't have space for that when we make the label, we're probably going to dump it as soon as we generate code anyway, so this check should be harmless. At a guess, I'd say that the label does this check to avoid having a jump to a label which immediately jumps over a literal pool. It'd be more efficient to branch directly over the literal pool.

We were going to call ensureSpace before each PIC, weren't we? Doesn't that solve it anyway?

I think we should add some debug wrappers around offset-sensitive code (such as PICs) to assert that the constant pool is not flushed. This change shouldn't be huge, and will make constant-pool-related-offset-bugs much easier to track down.

----

Incidentally, if all else fails, we can stop using a constant pool altogether by using MOVW/MOVT, or simple arithmetic instruction for pre-ARMv7 devices. The Thumb-2 back-end already does this (using MOVW/MOVT). Szeged did some experimentation in this area: http://webkit.sed.hu/blog/20090518/technical-discussion-part-2-constructing-constants

This makes patching more awkward in a way because the sequences become variable-length, but greatly simplifies some other things. We can mitigate the variable-length problem on ARMv7 by using a MOVW/MOVT pair, but older architectures need four arithmetic instructions to generate an arbitrary 32-bit constant.

On the other hand, other RISC-like architectures will probably need constant pools too, so we might just be postponing the problem by doing this.

Comment 3

8 years ago
why should this block fennec from shipping?
(In reply to comment #3)
> why should this block fennec from shipping?

It can cause random crashers with methodjit code repatching. It's a "final" blocker.

Bug 614323 already did it for mono ICs and we'll do it for PICs in bug 588021, but the back end needs to be audited to make sure it does not assume it can place two labels in a row and they'll be equivalent. (Since the second label can dump a constant pool immediately before it.)

Updated

8 years ago
tracking-fennec: ? → 2.0+
We audited the ICs very well in 588021 -- makes me think we can close this bug out -- jbramley, what do you think?
Yep, we can close this. We need to be aware of it, but as long as we remember to use AutoReserveICSpace in new PICs, we'll be safe.

I've marked it as WONTFIX because labels can still dump the constant pool in other regions.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.