Make eval("arguments") work inside of lambdas. #1463

j-wing · 2016-08-18T07:01:41Z

This is fairly close to the solution @aneeshdk @ianwjhalliday and I discussed. There's one minor difference: in order for the new location of AddVarsToScope to be as clear as possible, we planned to move it above all of the paramScope code. Sadly, the call the pushScope(paramScope) needs to happen prior to AddVarsToScope to avoid an assertion failure in ByteCodeGenerator.cpp:1659 due to currentScope not being updated.

I couldn't come up with a less repetitive way to test this than to just copy/paste the bulk of the "arguments in lambdas" test, and replace the use of arguments with eval("arguments"). Definitely open to suggestions here.

msftclas · 2016-08-18T07:01:45Z

Hi @jordonwii, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Jordon Wing). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

pleath · 2016-08-18T17:48:57Z

lib/Runtime/ByteCode/ByteCodeGenerator.cpp

+
+                See Github issue #1209 for more.
+                */
+                AddVarsToScope(pnodeScope->sxFnc.pnodeVars, byteCodeGenerator);


These symbols have to be added to the body scope. Does that not mean that the call has to happen where it currently happens, after we've begun processing the body scope?

This works because AddVarsToScope explicitly adds vars to the current function's body scope. It uses ByteCodeGenerator::AddSymbolToFunctionScope, which in turn calls ByteCodeGenerator:AddSymbolToScope with scope = currentScope->GetFunc()->GetBodyScope().

The real problem, I think, is that the processing of the arguments symbol is still being handled by a method that's specific to the body scope. How about if we move that logic to the processing of the param scope?

Good question. I don't have a solid answer to that. If you're referring to the setting of argumentsSymbol inside of AddVarsToScope, my thinking would be that we set it there because that already has to do a pass over all of the variables in the body scope, and doing it elsewhere would mean doing two passes. In the more common case where we don't have a split scope, we don't do any param scope processing logic, so we can't only set it there.

@ianwjhalliday thoughts?

We can restructure things so that two passes are not necessary, i.e., make the arguments symbol available without searching the body scope vars to find it. Going forward, we're going to create real symbols for this, super, and new.target to staunch the trickle of special-casey bugs involving those constructs, so whatever we invent to allow easy processing of "arguments" in the param scope will work for those other symbols as well.

And indeed @aneeshdk and I discussed making the arguments symbol a normal symbol instead of a special case, to remove a lot of this extra work. After some code review and discussion we concluded that there is no reason to have a specially handled arguments symbol and it should just work if we make it a normal symbol (with some special care to ensure it is available regardless whether the user script declared it or not).

Out of scope for this PR of course.

I am in favor of jiggering this code however needed to make it work for this PR.

Then we should very soon go and make the arguments symbol not a special case.

j-wing · 2016-08-18T19:33:48Z

It turns out that in my first push of the test, I mistakenly included an eval call inside of the new test's body (in lambda1.js), and that buried some failures which I'm currently trying to figure out. In the debug builds, there's an assertion failure I don't understand yet, and in the test builds, arguments.callee gets set to assert.areEqual rather than the lambda's non-lambda parent.

j-wing · 2016-08-21T03:51:59Z

I can't seem to reproduce either the test failure or the assertion failure outside of running runtests.cmd. The failures only happen in the "dynapogo" variant, and if I copy/paste the command it reports that it's running and run ch.exe manually, there are no failures. Any ideas as to why this might happen?

Edit: The bug happens when the function containing a lambda with eval('arguments') has been JIT'd. I forgot that the test runner's run of the interpreted variant produces JIT profile data for the second run, so once I got it to try to inline the function I was able to reproduce the assertion failure in the debugger.

It looks like there's another "uses arguments" related flag the JIT'd function body (UsesArgumentsObject). It gets set in the ByteCodeGenerator when the function explicitly uses arguments (the UsesArguments flag, which we don't currently set in the case of eval in a lambda). Without that, there's an assertion that fails when GetInlineeArgvSlotOpnd is called from LowererMDArch::LoadHeapArguments. I'm still trying to figure out how this works in the normal eval() case.

dilijev · 2017-02-22T23:13:39Z

Contacted @jordonwii to follow up on this PR.

He is planning to continue work on this PR once he has the time. I'll keep in touch with him.

Conflicts: lib/Runtime/ByteCode/ByteCodeGenerator.cpp test/es6/default-splitscope.js

…e1209 Conflicts: lib/Runtime/ByteCode/ByteCodeGenerator.cpp test/es6/default-splitscope.js

j-wing · 2017-03-15T19:31:52Z

Progress, at last. :)

