AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

Revert "[parser] Fix variable caching for conflict lookup"

Browse Source 
This reverts commit 026a0c214a.

Reason for revert: Reverting due to https://crbug.com/1029461

Original change's description:
> [parser] Fix variable caching for conflict lookup
> 
> During conflict lookup (for lexical variables and sloppy block function
> hoisting), we cache the looked-up variable on the current scope if the
> lookup goes through a ScopeInfo. However, for variable lookup during
> scope analysis, we use the "entry point" as the cache.
> 
> Since both lookups can create Variables, this can cause us to create
> duplicate variables, e.g. a duplicate function name variable in the
> attached test.
> 
> Instead, for ScopeInfo conflict lookups we can cache the result on the
> function's outer scope, which shoud be equivalent to the entry point.
> 
> As a (necessary) drive-by, we can terminate the lookup early if we find
> a VAR with the same name, as we can safely assume that its existence
> means that it doesn't conflict, which means that our variable can't
> conflict either.
> 
> Bug: chromium:1026603
> Change-Id: I19f80f65597ba6573ebe0b48aa5698f55e5c3ea1
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1928861
> Commit-Queue: Leszek Swirski <leszeks@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#65138}

TBR=leszeks@chromium.org,verwaest@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:1026603
Bug: chromium:1029461
Change-Id: Id7f5dd342e32e1bb57c51b3748feff32ee0ba41d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1958014
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65390}
This commit is contained in:
Leszek Swirski 2019-12-09 11:05:00 +00:00 committed by Commit Bot
parent f8d64084cd
commit b8fef1a7ce
3 changed files with 9 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
src/ast/scopes.cc
View File

@ -509,9 +509,9 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
// example, that does not prevent hoisting of the function in
// `{ let e; try {} catch (e) { function e(){} } }`
do {
var = query_scope->LookupInScopeOrScopeInfo(name, outer_scope_);
if (var != nullptr) {
should_hoist = !IsLexicalVariableMode(var->mode());
var = query_scope->LookupInScopeOrScopeInfo(name);
if (var != nullptr && IsLexicalVariableMode(var->mode())) {
should_hoist = false;
break;
}
query_scope = query_scope->outer_scope();
@ -1161,12 +1161,9 @@ Declaration* DeclarationScope::CheckConflictingVarDeclarations() {
do {
// There is a conflict if there exists a non-VAR binding up to the
// declaration scope in which this sloppy-eval runs.
Variable* other_var = current->LookupInScopeOrScopeInfo(
decl->var()->raw_name(), outer_scope_);
if (other_var != nullptr) {
// If this is a VAR, then we know that it doesn't conflict with
// anything, so we can't conflict with anything either.
if (!IsLexicalVariableMode(other_var->mode())) return nullptr;
Variable* other_var =
current->LookupInScopeOrScopeInfo(decl->var()->raw_name());
if (other_var != nullptr && IsLexicalVariableMode(other_var->mode())) {
DCHECK(!current->is_catch_scope());
return decl;
}

7
src/ast/scopes.h
View File

@ -558,16 +558,15 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
return false;
}
Variable* LookupInScopeOrScopeInfo(const AstRawString* name, Scope* cache) {
Variable* LookupInScopeOrScopeInfo(const AstRawString* name) {
Variable* var = variables_.Lookup(name);
if (var != nullptr || scope_info_.is_null()) return var;
DCHECK_NOT_NULL(cache);
return LookupInScopeInfo(name, cache);
return LookupInScopeInfo(name, this);
}
Variable* LookupForTesting(const AstRawString* name) {
for (Scope* scope = this; scope != nullptr; scope = scope->outer_scope()) {
Variable* var = scope->LookupInScopeOrScopeInfo(name, this);
Variable* var = scope->LookupInScopeOrScopeInfo(name);
if (var != nullptr) return var;
}
return nullptr;

13
test/mjsunit/regress/regress-crbug-1026603.js
View File

@ -1,13 +0,0 @@
// Copyright 2019 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
(function f() {
with ({}) {
// Make sure that variable conflict resulution and variable lookup through
// deserialized scopes use the same cache scope. Declare a variable which
// checks for (and fails to find) a conflict, allocating the f variable as
// it goes, then access f, which also looks up f.
eval("var f; f;");
}
})();