The default bug view has changed. See this FAQ.

IonMonkey: ICs add non-IonCode jump targets to jumps_ list.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
This is preventing x86_64 from running the SunSpider benchmark suite to completion. It can be reproduced by running SunSpider 1.0 with --ion -n.

IonCacheGetProperty::attachNative() calls branchPtrWithPatch(), which calls addPatchableJump(), causing the jumpsite to be added to the jumps_ list. On GC, each entry in the jumps_ list will have its jump target extracted for tracing purposes, and therefore each target must be the raw() buffer of an IonCode, since tracing logic subtracts sizeof(IonCode *) from the buffer to derive the IonCode GCThing.

Later in the same function, the jump is patched via PatchJump(exitJump, cacheLabel()). But by this point, the cacheLabel_ has been CodeLocationLabel::repoint()ed, the non-debug code to which is reproduced below:

> void repoint(IonCode *code) {
>   raw_ = code->raw() + size_t(raw_);
>   markAbsolute(true);
> }

Since |raw_| is added to the buffer we want, the jump target given by cacheLabel() is invalid for purposes of tracing, and we segfault.

The most likely solution is to not include IC-related jumps during tracing, since all IC stubs will be destroyed upon GC invocation. But I don't understand the repoint() logic -- is it possible to bake in (IonCode*)->raw() targets?
The cache label is the rejoin point for the code to continue execution after finishing the IC stub, so will necessarily be an internal pointer in the IonScript's IonCode.  The ICs will be purged during GC, so I think that just ignoring these jumps is the best thing to do.
(Reporter)

Comment 2

5 years ago
Created attachment 590395 [details]
Simpler test case.
(Reporter)

Comment 3

5 years ago
Created attachment 590860 [details] [diff] [review]
Fix: Don't append to reloc table from ICs.

Fixes segfault by not appending IC jump targets to jumpRelocations_ list. Renames CODE => IONCODE and EXTERNAL => HARDCODED for clarity. Explicitly shows that branchPtrWithPatch() refers to HARDCODED.

With this patch, x64 can successfully run the SunSpider test suite.
Attachment #590860 - Flags: review?(dvander)
Comment on attachment 590860 [details] [diff] [review]
Fix: Don't append to reloc table from ICs.

Review of attachment 590860 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.
Attachment #590860 - Flags: review?(dvander) → review+
(Reporter)

Comment 5

5 years ago
hg.mozilla.org/projects/ionmonkey/rev/a6061672ef3c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.