Closed Bug 719346 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Unassigned)

Details

Attachments

(2 files)

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.
Attached file Simpler test case.
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+
hg.mozilla.org/projects/ionmonkey/rev/a6061672ef3c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.