Last Comment Bug 719346 - IonMonkey: ICs add non-IonCode jump targets to jumps_ list.
: IonMonkey: ICs add non-IonCode jump targets to jumps_ list.
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: ---
Assigned To: general
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-01-19 00:03 PST by Sean Stangl [:sstangl]
Modified: 2012-01-23 16:32 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Simpler test case. (194 bytes, application/javascript)
2012-01-20 17:05 PST, Sean Stangl [:sstangl]
no flags Details
Fix: Don't append to reloc table from ICs. (11.11 KB, patch)
2012-01-23 13:55 PST, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description User image Sean Stangl [:sstangl] 2012-01-19 00:03:43 PST
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 User image Brian Hackett (:bhackett) 2012-01-19 07:16:19 PST
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 User image Sean Stangl [:sstangl] 2012-01-20 17:05:17 PST
Created attachment 590395 [details]
Simpler test case.
Comment 3 User image Sean Stangl [:sstangl] 2012-01-23 13:55:01 PST
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.
Comment 4 User image David Anderson [:dvander] 2012-01-23 16:16:36 PST
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 User image Sean Stangl [:sstangl] 2012-01-23 16:32:10 PST

Note You need to log in before you can comment on or make changes to this bug.