IonMonkey: Correctly extract JSFunction from JSObject for calls.




JavaScript Engine
6 years ago
6 years ago


(Reporter: sstangl, Unassigned)


Firefox Tracking Flags

(Not tracked)



(2 attachments)



6 years ago
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.
Attachment #565029 - Flags: review?(dvander)
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 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 :)
Attachment #565029 - Flags: review?(dvander) → review+

Comment 3

6 years ago
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.
Attachment #565068 - Flags: review?(dvander)

Comment 4

6 years ago

With the patch still up for review applied, we pass all jit-tests on x64 --ion-eager.
Attachment #565068 - Flags: review?(dvander) → review+

Comment 5

6 years ago
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.