Bug 145295 – DFG::PutStackSinkingPhase should not treat the stack variables written by LoadVarargs/ForwardVarargs as being live

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

[Filip Pizlo]

...

[Filip Pizlo]

It would be great if PutStackSinkingPhase didn't have to be precise about this.

What's your argument for why the weird IR that PutStackSinkingPhase generates is wrong?  Could we just formalize the concept that a stray GetStack can load garbage so long as the value is irrelevant?

[Saam Barati]

(In reply to comment #1)
> It would be great if PutStackSinkingPhase didn't have to be precise about
> this.
> 
> What's your argument for why the weird IR that PutStackSinkingPhase
> generates is wrong?  Could we just formalize the concept that a stray
> GetStack can load garbage so long as the value is irrelevant?

I don't have an argument yet. I really need to investigate it more, I currently don't really know why we're invalidating the block we crash on as unreachable when it really is reachable. My hypothesis is we're generating dead code that the DFG thinks isn't dead b/c of how it will sink PutStacks to LoadVarargs even when the LoadVarargs is about to write to the same stack location as the PutStack. This is probably causing something up the chain to invalidate the block we're crashing in.

I'll post more as I find out more.

[Filip Pizlo]

(In reply to comment #2)
> (In reply to comment #1)
> > It would be great if PutStackSinkingPhase didn't have to be precise about
> > this.
> > 
> > What's your argument for why the weird IR that PutStackSinkingPhase
> > generates is wrong?  Could we just formalize the concept that a stray
> > GetStack can load garbage so long as the value is irrelevant?
> 
> I don't have an argument yet. I really need to investigate it more, I
> currently don't really know why we're invalidating the block we crash on as
> unreachable when it really is reachable. My hypothesis is we're generating
> dead code 

Let's not use the term "dead code".  I propose the following terms:

- Unreachable code.  This is code that cannot be reached by any control flow path.
- Always-exiting code.  This is a special kind of unreachable code, where you are unreachable because anyone who could have a path to you will always exit.
- Code with unused results.  This is code that has no side effects and produces a value that nobody consumes.  This is traditionally called "dead code".

I'm pretty sure you don't mean that the code is dead in the sense that its results are unused.  A PutStack is an effectful operation so it is never dead.  Same for LoadVarargs.  Probably, you are talking about unreachable code.

> that the DFG thinks isn't dead b/c of how it will sink PutStacks
> to LoadVarargs even when the LoadVarargs is about to write to the same stack
> location as the PutStack. 

This does not seem plausible.  The fact that you did a PutStack that gets overwritten has no effect on whether code is reachable or alive.

> This is probably causing something up the chain to
> invalidate the block we're crashing in.
> 
> I'll post more as I find out more.

[Saam Barati]

Created attachment 265084 [details]
patch

[WebKit Commit Bot]

Attachment 265084 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:119:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:278:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:491:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 265084 [details]
patch

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

You should also probably remove or edit this comment:

        // This code has some interesting quirks because of the fact that neither liveness nor
        // deferrals are very precise. They are only precise enough to be able to correctly tell us
        // when we may [sic] need to execute PutStacks. This means that they may report the need to
        // execute a PutStack in cases where we actually don't really need it, and that's totally OK.

> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:122
> +
> +                    auto writeHandler = [&] (VirtualRegister operand) {
> +                        RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs);
> +                        live.operand(operand) = false;
> +                    };

This seems kind of dangerous.  What if an operation both reads and writes a local?  In that case, it's live.  But you'll make it dead.

[Geoffrey Garen]

Comment on attachment 265084 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:21
> +        block would indeed be reached.The solution here is to be more precise about the 

. The

> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:110
> -                    
> +

Revert.

[Saam Barati]

Created attachment 265099 [details]
patch

Enforced correct liveness by executing writes before reads.
I also edited the comment to describe that being imprecise
about liveness can be dangerous because it can cause us
to insert GetStacks in regions where such a GetStack
makes no sense and would load garbage.

[WebKit Commit Bot]

Attachment 265099 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:288:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:501:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 265099 [details]
patch

Nice!

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/192190