Bug 140236 – Make the LLINT and Baseline JIT's op_create_arguments and op_get_argument_by_val use their lexicalEnvironment operand

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

[Mark Lam]

After https://webkit.org/b/140233 lands, we can start making use of the lexicalEnvironment operand in the LLINT and Baseline JIT's implementation of op_create_arguments and op_get_argument_by_val.

[Mark Lam]

Created attachment 244233 [details]
the patch.

This patch is contingent on https://bugs.webkit.org/show_bug.cgi?id=140233 being resolved.  Until then, it will certainly not build, but I'm uploading it to archive the work for now.

[Mark Lam]

(In reply to comment #1)
> Until then, it will certainly not build, but I'm uploading it to archive the work for now.

Correction: it may build, but it will not run correctly.

[Mark Lam]

Comment on attachment 244233 [details]
the patch.

https://bugs.webkit.org/show_bug.cgi?id=140233 is resolved.  This patch is ready for review.

[Geoffrey Garen]

Comment on attachment 244233 [details]
the patch.

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

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:767
> +        if (VirtualRegister(lexicalEnvironmentReg).isLocal()) {

The use of isLocal here is confusing. When is the lexical environment ever an argument?

Did you mean to use isValid()?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:769
> +            lexicalEnvironment = exec->uncheckedR(lexicalEnvironmentReg).Register::lexicalEnvironment();

Why does lexicalEnvironment need explicit name qualification here?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:226
> +    if (VirtualRegister(lexicalEnvironmentReg).isLocal()) {

Ditto.

[Mark Lam]

Comment on attachment 244233 [details]
the patch.

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

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:767
>> +        if (VirtualRegister(lexicalEnvironmentReg).isLocal()) {
> 
> The use of isLocal here is confusing. When is the lexical environment ever an argument?
> 
> Did you mean to use isValid()?

I do mean isLocal() because the other value isConstant().  isValid() returns true for both.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:769
>> +            lexicalEnvironment = exec->uncheckedR(lexicalEnvironmentReg).Register::lexicalEnvironment();
> 
> Why does lexicalEnvironment need explicit name qualification here?

I’m overriding the lexcicalEnvironment var which was initialized to a nullptr by default above.  How is this unnecessary?  What am I missing?  The alternative is replace this if statement and use a super long ?: expression with a comma expression for the ASSERT().  Is that what you’re suggesting?

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:226
>> +    if (VirtualRegister(lexicalEnvironmentReg).isLocal()) {
> 
> Ditto.

Same here.  isValid() will not weed out the constant 0 when we don’t have a JSLexicalEnvironment.

[Filip Pizlo]

(In reply to comment #5)
> Comment on attachment 244233 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244233&action=review
> 
> >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:767
> >> +        if (VirtualRegister(lexicalEnvironmentReg).isLocal()) {
> > 
> > The use of isLocal here is confusing. When is the lexical environment ever an argument?
> > 
> > Did you mean to use isValid()?
> 
> I do mean isLocal() because the other value isConstant().  isValid() returns
> true for both.

Then !isConstant() is what you should be doing.

> 
> >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:769
> >> +            lexicalEnvironment = exec->uncheckedR(lexicalEnvironmentReg).Register::lexicalEnvironment();
> > 
> > Why does lexicalEnvironment need explicit name qualification here?
> 
> I’m overriding the lexcicalEnvironment var which was initialized to a
> nullptr by default above.  How is this unnecessary?  What am I missing?  The
> alternative is replace this if statement and use a super long ?: expression
> with a comma expression for the ASSERT().  Is that what you’re suggesting?
> 
> >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:226
> >> +    if (VirtualRegister(lexicalEnvironmentReg).isLocal()) {
> > 
> > Ditto.
> 
> Same here.  isValid() will not weed out the constant 0 when we don’t have a
> JSLexicalEnvironment.

[Mark Lam]

Created attachment 244275 [details]
patch 2

Geoff clarified offline that he was referring to the unnecessary qualification of lexicalEnvironment() as Register::lexicalEnvironment().  This patch removes that qualification as well as applies Fil's suggestion to use isConstant().  Since it is evident that it is not clear from the code that the lexicalEnvironment operand can only be the lexicalEnvironment local or a constant 0, I added a comment to document this.

[Mark Lam]

Created attachment 244276 [details]
patch 3

The constant case is a constant empty JSValue, not a constant 0.  Patch 3 fixes the comment.

[Geoffrey Garen]

Comment on attachment 244275 [details]
patch 2

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:227
> +    // The lexicalEnvironmentReg operand can only be the local designated for the lexicalEnvironment
> +    // if available. Otherwise, it's a constant 0 indicating a nullptr.

0 is not quite right.

I would just say "If lexicalEnvironmentReg is constant, then there is no lexical environment".

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:229
> +        ASSERT(exec->codeBlock()->activationRegister().offset() == lexicalEnvironmentReg);

I don't think this ASSERT adds anything.

Side note: This ASSERT is wrong because of a bug in a previous patch. CodeBlock::hasActivationRegister(), needsActivation(), and so on, are broken:

    bool hasActivationRegister() const { return m_lexicalEnvironmentRegister.isValid(); }

Now that we use a constant null register, isValid() is not the right check. You should fix this.

[Mark Lam]

Comment on attachment 244275 [details]
patch 2

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

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:227
>> +    // if available. Otherwise, it's a constant 0 indicating a nullptr.
> 
> 0 is not quite right.
> 
> I would just say "If lexicalEnvironmentReg is constant, then there is no lexical environment".

OK.  I’ll change it.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:229
>> +        ASSERT(exec->codeBlock()->activationRegister().offset() == lexicalEnvironmentReg);
> 
> I don't think this ASSERT adds anything.
> 
> Side note: This ASSERT is wrong because of a bug in a previous patch. CodeBlock::hasActivationRegister(), needsActivation(), and so on, are broken:
> 
>     bool hasActivationRegister() const { return m_lexicalEnvironmentRegister.isValid(); }
> 
> Now that we use a constant null register, isValid() is not the right check. You should fix this.

Not true.  With Fil’s objection, I no longer set m_lexicalEnvironmentRegister to the empty JSValue.  Hence, the isValid() check is still correct (which is how Fil wanted it).

What this assert is checking is making sure that the lexicalEnvironment operand passed in does indeed matches the codeBlock’s expected lexicalEnvironment register.  Since the 2 values come from 2 different sources, it is meaningful to assert that one of them hasn’t been corrupted.

[Mark Lam]

Created attachment 244296 [details]
patch 4

Per my offline discussion with Geoff, patch 4 makes it so that both the CodeBlock's m_lexicalEnvironmentReg and the op_create_arguments (and op_get_argument_by_val) lexicalEnvironment operand is either a local (corresponding to the CodeBlock's designated local for the lexicalEnvironment) or an invalid VirtualRegister.

I've remove all unnecessary comments, and also the asserts in LLIntSlowPath and CommonSlowPath.  I changed my mind on the necessity of the asserts there because Arguments::create() will already assert the same thing.

Patch 4 has been tested with the JSC stress tests on both 32-bit and 64-bit x86.

[Geoffrey Garen]

Comment on attachment 244296 [details]
patch 4

r=me

[WebKit Commit Bot]

Comment on attachment 244296 [details]
patch 4

Clearing flags on attachment: 244296

Committed r178143: <http://trac.webkit.org/changeset/178143>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.