595336 - Fix for bug 590083 causes ARM crashes

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

We can fix this by disabling ARM JIT for the moment, but we do have to fix this.

Comment 1

[Robert Sayre]

I disabled the ARM method jit for the moment

http://hg.mozilla.org/tracemonkey/rev/3a6f15524d13

Comment 4

[Jacob Bramley [:jbramley]]

The assertion failures on ARM look remarkably similar to the failure in bug 595470, so this may be the same issue.

Comment 6

[Jacob Bramley [:jbramley]]

Increasing the size of the offset fields in CallICInfo fixes this crash. Is 8 bits really not enough to describe the offset? Clearly it's not. MICs weren't that big when I last looked. 8 bits gives enough space for 64 ARM instructions.

I increased these fields to 16 bits each to pass the tests, but I didn't check the optimal size. It's worth mentioning that on ARM, the offsets will always be 4-byte-aligned, so if we wrap the assignment (and check) in a macro or inline function, we should be able to save 2 bits on each field.

Is it really worth storing offsets like this, rather than using CodeLocationLabel objects? Yes, it makes the structure smaller, but is it a worthwhile optimization? There surely aren't hundreds of these kicking around, and if there are, many will be identical anyway, so we could alias them from within IC records. (This code still feels fragile to me.)

Comment 7

[Jacob Bramley [:jbramley]]

Attachment: A trivial (but not ideal) fix.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

There is one of these for every callsite, so hundreds is probably a good estimate. Even with a hundred thousand though, a few extra bytes won't matter. If CodeLocationLabel makes things much easier, we can do that.

In the early JM days, large offsets have pointed to bugs or inefficiencies in FrameState. But those are all gone now.

Comment 9

[Robert Sayre]

Can we land the trivial fix to get ARM working again?

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 474700 [details] [diff] [review]
A trivial (but not ideal) fix.

r=me on this fix but we should have a follow-up, to either minimize the fields or use CodeLocationLabel.

Comment 11

[Robert Sayre]

I landed this

http://hg.mozilla.org/tracemonkey/rev/5899a40b6d6d

Comment 12

[Stuart Parmenter]

blocking 2.0 final for real fix

Comment 13

[Jacob Bramley [:jbramley]]

(In reply to comment #12)
> blocking 2.0 final for real fix

I don't think it needs to block 2.0.

Comment 14

[Doug Turner (:dougt)]

robert, is there anything left to do here?

Comment 15

[Lars T Hansen [:lth]]

Pertains to obsolete compilers (TM, JM).