Bug 172455 – FTL stack overflow handling should not assume that B3 never selects callee-saves in the prologue

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

[Filip Pizlo]

In one bizarre case, B3 used a callee-save for the CodeBlock constant created in the prologue before the stack overflow check.

[Filip Pizlo]

Created attachment 310890 [details]
the patch

[Filip Pizlo]

Comment on attachment 310890 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310890&action=review

> Source/JavaScriptCore/heap/Subspace.cpp:56
> +
> +

I'll revert

[Mark Lam]

Comment on attachment 310890 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310890&action=review

r=me with comment.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:233
> +                    // OOPS! Need bug URL.

Please add bug url before landing.

> Source/JavaScriptCore/heap/Subspace.cpp:220
> +ALWAYS_INLINE void Subspace::didAllocate(void* ptr)
> +{
> +    UNUSED_PARAM(ptr);
> +    
> +    // This is useful for logging allocations, or doing other kinds of debugging hacks. Just make
> +    // sure you JSC_forceGCSlowPaths=true.
> +}
> +

This set of changes (while useful) seems totally unrelated to this patch.  Did you intend to include it?

[Filip Pizlo]

(In reply to Mark Lam from comment #3)
> Comment on attachment 310890 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310890&action=review
> 
> r=me with comment.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:233
> > +                    // OOPS! Need bug URL.
> 
> Please add bug url before landing.

Ooops!  I filed it: https://bugs.webkit.org/show_bug.cgi?id=172456

> 
> > Source/JavaScriptCore/heap/Subspace.cpp:220
> > +ALWAYS_INLINE void Subspace::didAllocate(void* ptr)
> > +{
> > +    UNUSED_PARAM(ptr);
> > +    
> > +    // This is useful for logging allocations, or doing other kinds of debugging hacks. Just make
> > +    // sure you JSC_forceGCSlowPaths=true.
> > +}
> > +
> 
> This set of changes (while useful) seems totally unrelated to this patch. 
> Did you intend to include it?

Yeah.  This bug looked like a GC bug until for the first ~2 hours that I spent on it.  Every time I run into a GC bug, the first thing I do is instrument every allocation site.  I finally decided to do it the "right" way.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/217221/webkit

[Saam Barati]

Comment on attachment 310890 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310890&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        The FTL needs to run B3's callee-save register restoration before it runs the exception
> +        handler's callee-save register restoration.  This exposes B3's callee-save register
> +        algorithm in AssemblyHelpers so that the FTL can call it.

Nice. I actually realized this bug when reading the FTL overflow code when doing wasm overflow. I should have file a bug when I thought this could be an issue.

An alternative way to solve this, and is what I did for wasm, which is unwind from the top frame (not the caller frame of the top frame), and just let the unwinder
restore the callee saves. I concluded this was safe for wasm/JS because the caller is responsible for ensuring that the callee's FP is not overflowed.