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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

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?

Comment 1

[Brian Hackett [Laid off!]]

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.

Comment 2

[Sean Stangl [:sstangl]]

Attachment: Simpler test case.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: 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.

Comment 4

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

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.

Comment 5

[Sean Stangl [:sstangl]]

hg.mozilla.org/projects/ionmonkey/rev/a6061672ef3c