Bug 184582

Summary: Add SetCallee as DFG-Operation
Product: WebKit Reporter: Dominik Inführ <dominik.infuehr>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, rmorisset, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dominik Inführ 2018-04-13 03:10:03 PDT
Add SetCallee as DFG-Operation
Comment 1 Dominik Inführ 2018-04-13 04:34:04 PDT
Created attachment 337878 [details]
Patch
Comment 2 Dominik Inführ 2018-04-13 04:40:19 PDT
Created attachment 337879 [details]
Patch
Comment 3 EWS Watchlist 2018-04-13 04:43:07 PDT
Attachment 337879 [details] did not pass style-queue:


ERROR: JSTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Dominik Inführ 2018-04-13 06:10:18 PDT
Created attachment 337883 [details]
Patch
Comment 5 Saam Barati 2018-04-13 08:15:05 PDT
I know Robin was also working on exactly this bug
Comment 6 Saam Barati 2018-04-13 08:21:25 PDT
Comment on attachment 337883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337883&action=review

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2455
> +    case SetCallee:

Do we not model Callee is a variable?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1399
> +            addToGraph(SetCallee, OpInfo(bitwise_cast<intptr_t>(function)));

This doesn’t look completely right. You need to always do this if you’re looping back to the machine call frame (regardless of the variant being a constant value). Also, you need to do this anytime you loop back to an inline frame that has its callee in a stack slot.
Comment 7 Saam Barati 2018-04-13 08:22:23 PDT
Comment on attachment 337883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337883&action=review

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2455
>> +    case SetCallee:
> 
> Do we not model Callee is a variable?

Ignore this. I see that we don’t for the machine frame. My guess is we do for inline frames.
Comment 8 Dominik Inführ 2018-04-13 09:30:55 PDT
I guess it should look like this?

addToGraph(SetCallee, OpInfo(bitwise_cast<intptr_t>(callVariant.function())));

if (stackEntry->m_inlineCallFrame)
    // update callee stack slot for inline frame
Comment 9 Saam Barati 2018-04-13 10:42:24 PDT
(In reply to Dominik Inführ from comment #8)
> I guess it should look like this?
> 
> addToGraph(SetCallee,
> OpInfo(bitwise_cast<intptr_t>(callVariant.function())));
> 
> if (stackEntry->m_inlineCallFrame)
>     // update callee stack slot for inline frame

Why is your call variant guaranteed to be a constant callee? That should the the case. What you want to do is express it in a normal data flow edge in the IR

Like:
a: Foo
b: SetCallee(SomeUse:@a)
Comment 10 Dominik Inführ 2018-04-19 06:50:11 PDT
Created attachment 338321 [details]
Patch
Comment 11 Dominik Inführ 2018-04-19 07:00:13 PDT
I've now updated the patch. The callee is now set both for the inline call frame and the machine call frame using the callTargetNode.

I've also added a check for the case when an inline call frame isn't a closure, in this case the callee is a constant and therefore can't be set/changed.
Comment 12 Robin Morisset 2018-04-20 07:47:06 PDT
I am not an official reviewer, so please wait for a review by someone more senior, but it looks good to me.

While reviewing your patch, I realized that the following lines:
```
// Currently we cannot do this optimisation for closures. The problem is that "emitFunctionChecks" which we use later is too coarse, only checking the executable
    // and not the value of captured variables.
    if (callVariant.isClosureCall())
        return false;
```
were wrong (not a source of crashes, but they disable the optimization in many cases for no good reason). The case I was trying to prevent was precisely the case where the callee needs to be changed (and I got it wrong, isClosureCall() does not do what I thought it would do).
You may either remove them in this patch, or I will make a separate patch that removes them.
Comment 13 Dominik Inführ 2018-04-20 11:05:14 PDT
Created attachment 338440 [details]
Patch
Comment 14 Dominik Inführ 2018-04-20 11:09:59 PDT
Thanks for looking into the patch! I was actually wondering about that code but then forgot it... I've now removed the check you mentioned, hope this is okay for you.
Comment 15 Filip Pizlo 2018-04-20 11:13:10 PDT
R=me, based on Robin’s approval.
Comment 16 Saam Barati 2018-04-20 14:59:52 PDT
Comment on attachment 338440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338440&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1402
> +                setDirect(stackEntry->remapOperand(VirtualRegister(CallFrameSlot::callee)), callTargetNode, NormalSet);

Why setDirect here? Why not also emit that for the machine frame case instead of adding a SetCallee node?
Or is the SetCallee node needed for some reason?
Comment 17 Saam Barati 2018-04-20 15:00:09 PDT
(In reply to Saam Barati from comment #16)
> Comment on attachment 338440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338440&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1402
> > +                setDirect(stackEntry->remapOperand(VirtualRegister(CallFrameSlot::callee)), callTargetNode, NormalSet);
> 
> Why setDirect here? Why not also emit that for the machine frame case
> instead of adding a SetCallee node?
> Or is the SetCallee node needed for some reason?

LGTM too in general. Just this one question.
Comment 18 Dominik Inführ 2018-04-23 00:51:04 PDT
I've added SetCallee based on the already exisiting node SetArgumentCountIncludingThis. We can't use SetDirect (at least in its current form) since it only handles setting arguments and locals. Setting the callee with setDirect, invokes setArgument which fails on the following assertion:

        unsigned argument = operand.toArgument();
        ASSERT(argument < m_numArguments);
Comment 19 Robin Morisset 2018-04-23 06:02:34 PDT
*** Bug 180044 has been marked as a duplicate of this bug. ***
Comment 20 Dominik Inführ 2018-05-01 02:33:58 PDT
Hi, is there anything left for me to do such that the patch can be merged? I think I've addressed Saam's question.
Comment 21 WebKit Commit Bot 2018-05-01 08:42:16 PDT
Comment on attachment 338440 [details]
Patch

Clearing flags on attachment: 338440

Committed r231195: <https://trac.webkit.org/changeset/231195>
Comment 22 WebKit Commit Bot 2018-05-01 08:42:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2018-05-01 08:43:17 PDT
<rdar://problem/39866759>