Bug 131336

Summary: Setters are just getters that take an extra argument and don't return a value
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 130750    
Attachments:
Description Flags
the patch
none
the patch
none
the patch ggaren: review+

Description Filip Pizlo 2014-04-07 18:35:13 PDT
Other than that, they're totally the same thing.
Comment 1 Filip Pizlo 2014-04-07 18:38:53 PDT
Created attachment 228790 [details]
the patch
Comment 2 WebKit Commit Bot 2014-04-07 18:41:09 PDT
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.
Comment 3 Filip Pizlo 2014-04-07 18:42:36 PDT
(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.
Comment 4 Mark Hahnenberg 2014-04-07 18:58:00 PDT
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?
Comment 5 Filip Pizlo 2014-04-07 18:59:32 PDT
(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.
Comment 6 Filip Pizlo 2014-04-07 19:01:33 PDT
Created attachment 228792 [details]
the patch

With fixes.
Comment 7 Geoffrey Garen 2014-04-07 19:03:55 PDT
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?
Comment 8 Geoffrey Garen 2014-04-07 19:04:40 PDT
> if someone added a GetValue ByIdStubKind

*SetValue
Comment 9 Filip Pizlo 2014-04-07 19:19:45 PDT
(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.
Comment 10 Filip Pizlo 2014-04-07 19:24:45 PDT
(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.
Comment 11 Filip Pizlo 2014-04-07 19:25:18 PDT
Created attachment 228796 [details]
the patch

Make the isAccessor thing more obvious
Comment 12 Geoffrey Garen 2014-04-07 20:46:15 PDT
Comment on attachment 228796 [details]
the patch

r=me
Comment 13 Filip Pizlo 2014-04-07 20:55:55 PDT
Landed in http://trac.webkit.org/changeset/166908