Fix for bug 590083 causes ARM crashes

RESOLVED INCOMPLETE

Status

()

Core
JavaScript Engine
--
critical
RESOLVED INCOMPLETE
8 years ago
3 years ago

People

(Reporter: Robert Sayre, Unassigned)

Tracking

({crash})

Trunk
ARM
Android
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -, fennec-)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
We can fix this by disabling ARM JIT for the moment, but we do have to fix this.
(Reporter)

Updated

8 years ago
Blocks: 590083
(Reporter)

Updated

8 years ago
blocking2.0: --- → betaN+

Updated

8 years ago
tracking-fennec: --- → 2.0b1+

Updated

8 years ago
tracking-fennec: 2.0b1+ → ---

Updated

8 years ago
tracking-fennec: --- → ?

Updated

8 years ago
tracking-fennec: ? → 2.0b1+
(Reporter)

Comment 1

8 years ago
I disabled the ARM method jit for the moment

http://hg.mozilla.org/tracemonkey/rev/3a6f15524d13
(Reporter)

Updated

8 years ago
Duplicate of this bug: 595584
(Reporter)

Updated

8 years ago
Duplicate of this bug: 595584
The assertion failures on ARM look remarkably similar to the failure in bug 595470, so this may be the same issue.
Duplicate of this bug: 595470
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.)
Created attachment 474700 [details] [diff] [review]
A trivial (but not ideal) fix.
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.
(Reporter)

Comment 9

8 years ago
Can we land the trivial fix to get ARM working again?
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.
Attachment #474700 - Flags: review+
Blocks: 596076
Keywords: crash

Updated

8 years ago
Severity: normal → critical

Comment 12

8 years ago
blocking 2.0 final for real fix
tracking-fennec: 2.0b1+ → 2.0+
(In reply to comment #12)
> blocking 2.0 final for real fix

I don't think it needs to block 2.0.
OS: Mac OS X → All
Hardware: x86 → ARM
(Reporter)

Updated

8 years ago
blocking2.0: betaN+ → -
tracking-fennec: 2.0+ → 2.0-
robert, is there anything left to do here?

Updated

7 years ago
OS: All → Android
(Assignee)

Updated

4 years ago
Assignee: general → nobody
Pertains to obsolete compilers (TM, JM).
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.