Bug 151193 – FTLLazySlowPaths should be able to handle being passed the zero register as a location

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

[Saam Barati]

I'm not sure why it's broken. I'll update this patch as I find out more.

[Saam Barati]

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.

[Saam Barati]

*** Bug 151313 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 265605 [details]
WIP

working on arm64 but not on x86_64

[Saam Barati]

Created attachment 265645 [details]
patch

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

Another way to say the same thing: Yes, sp vs zr depends on context -- and presence in a stackmap is context.

[Filip Pizlo]

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().

[Filip Pizlo]

(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.

[Saam Barati]

Created attachment 265689 [details]
patch for landing.

[WebKit Commit Bot]

Comment on attachment 265689 [details]
patch for landing.

Clearing flags on attachment: 265689

Committed r192525: <http://trac.webkit.org/changeset/192525>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.