Summary: | arguments[-1] should have well-defined behavior | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | barraclough, benjamin, commit-queue, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, saam, sam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 141193 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Filip Pizlo
2015-02-02 17:03:33 PST
Created attachment 245916 [details]
work in progress
This fixes all of the 64-bit paths. Still need to do 32-bit DFG.
Created attachment 245917 [details]
the patch
Attachment 245917 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2007: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 245918 [details]
the patch
Fix style and the wording of the ChangeLog
Comment on attachment 245918 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245918&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4356 > Imm32(node->origin.semantic.inlineCallFrame->arguments.size()))); I think you need to -1 from arguments.size() here. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4400 > + m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag) + sizeof(EncodedJSValue)), Per our offline discussion, change offsetOfArgumentsIncludingThis() into offsetOfArguments() and hence, no need for the "+ sizeof(EncodedJSValue))" thereafter. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4432 > Imm32(node->origin.semantic.inlineCallFrame->arguments.size()))); Ditto. Need to -1 from arguments.size() here. > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1053 > + loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.payload) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT0); > + loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.tag) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT1); Instead of CallFrame::thisArgumentOffset(), you can use CallFrame::argumentOffset(0) here, and do away with the "+ sizeof(Register)" adjustments. > Source/JavaScriptCore/jit/JITOpcodes.cpp:928 > + load64(BaseIndex(callFrameRegister, regT1, TimesEight, CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT0); Ditto. Use CallFrame::argumentOffset(0) and remove adjustment. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1621 > + loadi ThisArgumentOffset + 8 + TagOffset[cfr, t2, 8], t0 > + loadi ThisArgumentOffset + 8 + PayloadOffset[cfr, t2, 8], t1 I think it'll be clearer if you define a FirstArgumentOffset in LowLevelInterpreter.asm, and use that here instead of using ThisArgumentOffset + 8. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1480 > + loadq ThisArgumentOffset + 8[cfr, t2, 8], t0 Ditto. Use FirstArgumentOffset. Created attachment 245920 [details]
the patch
Address Mark's comments
Comment on attachment 245920 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245920&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1617 > + subi 1, t2 Shouldn't this be t1? Comment on attachment 245920 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245920&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1617 >> + subi 1, t2 > > Shouldn't this be t1? Yes. :-) I just fixed that locally. Magically, tests started passing. Comment on attachment 245920 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245920&action=review r=me with remaining issues addressed. > Source/JavaScriptCore/jit/AssemblyHelpers.h:609 > + return CallFrame::argumentOffsetIncludingThis(1) * sizeof(Register); You can make this CallFrame::argumentOffset(0) instead of CallFrame::argumentOffsetIncludingThis(1). (In reply to comment #9) > Comment on attachment 245920 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245920&action=review > > r=me with remaining issues addressed. > > > Source/JavaScriptCore/jit/AssemblyHelpers.h:609 > > + return CallFrame::argumentOffsetIncludingThis(1) * sizeof(Register); > > You can make this CallFrame::argumentOffset(0) instead of > CallFrame::argumentOffsetIncludingThis(1). Thanks for the feedback, I just made this change locally. Landed in http://trac.webkit.org/changeset/179538 |