Don't create activations to track strictness #2197
Conversation
| private boolean sharedWithActivation(int index) { | ||
| Context cx = Context.getContext(); | ||
| if (cx.isStrictMode()) { | ||
| if (activation.isStrict) { |
There was a problem hiding this comment.
Why are you looking at the activation here, and not Context::isStrictMode? I understand that, if we have arguments, we always have an activation, but it feels like we'd like to get rid of the "strict" flag in activations completely given this PR, no?
And I fear this might create problems with the debugger, since we always inject activations when we have the debugger turned on.
There was a problem hiding this comment.
And I fear this might create problems with the debugger, since we always inject activations when we have the debugger turned on.
Oh, right. Guess I'd better make a PR from my scratch branch… #2205
Also, if we are interested in whether the function is strict, then the function descriptor carries that information.
There was a problem hiding this comment.
Right. We can't get rid of the strict flag in activations yet - there are (deprecated) public methods that depend on it. I'll deprecate the flag and change the usage sites to use function descriptors.
| enterAreaStartLabel = -1; | ||
| generatorStateLocal = -1; | ||
| savedHomeObjectLocal = -1; | ||
| parentStrictnessLocal = -1; |
There was a problem hiding this comment.
Nit: IMHO this could be a local variable, rather than a field.
There was a problem hiding this comment.
I did try it out and it doesn't seem like an improvement.
| ScriptRuntime.exitActivationFunction(cx); | ||
| } | ||
|
|
||
| if (frame.fnOrScript.getDescriptor().getFunctionType() != 0) { |
There was a problem hiding this comment.
Why aren't you exiting for scripts? Imagine (in our ServiceNow codebase) a script include inclusion - we should restore the flag once we are done including it!
There was a problem hiding this comment.
Good point! I looked into this a bit and it turns out strictness handling around this is buggy - activations weren't created for strict mode scripts at all. I'll fix it.
There was a problem hiding this comment.
@andreabergia I realise my last comment didn't make a lot of sense.
We never created activations for scripts. This PR doesn't change this behaviour.
I took your comment to mean that not creating activations for scripts was probably a bug, so I tried to fix it. However, I found that it broke 2 test262 tests that had been passing. Upon close investigation, those tests had nothing to do with strictness - they just happened to rely on the "bug"
Now I'm not sure what the semantics of this kind of "script inclusion" should be. However, I found that require() also doesn't honor the script's strictness and this definitely seems to be a bug.
| @Test | ||
| public void maxLocals() throws IOException { | ||
| test(339); | ||
| test(338); |
There was a problem hiding this comment.
A comment here explaining why you had to lower the number would be nice
This is not needed anymore after https://code.devsnc.com/dev/rhino-ng/pull/863
balajirrao
force-pushed
the
no-activation-for-strictness
branch
from
ca6c4db to
b80541e
Compare
|
@balajirrao, We have introduced new failures in script-decl-func-err-non-extensible.js and script-decl-var-err.js in strict mode, only with the optimizing compiler. I think we should address those. |
@0xe yeah, on it. I swear I thought I saw them all pass before pushing 😅 |
balajirrao
force-pushed
the
no-activation-for-strictness
branch
from
b80541e to
41af741
Compare
|
The tests are passing now. See #2197 (comment) for details. |
| final JSFunction function; | ||
| final Object[] originalArgs; | ||
| final boolean isStrict; | ||
| @Deprecated final boolean isStrict; |
There was a problem hiding this comment.
Why deprecated?
Can we remove it? Otherwise, this needs a comment imho.
There was a problem hiding this comment.
@andreabergia there's a NativeCall constructor that sets this field - I forgot to mark that deprecated.
I've pushed a commit with the change, please take a look!
There was a problem hiding this comment.
The constructor isn't public. Don't deprecate it, get rid of it entirely, and the field.
balajirrao
force-pushed
the
no-activation-for-strictness
branch
from
1df087c to
fb0cc73
Compare
4 participants
This has 2 changes
isTopLevelStrictfromContext. All tests pass, however I'm not a 100% confident on this one would appreciate a careful review.This applies only to functions that have a 'use strict' directive in the body. Functions that are in strict top-levels don't use activations.