Bug 121519 – DFG doesn't properly keep scope alive for op_put_to_scope

• Status: REOPENED
• Component: JavaScriptCore

[Mark Hahnenberg]

This was a latent bug that can't actually occur in ToT. It was uncovered by causing slow path calls in the baseline JIT for op_put_to_scope in places where we couldn't before (but which were necessary for gen GC).

[Mark Hahnenberg]

Created attachment 211940 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 211940 [details]
Patch

Clearing flags on attachment: 211940

Committed r156003: <http://trac.webkit.org/changeset/156003>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 211940 [details]
Patch

Thanks for the fix. 

I think we need to go even further. I think we need a Phantom after op_get_from_scope and op_put_to_scope, and we need it regardless of the get/put kind. The bytecode implies that the output scope from op_resolve_scope is live until after op_get_from_scope / op_put_to_scope.

[Geoffrey Garen]

Reopening to track a more complete fix.

[Filip Pizlo]

Comment on attachment 211940 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3125
>                  addToGraph(PutGlobalVar, OpInfo(operand), get(value));
> +                // Keep scope alive until after put.
> +                addToGraph(Phantom, get(scope));

PutGlobalVar doesn't exit, right?  So you could have had the Phantom before the PutGlobalVar, probably.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 211940 [details])
> Thanks for the fix. 
> 
> I think we need to go even further. I think we need a Phantom after op_get_from_scope and op_put_to_scope, and we need it regardless of the get/put kind. The bytecode implies that the output scope from op_resolve_scope is live until after op_get_from_scope / op_put_to_scope.

Some put/get kinds already keep the scope alive by virtue of using it.  I think PutClosureVar is an example.  

GetClosureVar doesn't need to keep anything alive because there is no way to exit between GetClosureRegisters and GetClosureVar.  If CSE kills the GetClosureRegisters, it will already correctly replace it with a Phantom.

Mark's fix may very well be the only place where we need this, and since handlePutByOffset may exit and there were no other uses of the scope; PutGlobalVar won't exit but that case also otherwise has no Phantoms on the scope.

There is some small benefit to not adding Phantoms for those that already keep things alive appropriately, because each Phantom puts a constraint on the register allocator.  It won't matter in the FTL but it will matter in the DFG.  So, it wouldn't be wrong to take a conservative approach and sprinkle Phantom's everywhere.  But I would resist the temptation if there is a way to reason through this and have fewer Phantoms.

[Geoffrey Garen]

These remaining cases can exit unsafely:

op_get_from_scope+GlobalProperty -- structure check failure
op_get_from_scope+GlobalPropertyWithVarInjectionChecks -- structure check failure
op_get_from_scope+GlobalVar -- global var watchpoint
op_get_from_scope+GlobalVarWithVarInjectionChecks -- global var watchpoint

FWIW, I found it difficult to reason through the side-effects and guarantee that other Phantoms were not necessary.

[Filip Pizlo]

(In reply to comment #8)
> These remaining cases can exit unsafely:
> 
> op_get_from_scope+GlobalProperty -- structure check failure
> op_get_from_scope+GlobalPropertyWithVarInjectionChecks -- structure check failure
> op_get_from_scope+GlobalVar -- global var watchpoint
> op_get_from_scope+GlobalVarWithVarInjectionChecks -- global var watchpoint

Interesting.  FWIW, I find these investigations useful.  In addition to having to reason about liveness during an exit, it's worthwhile to reason about these things to ensure that you don't emit nodes for a bytecode instruction that do the following:

1: Foo(..., bc#42) // performs an observable side-effect
2: Bar(..., bc#42) // exits backwards



> 
> FWIW, I found it difficult to reason through the side-effects and guarantee that other Phantoms were not necessary.

I guess I don't find it as difficult.  But I have to think about it a lot.

We could definitely fix this by introducing the following rules:

- Bytecode parser *always* inserts phantoms at the points where bytecode kills operands.  The best way to do this is to run Mark's liveness analysis and whenever that analysis says something is killed, we put a Phantom.  This minimizes the Phantoms and means that we don't need any more Phantoms inserted anywhere in the DFG.

- Have a DFG phase that hoists Phantoms.  This will run very late in the pipeline and never in FTL mode.  It will run just before virtual register allocation.  It will just take each Phantom and put it just below the last node that exited.  We could even go further and annotate nodes that exit with the set of nodes that must be alive right before them.  This will mean that a Phantom will indicate that the last node prior to the Phantom, that exits, also kills the thing that the Phantom is trying to keep alive.

Something like would definitely make our IR easier to reason about.  But, as with all things we've ever done to make our IR easier to think about, it will probably be a compile-time regression. :-)

[Geoffrey Garen]

> - Bytecode parser *always* inserts phantoms at the points where bytecode kills operands.  

I like this idea, actually. Nice and automatic.