Bug 196712 – Modify how we do SetArgument when we inline varargs calls

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Filip Pizlo]

wait, GetStack has children?

[Filip Pizlo]

Also... this smells like a safeToExecute situation.

[Saam Barati]

        for (unsigned i = data->limit - 1; i--;)
=>
        for (unsigned i = data->limit; i--;)

[Saam Barati]

<rdar://problem/49605012>

[Saam Barati]

(In reply to Saam Barati from comment #3)
>         for (unsigned i = data->limit - 1; i--;)
> =>
>         for (unsigned i = data->limit; i--;)

Interestingly this isn't the fix. But it also looks wrong.

[Saam Barati]

Created attachment 367017 [details]
WIP

Currently fails a bunch of tests

[Saam Barati]

Created attachment 367028 [details]
WIP

test on EWS

[EWS Watchlist]

Comment on attachment 367028 [details]
WIP

Attachment 367028 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/11815225

New failing tests:
stress/tailCallForwardArguments.js.dfg-eager-no-cjit-validate
stress/tailCallForwardArguments.js.ftl-eager
stress/tailCallForwardArguments.js.ftl-eager-no-cjit-b3o1
v8-v6/v8-raytrace.js.ftl-eager

[Saam Barati]

Created attachment 367034 [details]
WIP

See if this works on EWS. I'm thinking perhaps we really want to keep these SetArgument nodes and just mark the ones from mandatoryMinimum to limit to not be lowered to GetStack in SSA conversion.

[EWS Watchlist]

Attachment 367034 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1861:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:412:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4347:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4349:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4358:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4360:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1870:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1872:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 8 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

(In reply to Saam Barati from comment #3)
>         for (unsigned i = data->limit - 1; i--;)
> =>
>         for (unsigned i = data->limit; i--;)

FWIW, this isn't an issue. We're doing the right thing here.

The bug is actually in clobberize, we claim to write an extra stack slot.

[Saam Barati]

Created attachment 367260 [details]
patch

[Michael Saboff]

Comment on attachment 367260 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:15
> +        However, we used to always to emit SetArgumentDefinitely up to "limit - 1" for

Awkward wording.  How about "However, we used to always emit ..."

> Source/JavaScriptCore/ChangeLog:16
> +        all are arguments, even when some arguments aren't guaranteed to be in a valid

Delete *are*?

[Saam Barati]

Created attachment 367484 [details]
patch for landing

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 367484 [details]
patch for landing

Clearing flags on attachment: 367484

Committed r244324: <https://trac.webkit.org/changeset/244324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.