Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2520883002:
[parser] Fix scopes in rewriting of for-of and destructuring assignments. (Closed)
Description[parser] Fix scopes in rewriting of for-of and destructuring assignments.
The catch scopes were created with the wrong parent scope.
R=littledan@chromium.org
BUG=v8:5648
Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd
Committed: https://crrev.com/b481afd89300d097531347412c68c5d5fb74476d
Cr-Original-Commit-Position: refs/heads/master@{#41222}
Cr-Commit-Position: refs/heads/master@{#41253}
Patch Set 1 #Patch Set 2 : Take into account that the scope may have been removed when we get to rewrite. #
Total comments: 10
Patch Set 3 : Try to address feedback, add more tests. #Patch Set 4 : Remove debug print from test. #
Total comments: 3
Patch Set 5 : Address feedback #Patch Set 6 : Typo #Patch Set 7 : Remove incorrect DCHECK. #
Messages
Total messages: 48 (32 generated)
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
neis@chromium.org changed reviewers: + verwaest@chromium.org
+verwaest: The second patch set addresses the problem I told you about. Let me know if it makes sense to you.
Only looked at patch 2, that lgtm with minor comments https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:640: Scope* parent = outer_scope(); if (is_declaration_scope()) return false; ? https://codereview.chromium.org/2520883002/diff/20001/src/parsing/pattern-rew... File src/parsing/pattern-rewriter.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/parsing/pattern-rew... src/parsing/pattern-rewriter.cc:597: DCHECK_NOT_NULL(real_scope); What about moving this into an aptly named helper function?
lgtm Generally, the fix seems good, but I hope we can eventually move to a cleaner design where desugarings don't have to worry about denormalized datastructures like the scope/block duality. The data structure manipulations to check if scopes are live feel a bit roundabout to me, but I don't have concrete suggestions for improvement besides what Toon mentioned about factoring into named methods. https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:640: Scope* parent = outer_scope(); On 2016/11/22 08:30:18, Toon Verwaest wrote: > if (is_declaration_scope()) return false; ? Or, that could be an assertion before the return true, that it's not a declaration scope https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:649: } This is asymptotically a bit unfortunate. Is there another way we could mark already-removed scopes? This can wait for a follow-on patch. https://codereview.chromium.org/2520883002/diff/20001/src/parsing/pattern-rew... File src/parsing/pattern-rewriter.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/parsing/pattern-rew... src/parsing/pattern-rewriter.cc:592: // case, find the first outer non-removed scope. Should we put this logic to find the appropriate non-deleted scope at the entrypoint to the rewriter, rather than in its usage here? I wonder if there are other bugs lurking or possible to create in the future where we use a scope that's actually deleted (the call to NewTemporary() seems saved just because we never delete closure scopes right now, though).
https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:640: Scope* parent = outer_scope(); On 2016/11/22 16:07:00, Dan Ehrenberg wrote: > On 2016/11/22 08:30:18, Toon Verwaest wrote: > > if (is_declaration_scope()) return false; ? > > Or, that could be an assertion before the return true, that it's not a > declaration scope That's what I was originally thinking of suggesting; but we'd walk the chain for no reason which is slower :)
https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:649: } On 2016/11/22 16:07:00, Dan Ehrenberg wrote: > This is asymptotically a bit unfortunate. Is there another way we could mark > already-removed scopes? This can wait for a follow-on patch. Yeah that's a good suggestion though. We can just have a single bit on Scope.
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Added offline comments for the record. https://codereview.chromium.org/2520883002/diff/60001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2520883002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:640: if (!is_block_scope() || is_declaration_scope()) return false; // Shortcut. if (is_declaration_scope()) return false; DCHECK(is_block_scope()); https://codereview.chromium.org/2520883002/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2520883002/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:4316: while (scope != nullptr && scope->HasBeenRemoved()) { Move to helper function as mentioned before.
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:640: Scope* parent = outer_scope(); On 2016/11/22 08:30:18, Toon Verwaest wrote: > if (is_declaration_scope()) return false; ? Done. https://codereview.chromium.org/2520883002/diff/20001/src/parsing/pattern-rew... File src/parsing/pattern-rewriter.cc (right): https://codereview.chromium.org/2520883002/diff/20001/src/parsing/pattern-rew... src/parsing/pattern-rewriter.cc:592: // case, find the first outer non-removed scope. On 2016/11/22 16:07:00, Dan Ehrenberg wrote: > Should we put this logic to find the appropriate non-deleted scope at the > entrypoint to the rewriter, rather than in its usage here? I wonder if there are > other bugs lurking or possible to create in the future where we use a scope > that's actually deleted (the call to NewTemporary() seems saved just because we > never delete closure scopes right now, though). Done. https://codereview.chromium.org/2520883002/diff/20001/src/parsing/pattern-rew... src/parsing/pattern-rewriter.cc:597: DCHECK_NOT_NULL(real_scope); On 2016/11/22 08:30:18, Toon Verwaest wrote: > What about moving this into an aptly named helper function? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Description was changed from ========== [parser] Fix scopes in for-of rewriting. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 ========== to ========== [parser] Fix scopes in rewriting of for-of and destructuring assignment. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 ==========
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16727) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16688)
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [parser] Fix scopes in for-of rewriting. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 ========== to ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by neis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, verwaest@chromium.org Link to the patchset: https://codereview.chromium.org/2520883002/#ps100001 (title: "Typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479907382706920,
"parent_rev": "9c8a45e111363b73d152a64a8aba5fba0c5a9fca", "commit_rev":
"52866f3d8da1c8c2ffdd7e2f7f9583a55ec85ff5"}
Message was sent while issue was closed.
Description was changed from ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 ========== to ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 ========== to ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Cr-Commit-Position: refs/heads/master@{#41222} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Cr-Commit-Position: refs/heads/master@{#41222}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2519333005/ by machenbach@chromium.org. The reason for reverting is: Speculative revert: Seems to break jsfunfuzz: https://build.chromium.org/p/client.v8/builders/V8%20Fuzzer/builds/14385.
Message was sent while issue was closed.
https://codereview.chromium.org/2520883002/diff/60001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2520883002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:640: if (!is_block_scope() || is_declaration_scope()) return false; // Shortcut. On 2016/11/23 12:20:45, Toon Verwaest wrote: > if (is_declaration_scope()) return false; > DCHECK(is_block_scope()); This DCHECK is failing, for instance in the following nonsense code: for(let y in [5,6,7,8]) with({}) { let([, [x4]] = ([new (undefined)(-1)]), [] = [11,12,13,14].map) ((function(){throw StopIteration;})()); } where it's a with scope.
Message was sent while issue was closed.
Description was changed from ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Cr-Commit-Position: refs/heads/master@{#41222} ========== to ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Cr-Commit-Position: refs/heads/master@{#41222} ==========
The CQ bit was checked by neis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, verwaest@chromium.org Link to the patchset: https://codereview.chromium.org/2520883002/#ps120001 (title: "Remove incorrect DCHECK.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479979172431040,
"parent_rev": "27c53ec5be44a6f26705455233580a8d873bbdb6", "commit_rev":
"47d84e4f355e0a411b07f8fd988d18bb391c3a69"}
Message was sent while issue was closed.
Description was changed from ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Cr-Commit-Position: refs/heads/master@{#41222} ========== to ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Cr-Commit-Position: refs/heads/master@{#41222} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Cr-Commit-Position: refs/heads/master@{#41222} ========== to ========== [parser] Fix scopes in rewriting of for-of and destructuring assignments. The catch scopes were created with the wrong parent scope. R=littledan@chromium.org BUG=v8:5648 Committed: https://crrev.com/f385268d11d6da9508e481202b39f75f4b56afdd Committed: https://crrev.com/b481afd89300d097531347412c68c5d5fb74476d Cr-Original-Commit-Position: refs/heads/master@{#41222} Cr-Commit-Position: refs/heads/master@{#41253} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b481afd89300d097531347412c68c5d5fb74476d Cr-Commit-Position: refs/heads/master@{#41253} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
