Summary: | Setters are just getters that take an extra argument and don't return a value | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||
Component: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2014-04-07 18:35:13 PDT
Created attachment 228790 [details]
the patch
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.
(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 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? (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. Created attachment 228792 [details]
the patch
With fixes.
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? > if someone added a GetValue ByIdStubKind
*SetValue
(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. (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. Created attachment 228796 [details]
the patch
Make the isAccessor thing more obvious
Comment on attachment 228796 [details]
the patch
r=me
Landed in http://trac.webkit.org/changeset/166908 |