Summary: | FTLLazySlowPaths should be able to handle being passed the zero register as a location | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ggaren, keith_miller, mark.lam, msaboff | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Saam Barati
2015-11-12 09:12:12 PST
There is one bug where if LLVM proves that something is zero, it might pass us the zero register for that value. The zero register on arm64 is also the stack pointer. The register meaning is context dependent. I'm running the tests now w/ forGCSlowPaths option set to see if there are other bugs. *** Bug 151313 has been marked as a duplicate of this bug. *** Created attachment 265605 [details]
WIP
working on arm64 but not on x86_64
Created attachment 265645 [details]
patch
Attachment 265645 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ftl/FTLLazySlowPath.h:70: The parameter name "callSiteIndex" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLazySlowPath.cpp:45: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLazySlowPath.cpp:46: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4]
Total errors found: 3 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 265645 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=265645&action=review I guess this is OK. But wouldn't it be simpler just to convert any sp stackmap entry to zr on ARM64? I think we can assume that LLVM will never say "the value is in SP" and mean "the stack pointer" instead of "the zero register". Right? > Source/JavaScriptCore/ftl/FTLLazySlowPath.cpp:39 > - const RegisterSet& usedRegisters, CallSiteIndex callSiteIndex, RefPtr<Generator> generator) > + const RegisterSet& usedRegisters, CallSiteIndex callSiteIndex, RefPtr<Generator> generator, > + GPRReg newZeroReg, ScratchRegisterAllocator scratchRegisterAllocator) I agree with stylebot here. Please remove callSiteIndex argument name. Another way to say the same thing: Yes, sp vs zr depends on context -- and presence in a stackmap is context. Comment on attachment 265645 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=265645&action=review > Source/JavaScriptCore/ftl/FTLCompile.cpp:937 > +#if CPU(ARM64) Use "if (isARM64())" > Source/JavaScriptCore/ftl/FTLInlineCacheSize.cpp:190 > +size_t sizeOfLazySlowPath() > +{ > + return MacroAssembler::maxJumpReplacementSize(); > +} > + Is this really necessary? For starters, the name is incredibly misleading. The lazy slow path's size is not maxJumpReplacementSize(). The size of the jump to the lazy slow path is maxJumpReplacementSize(). (In reply to comment #6) > Comment on attachment 265645 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265645&action=review > > I guess this is OK. > > But wouldn't it be simpler just to convert any sp stackmap entry to zr on > ARM64? I think we can assume that LLVM will never say "the value is in SP" > and mean "the stack pointer" instead of "the zero register". Right? That assumption is correct, but it would be crazy to just pass this register along as ZR. ZR behaves like ZR on some subset of ARM64 instructions. On other instructions, it behaves like SP. Our MacroAssembler does not guarantee that it will convert ZR into the immediate zero when used in those instructions that interpret it as SP. So, if we just did such a conversion, we'd have super subtle bugs inside LazySlowPath code. Just using some MacroAssembler instruction that we hadn't used previously from other LazySlowPaths might trigger this bug. > > > Source/JavaScriptCore/ftl/FTLLazySlowPath.cpp:39 > > - const RegisterSet& usedRegisters, CallSiteIndex callSiteIndex, RefPtr<Generator> generator) > > + const RegisterSet& usedRegisters, CallSiteIndex callSiteIndex, RefPtr<Generator> generator, > > + GPRReg newZeroReg, ScratchRegisterAllocator scratchRegisterAllocator) > > I agree with stylebot here. Please remove callSiteIndex argument name. Created attachment 265689 [details]
patch for landing.
Comment on attachment 265689 [details] patch for landing. Clearing flags on attachment: 265689 Committed r192525: <http://trac.webkit.org/changeset/192525> All reviewed patches have been landed. Closing bug. |