Bug 151193 - FTLLazySlowPaths should be able to handle being passed the zero register as a location
Summary: FTLLazySlowPaths should be able to handle being passed the zero register as a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
: 151313 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-11-12 09:12 PST by Saam Barati
Modified: 2015-11-17 12:52 PST (History)
5 users (show)

See Also:


Attachments
WIP (8.81 KB, patch)
2015-11-16 12:39 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (10.72 KB, patch)
2015-11-16 17:37 PST, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
patch for landing. (9.52 KB, patch)
2015-11-17 11:53 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-11-12 09:12:12 PST
I'm not sure why it's broken. I'll update this patch as I find out more.
Comment 1 Saam Barati 2015-11-13 10:43:05 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.
Comment 2 Saam Barati 2015-11-16 11:02:08 PST
*** Bug 151313 has been marked as a duplicate of this bug. ***
Comment 3 Saam Barati 2015-11-16 12:39:10 PST
Created attachment 265605 [details]
WIP

working on arm64 but not on x86_64
Comment 4 Saam Barati 2015-11-16 17:37:23 PST
Created attachment 265645 [details]
patch
Comment 5 WebKit Commit Bot 2015-11-16 17:39:16 PST
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 6 Geoffrey Garen 2015-11-16 19:40:06 PST
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.
Comment 7 Geoffrey Garen 2015-11-16 19:45:01 PST
Another way to say the same thing: Yes, sp vs zr depends on context -- and presence in a stackmap is context.
Comment 8 Filip Pizlo 2015-11-17 11:29:11 PST
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().
Comment 9 Filip Pizlo 2015-11-17 11:30:56 PST
(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.
Comment 10 Saam Barati 2015-11-17 11:53:34 PST
Created attachment 265689 [details]
patch for landing.
Comment 11 WebKit Commit Bot 2015-11-17 12:52:31 PST
Comment on attachment 265689 [details]
patch for landing.

Clearing flags on attachment: 265689

Committed r192525: <http://trac.webkit.org/changeset/192525>
Comment 12 WebKit Commit Bot 2015-11-17 12:52:34 PST
All reviewed patches have been landed.  Closing bug.