Bug 182368

Summary: When BytecodeParser inserts Unreachable after ForceOSRExit it needs to update ArgumentPositions for Flushes it inserts
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, 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

Description Saam Barati 2018-01-31 18:20:15 PST 
...
Comment 1 Saam Barati 2018-01-31 18:43:27 PST 
<rdar://problem/36932466>
Comment 2 Saam Barati 2018-01-31 18:48:20 PST 
Created attachment 332833 [details]
patch
Comment 3 Saam Barati 2018-01-31 18:48:41 PST 
Created attachment 332835 [details]
patch
Comment 4 Saam Barati 2018-01-31 19:06:46 PST 
Created attachment 332839 [details]
patch
Comment 5 Saam Barati 2018-02-01 11:22:29 PST 
Created attachment 332895 [details]
patch
Comment 6 Mark Lam 2018-02-02 13:45:11 PST 
Comment on attachment 332895 [details]
patch

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

r=me with a suggestion.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6679
> +                        VirtualRegister argument = operand - (inlineCallFrame ? inlineCallFrame->stackOffset : 0);
> +                        if (argument.isArgument() && !argument.isHeader()) {
> +                            const Vector<ArgumentPosition*>& arguments = m_inlineCallFrameToArgumentPositions.get(inlineCallFrame);
> +                            arguments[argument.toArgument()]->addVariable(variable);
> +                        }

If this is only needed when we create a new VAD, maybe you can move this into the body of the if statement above.
Comment 7 Saam Barati 2018-02-02 13:56:44 PST 
Comment on attachment 332895 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6679
>> +                        }
> 
> If this is only needed when we create a new VAD, maybe you can move this into the body of the if statement above.

Spoke to Mark offline about this:
We want to tell this ArgumentPosition about this VAD regardless of whether we added it or not. From reading other code in the bytecode parser, I believe it's possible that this VAD is from GetLocal only, and the ArgumentPosition would not know about it.
Comment 8 WebKit Commit Bot 2018-02-02 14:55:33 PST 
Comment on attachment 332895 [details]
patch

Clearing flags on attachment: 332895

Committed r228031: <https://trac.webkit.org/changeset/228031>
Comment 9 WebKit Commit Bot 2018-02-02 14:55:35 PST 
All reviewed patches have been landed.  Closing bug.