121519 2013-09-17 14:20:49 -0700 DFG doesn't properly keep scope alive for op_put_to_scope 2014-04-29 15:32:28 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified REOPENED P2 Normal --- 1 mhahnenberg mhahnenberg commit-queue fpizlo ggaren msaboff oldest_to_newest 930390 0 mhahnenberg 2013-09-17 14:20:49 -0700 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). 930393 1 211940 mhahnenberg 2013-09-17 14:22:26 -0700 Created attachment 211940 Patch 930416 2 211940 commit-queue 2013-09-17 14:56:23 -0700 Comment on attachment 211940 Patch Clearing flags on attachment: 211940 Committed r156003: <http://trac.webkit.org/changeset/156003> 930417 3 commit-queue 2013-09-17 14:56:24 -0700 All reviewed patches have been landed. Closing bug. 930486 4 211940 ggaren 2013-09-17 19:05:16 -0700 Comment on attachment 211940 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. 930487 5 ggaren 2013-09-17 19:05:39 -0700 Reopening to track a more complete fix. 930489 6 211940 fpizlo 2013-09-17 19:39:13 -0700 Comment on attachment 211940 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. 930490 7 fpizlo 2013-09-17 19:41:53 -0700 (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. 930508 8 ggaren 2013-09-17 21:31:34 -0700 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. 930518 9 fpizlo 2013-09-17 22:05:16 -0700 (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. :-) 930690 10 ggaren 2013-09-18 11:41:04 -0700 > - Bytecode parser *always* inserts phantoms at the points where bytecode kills operands. I like this idea, actually. Nice and automatic. 211940 2013-09-17 14:22:26 -0700 2013-09-17 19:39:13 -0700 Patch bug-121519-20130917142132.patch text/plain 2092 mhahnenberg SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU1OTk3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDEzLTA5LTE3ICBNYXJrIEhhaG5lbmJlcmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CisK KyAgICAgICAgREZHIGRvZXNuJ3QgcHJvcGVybHkga2VlcCBzY29wZSBhbGl2ZSBmb3Igb3BfcHV0 X3RvX3Njb3BlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMjE1MTkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBUaGlzIHdhcyBhIGxhdGVudCBidWcgdGhhdCBjYW4ndCBhY3R1YWxseSBvY2N1ciBpbiBUb1Qu IEl0IHdhcyB1bmNvdmVyZWQgYnkgY2F1c2luZyBzbG93IAorICAgICAgICBwYXRoIGNhbGxzIGlu IHRoZSBiYXNlbGluZSBKSVQgZm9yIG9wX3B1dF90b19zY29wZSBpbiBwbGFjZXMgd2hlcmUgd2Ug Y291bGRuJ3QgYmVmb3JlIChidXQgCisgICAgICAgIHdoaWNoIHdlcmUgbmVjZXNzYXJ5IGZvciBn ZW4gR0MpLgorCisgICAgICAgICogZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcDoKKyAgICAgICAg KEpTQzo6REZHOjpCeXRlQ29kZVBhcnNlcjo6cGFyc2VCbG9jayk6CisKIDIwMTMtMDktMTcgIEZp bGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KIAogICAgICAgICBEb24ndCBHQyB3aGlsZSBP U1IgY29tcGlsaW5nCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2Rl UGFyc2VyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5 dGVDb2RlUGFyc2VyLmNwcAkocmV2aXNpb24gMTU1ODk1KQorKysgU291cmNlL0phdmFTY3JpcHRD b3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTMxMTIsNiAr MzExMiw4IEBAIGJvb2wgQnl0ZUNvZGVQYXJzZXI6OnBhcnNlQmxvY2sodW5zaWduZWQKICAgICAg ICAgICAgICAgICB9CiAgICAgICAgICAgICAgICAgTm9kZSogYmFzZSA9IGNlbGxDb25zdGFudFdp dGhTdHJ1Y3R1cmVDaGVjayhnbG9iYWxPYmplY3QsIHN0YXR1cy5vbGRTdHJ1Y3R1cmUoKSk7CiAg ICAgICAgICAgICAgICAgaGFuZGxlUHV0QnlPZmZzZXQoYmFzZSwgaWRlbnRpZmllck51bWJlciwg c3RhdGljX2Nhc3Q8UHJvcGVydHlPZmZzZXQ+KG9wZXJhbmQpLCBnZXQodmFsdWUpKTsKKyAgICAg ICAgICAgICAgICAvLyBLZWVwIHNjb3BlIGFsaXZlIHVudGlsIGFmdGVyIHB1dC4KKyAgICAgICAg ICAgICAgICBhZGRUb0dyYXBoKFBoYW50b20sIGdldChzY29wZSkpOwogICAgICAgICAgICAgICAg IGJyZWFrOwogICAgICAgICAgICAgfQogICAgICAgICAgICAgY2FzZSBHbG9iYWxWYXI6CkBAIC0z MTE5LDYgKzMxMjEsOCBAQCBib29sIEJ5dGVDb2RlUGFyc2VyOjpwYXJzZUJsb2NrKHVuc2lnbmVk CiAgICAgICAgICAgICAgICAgU3ltYm9sVGFibGVFbnRyeSBlbnRyeSA9IGdsb2JhbE9iamVjdC0+ c3ltYm9sVGFibGUoKS0+Z2V0KHVpZCk7CiAgICAgICAgICAgICAgICAgQVNTRVJUKCFlbnRyeS5j b3VsZEJlV2F0Y2hlZCgpIHx8ICFtX2dyYXBoLndhdGNocG9pbnRzKCkuaXNTdGlsbFZhbGlkKGVu dHJ5LndhdGNocG9pbnRTZXQoKSkpOwogICAgICAgICAgICAgICAgIGFkZFRvR3JhcGgoUHV0R2xv YmFsVmFyLCBPcEluZm8ob3BlcmFuZCksIGdldCh2YWx1ZSkpOworICAgICAgICAgICAgICAgIC8v IEtlZXAgc2NvcGUgYWxpdmUgdW50aWwgYWZ0ZXIgcHV0LgorICAgICAgICAgICAgICAgIGFkZFRv R3JhcGgoUGhhbnRvbSwgZ2V0KHNjb3BlKSk7CiAgICAgICAgICAgICAgICAgYnJlYWs7CiAgICAg ICAgICAgICB9CiAgICAgICAgICAgICBjYXNlIENsb3N1cmVWYXI6Cg==