Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551023004:
[ignition] Fix hole check for dynamic local variables (Closed)
|
Created:
4 years ago by Leszek Swirski Modified:
4 years ago CC:
v8-reviews_googlegroups.com, rmcilroy Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[ignition] Fix hole check for dynamic local variables
The fast-path for dynamic local variables was previously checking the
lookup variable rather than the shadowed variable when deciding whether
to add a hole check.
BUG=669540
Committed: https://crrev.com/f6ee3b5ff36c00112b01a82ced154f5b22731ace
Cr-Commit-Position: refs/heads/master@{#41677}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move shadowed variable check to AccessNeedsHoleCheck #
Total comments: 1
Patch Set 3 : Add a DCHECK and a comment #
Messages
Total messages: 25 (15 generated)
The CQ bit was checked by leszeks@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.
leszeks@chromium.org changed reviewers: + adamk@chromium.org, neis@chromium.org
Adam and Georg, could you please check the correctness of this fix for crbug.com/669540? Notably, we'll end up doing the hole check twice whenever the slow path is taken, I don't think this is avoidable?
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
https://codereview.chromium.org/2551023004/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2551023004/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1863: if (local_variable->binding_needs_init()) { This seems a bit hacky, I would rather fix the hole_check_mode in scope.cc AccessNeedsHoleCheck (assuming this is where the error is introduced).
https://codereview.chromium.org/2551023004/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2551023004/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1863: if (local_variable->binding_needs_init()) { On 2016/12/05 19:18:07, rmcilroy wrote: > This seems a bit hacky, I would rather fix the hole_check_mode in scope.cc > AccessNeedsHoleCheck (assuming this is where the error is introduced). That's what I thought originally, but non-local variables were explicitly defined as not needing init in https://codereview.chromium.org/2232343002/ (which is why I added neis and adamk as reviewers). I'm not sure what a good representation of the semantics is -- the actual lookup variable itself does not need a hole check (because it's done in the runtime, and has to be done in the runtime unconditionally because it's runtime anything can happen). On the fast path, it's the local (non-shadowed) variable that we care about, and for this one we *do* want the hole check to be conditional, but it's conditional on some property of the local variable. So, the "needs hole check" for the lookup variable is correct, it's just not the same as "needs hole check" for the local variable. At least, that's how I understand it, I may be way off.
https://codereview.chromium.org/2551023004/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2551023004/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1863: if (local_variable->binding_needs_init()) { On 2016/12/05 19:42:37, Leszek Swirski wrote: > On 2016/12/05 19:18:07, rmcilroy wrote: > > This seems a bit hacky, I would rather fix the hole_check_mode in scope.cc > > AccessNeedsHoleCheck (assuming this is where the error is introduced). > > That's what I thought originally, but non-local variables were explicitly > defined as not needing init in https://codereview.chromium.org/2232343002/ > (which is why I added neis and adamk as reviewers). > > I'm not sure what a good representation of the semantics is -- the actual lookup > variable itself does not need a hole check (because it's done in the runtime, > and has to be done in the runtime unconditionally because it's runtime anything > can happen). On the fast path, it's the local (non-shadowed) variable that we > care about, and for this one we *do* want the hole check to be conditional, but > it's conditional on some property of the local variable. So, the "needs hole > check" for the lookup variable is correct, it's just not the same as "needs hole > check" for the local variable. > > At least, that's how I understand it, I may be way off. I think it's possible for us to do this determination earlier, in AccessNeedsHoleCheck, since we have all three pieces of data necessary: the VariableProxy, the DYNAMIC_LOCAL variable, and the local_if_not_shadowed variable. It might be as simple as the following at the top of AccessNeedsHoleCheck(): if (var->mode() == DYNAMIC_LOCAL) return AccessNeedsHoleCheck(var->local_if_not_shadowed(), proxy, scope);
The CQ bit was checked by leszeks@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/2551023004/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2551023004/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1863: if (local_variable->binding_needs_init()) { On 2016/12/06 19:16:42, adamk wrote: > On 2016/12/05 19:42:37, Leszek Swirski wrote: > > On 2016/12/05 19:18:07, rmcilroy wrote: > > > This seems a bit hacky, I would rather fix the hole_check_mode in scope.cc > > > AccessNeedsHoleCheck (assuming this is where the error is introduced). > > > > That's what I thought originally, but non-local variables were explicitly > > defined as not needing init in https://codereview.chromium.org/2232343002/ > > (which is why I added neis and adamk as reviewers). > > > > I'm not sure what a good representation of the semantics is -- the actual > lookup > > variable itself does not need a hole check (because it's done in the runtime, > > and has to be done in the runtime unconditionally because it's runtime > anything > > can happen). On the fast path, it's the local (non-shadowed) variable that we > > care about, and for this one we *do* want the hole check to be conditional, > but > > it's conditional on some property of the local variable. So, the "needs hole > > check" for the lookup variable is correct, it's just not the same as "needs > hole > > check" for the local variable. > > > > At least, that's how I understand it, I may be way off. > > I think it's possible for us to do this determination earlier, in > AccessNeedsHoleCheck, since we have all three pieces of data necessary: the > VariableProxy, the DYNAMIC_LOCAL variable, and the local_if_not_shadowed > variable. > > It might be as simple as the following at the top of AccessNeedsHoleCheck(): > > if (var->mode() == DYNAMIC_LOCAL) return > AccessNeedsHoleCheck(var->local_if_not_shadowed(), proxy, scope); Fair enough, I was worried that changing it here would be a violation of the semantics (or too tight a coupling with the execution), but if you're both fine with it then sure. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm once you add a DCHECK and a comment https://codereview.chromium.org/2551023004/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2551023004/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:1654: return AccessNeedsHoleCheck(var->local_if_not_shadowed(), proxy, scope); Can you add a DCHECK here: DCHECK(!var->binding_needs_init()); And a comment? The comment should say that dynamically-introduced variables themselves never need a hole check (since they're VAR bindings, either from var or function declarations), but that the thing they shadow might need a hole check.
On 2016/12/07 17:37:41, adamk wrote: > lgtm once you add a DCHECK and a comment > > https://codereview.chromium.org/2551023004/diff/20001/src/ast/scopes.cc > File src/ast/scopes.cc (right): > > https://codereview.chromium.org/2551023004/diff/20001/src/ast/scopes.cc#newco... > src/ast/scopes.cc:1654: return > AccessNeedsHoleCheck(var->local_if_not_shadowed(), proxy, scope); > Can you add a DCHECK here: > > DCHECK(!var->binding_needs_init()); > > And a comment? The comment should say that dynamically-introduced variables > themselves never need a hole check (since they're VAR bindings, either from var > or function declarations), but that the thing they shadow might need a hole > check. Thanks Adam, I shamelessly stole your text for the comment.
The CQ bit was checked by leszeks@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2551023004/#ps40001 (title: "Add a DCHECK and a comment")
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": 40001, "attempt_start_ts": 1481637652090260,
"parent_rev": "165d331f47b778f10e06e8666af267313da20d9a", "commit_rev":
"ce9ae98e1b391fa2e0827b9b2ca4c00abfb0a16f"}
Message was sent while issue was closed.
Description was changed from ========== [ignition] Fix hole check for dynamic local variables The fast-path for dynamic local variables was previously checking the lookup variable rather than the shadowed variable when deciding whether to add a hole check. BUG=669540 ========== to ========== [ignition] Fix hole check for dynamic local variables The fast-path for dynamic local variables was previously checking the lookup variable rather than the shadowed variable when deciding whether to add a hole check. BUG=669540 Review-Url: https://codereview.chromium.org/2551023004 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [ignition] Fix hole check for dynamic local variables The fast-path for dynamic local variables was previously checking the lookup variable rather than the shadowed variable when deciding whether to add a hole check. BUG=669540 Review-Url: https://codereview.chromium.org/2551023004 ========== to ========== [ignition] Fix hole check for dynamic local variables The fast-path for dynamic local variables was previously checking the lookup variable rather than the shadowed variable when deciding whether to add a hole check. BUG=669540 Committed: https://crrev.com/f6ee3b5ff36c00112b01a82ced154f5b22731ace Cr-Commit-Position: refs/heads/master@{#41677} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f6ee3b5ff36c00112b01a82ced154f5b22731ace Cr-Commit-Position: refs/heads/master@{#41677} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
