Last Comment Bug 692291 - IonMonkey: Correctly extract JSFunction from JSObject for calls.
: IonMonkey: Correctly extract JSFunction from JSObject for calls.
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: general
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 670484 677337
  Show dependency treegraph
Reported: 2011-10-05 15:00 PDT by Sean Stangl [:sstangl]
Modified: 2011-10-07 14:35 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix. (1.11 KB, patch)
2011-10-05 15:00 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review
Handle ION_DISABLED_SCRIPT in calls. (1.26 KB, patch)
2011-10-05 16:34 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description User image Sean Stangl [:sstangl] 2011-10-05 15:00:21 PDT
Created attachment 565029 [details] [diff] [review]

Calls are incorrect: they assume that testing a JSObject to be a JSFunction means that the JSObject can be used as a JSFunction, when the interpreter JSOP_CALL implementation actually fetches the JSFunction* from JSObject.privateData. So now we do that.

(Incidentally, privateData is pretty shady. Could we better express it using a union?)

This fixes crashes in 14 jit-tests on x64, --ion-eager. With this patch applied, we're down to 27.
Comment 1 User image Brian Hackett (:bhackett) 2011-10-05 15:09:03 PDT
privateData is an API thing, it is private to the JS class and different classes use it in different ways.  There are some well known uses of privateData like for FunctionClass, but in general it can hold any pointer-sized value.
Comment 2 User image David Anderson [:dvander] 2011-10-05 15:48:55 PDT
Comment on attachment 565029 [details] [diff] [review]

Review of attachment 565029 [details] [diff] [review]:

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +707,5 @@
>          return false;
> +    // Extract the function object.
> +    // TODO: Apparently this is not always required. Can TI help?
> +    masm.movePtr(Operand(objreg, offsetof(JSObject, privateData)), objreg);

For a generic call, we can suck this up - for a direct, monomorphic call, we shouldn't have to do it. So I'd lose the TODO so vim doesn't hang Christmas lights around it :)
Comment 3 User image Sean Stangl [:sstangl] 2011-10-05 16:34:16 PDT
Created attachment 565068 [details] [diff] [review]
Handle ION_DISABLED_SCRIPT in calls.

Second small patch to calls handles ION_DISABLED_SCRIPT -- the JSScript.ion is occasionally 0x1, designating a former compilation failure. Thus the NULL check is incorrect.

With this patch applied, we now have 2 jit-test failures, one probably from StackIter, one from SunSpider.
Comment 4 User image Sean Stangl [:sstangl] 2011-10-06 17:15:01 PDT

With the patch still up for review applied, we pass all jit-tests on x64 --ion-eager.
Comment 5 User image Sean Stangl [:sstangl] 2011-10-07 14:35:50 PDT

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