It looks like the errors I mentioned in my last comment were the result of the jit trying to inline the outer function, when it shouldn't have been able to because the inner Lambda used eval. The code added in these last few commits fixes that by marking the outer function arguments object as referenceable. I haven't figured out how to verify that this isn't over compensating somehow, but right now it appears to work.

If this looks good, I'll flatten the commits tonight and add a couple comments, especially to the tests.

dilijev · 2017-03-15T20:29:43Z

test/es6/lambda1.js

+    {
+        name: "eval('arguments') works correctly within lambdas",
+        body: function () {
+            var arguments = 'not arguments object';


To clarify this, should each instance of arguments in this line bind to the var called arguments or to an actual arguments object?

Both should refer to the variable ('not an arguments object'). The var arguments line shadows the default arguments object, so both the first param to areEqual and the arguments inside the lambda should refer to that.

dilijev · 2017-10-18T00:12:07Z

Hey @jordonwii - just checking in if you'd like to continue work on this PR

dilijev · 2017-11-21T03:45:28Z

@pleath: I contacted @jordonwii offline to discuss this PR and he said he was under the impression that this PR would be superceded by other refactoring work or another change. Is that the case? If so (regardless of whether that work is in progress or not), we should close this PR.

dilijev · 2017-12-19T00:24:56Z

Per offline discussion with @pleath I'm closing this PR.

msftclas added the cla-already-signed label Aug 18, 2016

j-wing force-pushed the issue1209 branch from 712c5d1 to 6eafcbc Compare August 18, 2016 15:22

Make eval("arguments") work inside of lambdas.

6a05885

j-wing force-pushed the issue1209 branch from 6eafcbc to 6a05885 Compare August 18, 2016 15:29

Make eval("arguments") work inside of lambdas.

c97ce8c

pleath reviewed Aug 18, 2016
View reviewed changes

dilijev self-assigned this Feb 22, 2017

dilijev added External Contributor Waiting labels Feb 23, 2017

j-wing added 5 commits March 12, 2017 13:40

Merge branch 'master' into issue1209

d8d3ed8

Conflicts: lib/Runtime/ByteCode/ByteCodeGenerator.cpp test/es6/default-splitscope.js

Merge branch 'issue1209' of github.com:jordonwii/ChakraCore into issu…

306331c

…e1209 Conflicts: lib/Runtime/ByteCode/ByteCodeGenerator.cpp test/es6/default-splitscope.js

Fix for assertion failures in JIT'd lambdas that eval(arguments)

7929b8a

Fix tabbing in last commit.

b5c7bab

Merge branch 'master' into issue1209

7028ae8

dilijev assigned pleath Mar 15, 2017

dilijev reviewed Mar 15, 2017

View reviewed changes

dilijev added this to the Backlog milestone May 18, 2017

msftgits removed the cla-already-signed label Sep 26, 2017

dilijev closed this Dec 19, 2017

snyk-bot mentioned this pull request Sep 16, 2021

[Snyk] Security upgrade yargs from 4.8.1 to 15.0.1 saurabharch/ChakraCore#5

Open

Make eval("arguments") work inside of lambdas. #1463

Make eval("arguments") work inside of lambdas. #1463

Uh oh!

Conversation

j-wing commented Aug 18, 2016

Uh oh!

msftclas commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-wing commented Aug 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-wing commented Aug 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dilijev commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-wing commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev commented Oct 18, 2017

Uh oh!

dilijev commented Nov 21, 2017

Uh oh!

dilijev commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

j-wing commented Aug 18, 2016 •

edited

Loading

j-wing commented Aug 21, 2016 •

edited

Loading

dilijev commented Feb 22, 2017 •

edited

Loading

dilijev commented Dec 19, 2017 •

edited

Loading