1554933 - ARM64: Add a SIGILL handler to dump the illegal instruction with a MOZ_CRASH_UNSAFE

Status: NEW

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Nicolas B. Pierron [:nbp] {away}]

Bug 1551205 and Bug 1550525 have a lot of SIGILL crashes, today we have no simple way of knowing whether the instruction is in code mapped as executable by the JIT, that we generated or something else.

The idea would be to replace these SIGILL by a MOZ_CRASH_UNSAFE(…), which would ouput something like: "In Ion (constant pool): mov x1, x1".

Due to Fennec being stuck on firefox 68, we should backport these patches to firefox 68, and maybe guard the code with FENNEC_NIGHTLY. Note, this mechanism might as well be useful on non-ARM64 backend, as a way to categorize MaybeEnterJit crashes based on the code which was used to generate the code.

Comment 1

[Jan de Mooij [:jandem]]

Interesting idea! However it sounds like it requires a fair amount of new instrumentation and it's already relatively easy to figure out the code around the pc when you have crash dump access...

Comment 2

[BugBot [:suhaib / :marco]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 3

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Jan de Mooij [:jandem] from comment #1)

Interesting idea! However it sounds like it requires a fair amount of new instrumentation and it's already relatively easy to figure out the code around the pc when you have crash dump access...

The idea being to easily classify these reports based on what we see in crash-stat, without having to look in the mini-dump. VIXL comes with a disassembler, which is currently being compiled.

Adding the Ion/IonCacheIR/Baseline/BaselineCacheIR/WASM would definitely be useful information, and it might also with newly introduced issues.

Comment 4

[Ted Campbell [:tcampbell]]

If we already have the vixl disassembler available then targeting just the one instruction at the crashing pc seems like a useful thing to show in a MOZ_CRASH_UNSAFE and seems like pretty low risk.

Since this is a newly expanded backend and we are trying to get it out the door to users, I think what you've proposed here makes sense. Does the SIGILL have the PC? It would be nice to minimize the amount of invasive instrumentation and if we could just get away with a membership test in the executableallocator that would be nice.

Comment 5

[Kannan Vijayan [:djvj]]

Wrong bug. Deleted comment.