Summary: | Add SetCallee as DFG-Operation | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dominik Inführ <dominik.infuehr> | ||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cgarcia, commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, rmorisset, saam, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Dominik Inführ
2018-04-13 03:10:03 PDT
Created attachment 337878 [details]
Patch
Created attachment 337879 [details]
Patch
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.
Created attachment 337883 [details]
Patch
I know Robin was also working on exactly this bug 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 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. 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 (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) Created attachment 338321 [details]
Patch
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. 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. Created attachment 338440 [details]
Patch
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. R=me, based on Robin’s approval. 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? (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. 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); *** Bug 180044 has been marked as a duplicate of this bug. *** 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 on attachment 338440 [details] Patch Clearing flags on attachment: 338440 Committed r231195: <https://trac.webkit.org/changeset/231195> All reviewed patches have been landed. Closing bug. |