Bug 131336 – Setters are just getters that take an extra argument and don't return a value

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

[Filip Pizlo]

Other than that, they're totally the same thing.

[Filip Pizlo]

Created attachment 228790 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/Repatch.cpp:396:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


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

[Filip Pizlo]

(In reply to comment #2)
> Attachment 228790 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/jit/Repatch.cpp:396:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> Total errors found: 1 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed.

[Mark Hahnenberg]

Comment on attachment 228790 [details]
the patch

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

> Source/JavaScriptCore/jit/Repatch.cpp:442
> +            stubJit.storeValue(
> +                valueRegs,
> +                calleeFrame.withOffset(
> +                    virtualRegisterForArgument(1).offset() * sizeof(Register)));
> +            

Is this okay to do if we're invoking a getter?

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 228790 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228790&action=review
> 
> > Source/JavaScriptCore/jit/Repatch.cpp:442
> > +            stubJit.storeValue(
> > +                valueRegs,
> > +                calleeFrame.withOffset(
> > +                    virtualRegisterForArgument(1).offset() * sizeof(Register)));
> > +            
> 
> Is this okay to do if we're invoking a getter?

Oh lol, no it's not!  It barely works because in almost all cases we're overestimating the amount of extra stack space that we need and the top of stack is clobberable (Baseline, DFG, and LLVM all leave slop at the top of stack).  But, it's wrong.  And inefficient.  I will fix by conditionalizing it.

[Filip Pizlo]

Created attachment 228792 [details]
the patch

With fixes.

[Geoffrey Garen]

Comment on attachment 228792 [details]
the patch

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

> Source/JavaScriptCore/jit/Repatch.cpp:316
> +    bool isAccessor = kind != GetValue;

This seems dangerous because it would go wrong silently if someone added a GetValue ByIdStubKind. Maybe use an isAccessorFor helper function with a switch statement inside instead?

[Geoffrey Garen]

> if someone added a GetValue ByIdStubKind

*SetValue

[Filip Pizlo]

(In reply to comment #8)
> > if someone added a GetValue ByIdStubKind
> 
> *SetValue

I presume that your SetValue means either PutByIdReplace or PutByIdTransition.  We already have those and they are handled by a different code path, since they require a different kind of idiom to handle propertly - they never involve "setting" something in the prototype chain but they may involve validating that the prototype chain doesn't have the thing we are setting.

I could add more assertions here but this code is well-tested - each line of code in here has a test case at this point - and so I don't think it's worth the clutter.

[Filip Pizlo]

(In reply to comment #9)
> (In reply to comment #8)
> > > if someone added a GetValue ByIdStubKind
> > 
> > *SetValue
> 
> I presume that your SetValue means either PutByIdReplace or PutByIdTransition.  We already have those and they are handled by a different code path, since they require a different kind of idiom to handle propertly - they never involve "setting" something in the prototype chain but they may involve validating that the prototype chain doesn't have the thing we are setting.
> 
> I could add more assertions here but this code is well-tested - each line of code in here has a test case at this point - and so I don't think it's worth the clutter.

It's actually even better than that.  isAccessor is only used in one place - where we decide to emit a call.  It's a very obvious if statement.  You would of course change this if statement if you added a new case to this code.

I don't think it's worth adding an assertion since if you *really* added a Put case to the enum but literally were so high that you forgot to change that if statement, then you would be guaranteed to ragecrash.

[Filip Pizlo]

Created attachment 228796 [details]
the patch

Make the isAccessor thing more obvious

[Geoffrey Garen]

Comment on attachment 228796 [details]
the patch

r=me

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/166908