From 50dfeabc3c747758b4beb71bf38a7ddc57234284 Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Tue, 13 Jun 2023 08:14:36 -0700 Subject: [PATCH] Merged: CLs related to TDZ elision var numbering bug Merged: [interpreter] Don't number non-lexicals in TDZ elision Revision: 260b62db02dd50a839829fe0af8f3edc31f5d375 Merged: [interpreter] Refine hole check numbering for initialization Revision: f72cbd53ed999d0027877e3a80e54f9bd4951698 Merged: [interpreter] Use |= in Variable::ForceHoleInitialization Revision: dc628cc1ec065265c46fa3c26a23cfcfba35bb1d Bug: chromium:1448545,chromium:1450771,v8:13723 Change-Id: Ie08b443061b48545bb65b3acbe4044fe604aaae8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4610688 Commit-Queue: Shu-yu Guo Reviewed-by: Leszek Swirski Cr-Commit-Position: refs/branch-heads/11.5@{#16} Cr-Branched-From: 0c4044b7336787781646e48b2f98f0c7d1b400a5-refs/heads/11.5.150@{#1} Cr-Branched-From: b71d3038a7d99c79e1c21239e8ae07da5fc8c90b-refs/heads/main@{#87781} --- src/ast/scopes.cc | 19 ++- src/ast/variables.cc | 3 +- src/ast/variables.h | 68 +++++--- src/interpreter/bytecode-generator.cc | 6 +- test/mjsunit/regress/regress-crbug-1448545.js | 153 ++++++++++++++++++ 5 files changed, 222 insertions(+), 27 deletions(-) create mode 100644 test/mjsunit/regress/regress-crbug-1448545.js diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc index 1ebe09bc5b64..215dba2e0be7 100644 --- a/src/ast/scopes.cc +++ b/src/ast/scopes.cc @@ -751,6 +751,13 @@ void DeclarationScope::DeclareThis(AstValueFactory* ast_value_factory) { THIS_VARIABLE, derived_constructor ? kNeedsInitialization : kCreatedInitialized, kNotAssigned); + // Derived constructors have hole checks when calling super. Mark the 'this' + // variable as having hole initialization forced so that TDZ elision analysis + // applies and numbers the variable. + if (derived_constructor) { + receiver_->ForceHoleInitialization( + Variable::kHasHoleCheckUseInUnknownScope); + } locals_.Add(receiver_); } @@ -2282,9 +2289,10 @@ void Scope::ResolveVariable(VariableProxy* proxy) { namespace { -void SetNeedsHoleCheck(Variable* var, VariableProxy* proxy) { +void SetNeedsHoleCheck(Variable* var, VariableProxy* proxy, + Variable::ForceHoleInitializationFlag flag) { proxy->set_needs_hole_check(); - var->ForceHoleInitialization(); + var->ForceHoleInitialization(flag); } void UpdateNeedsHoleCheck(Variable* var, VariableProxy* proxy, Scope* scope) { @@ -2303,7 +2311,7 @@ void UpdateNeedsHoleCheck(Variable* var, VariableProxy* proxy, Scope* scope) { // unknown at compilation time whether the binding referred to in the // exporting module itself requires hole checks. if (var->location() == VariableLocation::MODULE && !var->IsExport()) { - SetNeedsHoleCheck(var, proxy); + SetNeedsHoleCheck(var, proxy, Variable::kHasHoleCheckUseInUnknownScope); return; } @@ -2326,7 +2334,8 @@ void UpdateNeedsHoleCheck(Variable* var, VariableProxy* proxy, Scope* scope) { // The scope of the variable needs to be checked, in case the use is // in a sub-block which may be linear. if (var->scope()->GetClosureScope() != scope->GetClosureScope()) { - SetNeedsHoleCheck(var, proxy); + SetNeedsHoleCheck(var, proxy, + Variable::kHasHoleCheckUseInDifferentClosureScope); return; } @@ -2336,7 +2345,7 @@ void UpdateNeedsHoleCheck(Variable* var, VariableProxy* proxy, Scope* scope) { if (var->scope()->is_nonlinear() || var->initializer_position() >= proxy->position()) { - SetNeedsHoleCheck(var, proxy); + SetNeedsHoleCheck(var, proxy, Variable::kHasHoleCheckUseInSameClosureScope); return; } } diff --git a/src/ast/variables.cc b/src/ast/variables.cc index 3b474f951694..6f5d94257b91 100644 --- a/src/ast/variables.cc +++ b/src/ast/variables.cc @@ -49,7 +49,8 @@ void Variable::AssignHoleCheckBitmapIndex(ZoneVector& list, DCHECK_EQ(next_index, list.size() + 1); DCHECK_NE(kUncacheableHoleCheckBitmapIndex, next_index); DCHECK_LT(next_index, kHoleCheckBitmapBits); - hole_check_bitmap_index_ = next_index; + hole_check_analysis_bit_field_ = HoleCheckBitmapIndexField::update( + hole_check_analysis_bit_field_, next_index); list.push_back(this); } diff --git a/src/ast/variables.h b/src/ast/variables.h index 590390b31609..30d41ea800d0 100644 --- a/src/ast/variables.h +++ b/src/ast/variables.h @@ -30,16 +30,18 @@ class Variable final : public ZoneObject { next_(nullptr), index_(-1), initializer_position_(kNoSourcePosition), - hole_check_bitmap_index_(kUncacheableHoleCheckBitmapIndex), bit_field_(MaybeAssignedFlagField::encode(maybe_assigned_flag) | InitializationFlagField::encode(initialization_flag) | VariableModeField::encode(mode) | IsUsedField::encode(false) | ForceContextAllocationBit::encode(false) | - ForceHoleInitializationField::encode(false) | LocationField::encode(VariableLocation::UNALLOCATED) | VariableKindField::encode(kind) | - IsStaticFlagField::encode(is_static_flag)) { + IsStaticFlagField::encode(is_static_flag)), + hole_check_analysis_bit_field_(HoleCheckBitmapIndexField::encode( + kUncacheableHoleCheckBitmapIndex) | + ForceHoleInitializationFlagField::encode( + kHoleInitializationNotForced)) { // Var declared variables never need initialization. DCHECK(!(mode == VariableMode::kVar && initialization_flag == kNeedsInitialization)); @@ -164,18 +166,38 @@ class Variable final : public ZoneObject { return initialization_flag() == kNeedsInitialization; } + enum ForceHoleInitializationFlag { + kHoleInitializationNotForced = 0, + kHasHoleCheckUseInDifferentClosureScope = 1 << 0, + kHasHoleCheckUseInSameClosureScope = 1 << 1, + kHasHoleCheckUseInUnknownScope = kHasHoleCheckUseInDifferentClosureScope | + kHasHoleCheckUseInSameClosureScope + }; + ForceHoleInitializationFlag force_hole_initialization_flag_field() const { + return ForceHoleInitializationFlagField::decode( + hole_check_analysis_bit_field_); + } + bool IsHoleInitializationForced() const { - return ForceHoleInitializationField::decode(bit_field_); + return force_hole_initialization_flag_field() != + kHoleInitializationNotForced; + } + + bool HasHoleCheckUseInSameClosureScope() const { + return force_hole_initialization_flag_field() & + kHasHoleCheckUseInSameClosureScope; } // Called during scope analysis when a VariableProxy is found to // reference this Variable in such a way that a hole check will // be required at runtime. - void ForceHoleInitialization() { + void ForceHoleInitialization(ForceHoleInitializationFlag flag) { DCHECK_EQ(kNeedsInitialization, initialization_flag()); + DCHECK_NE(kHoleInitializationNotForced, flag); DCHECK(IsLexicalVariableMode(mode()) || IsPrivateMethodOrAccessorVariableMode(mode())); - bit_field_ = ForceHoleInitializationField::update(bit_field_, true); + hole_check_analysis_bit_field_ |= + ForceHoleInitializationFlagField::encode(flag); } // The first N-1 lexical bindings that need hole checks in a compilation are @@ -193,28 +215,29 @@ class Variable final : public ZoneObject { std::numeric_limits::digits; void ResetHoleCheckBitmapIndex() { - hole_check_bitmap_index_ = kUncacheableHoleCheckBitmapIndex; + hole_check_analysis_bit_field_ = HoleCheckBitmapIndexField::update( + hole_check_analysis_bit_field_, kUncacheableHoleCheckBitmapIndex); } void RememberHoleCheckInBitmap(HoleCheckBitmap& bitmap, ZoneVector& list) { DCHECK(v8_flags.ignition_elide_redundant_tdz_checks); - if (V8_UNLIKELY(hole_check_bitmap_index_ == - kUncacheableHoleCheckBitmapIndex)) { - uint8_t next_index = list.size() + 1; + uint8_t index = HoleCheckBitmapIndex(); + if (V8_UNLIKELY(index == kUncacheableHoleCheckBitmapIndex)) { + index = list.size() + 1; // The bitmap is full. - if (next_index == kHoleCheckBitmapBits) return; - AssignHoleCheckBitmapIndex(list, next_index); + if (index == kHoleCheckBitmapBits) return; + AssignHoleCheckBitmapIndex(list, index); } - bitmap |= HoleCheckBitmap{1} << hole_check_bitmap_index_; + bitmap |= HoleCheckBitmap{1} << index; DCHECK_EQ( 0, bitmap & (HoleCheckBitmap{1} << kUncacheableHoleCheckBitmapIndex)); } bool HasRememberedHoleCheck(HoleCheckBitmap bitmap) const { - bool result = bitmap & (HoleCheckBitmap{1} << hole_check_bitmap_index_); - DCHECK_IMPLIES(hole_check_bitmap_index_ == kUncacheableHoleCheckBitmapIndex, - !result); + uint8_t index = HoleCheckBitmapIndex(); + bool result = bitmap & (HoleCheckBitmap{1} << index); + DCHECK_IMPLIES(index == kUncacheableHoleCheckBitmapIndex, !result); return result; } @@ -305,13 +328,17 @@ class Variable final : public ZoneObject { Variable* next_; int index_; int initializer_position_; - uint8_t hole_check_bitmap_index_; uint16_t bit_field_; + uint16_t hole_check_analysis_bit_field_; void set_maybe_assigned() { bit_field_ = MaybeAssignedFlagField::update(bit_field_, kMaybeAssigned); } + uint8_t HoleCheckBitmapIndex() const { + return HoleCheckBitmapIndexField::decode(hole_check_analysis_bit_field_); + } + void AssignHoleCheckBitmapIndex(ZoneVector& list, uint8_t next_index); @@ -321,11 +348,14 @@ class Variable final : public ZoneObject { using ForceContextAllocationBit = LocationField::Next; using IsUsedField = ForceContextAllocationBit::Next; using InitializationFlagField = IsUsedField::Next; - using ForceHoleInitializationField = InitializationFlagField::Next; using MaybeAssignedFlagField = - ForceHoleInitializationField::Next; + InitializationFlagField::Next; using IsStaticFlagField = MaybeAssignedFlagField::Next; + using HoleCheckBitmapIndexField = base::BitField16; + using ForceHoleInitializationFlagField = + HoleCheckBitmapIndexField::Next; + Variable** next() { return &next_; } friend List; friend base::ThreadedListTraits; diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc index 13f30613cbd3..2e504491d606 100644 --- a/src/interpreter/bytecode-generator.cc +++ b/src/interpreter/bytecode-generator.cc @@ -4031,7 +4031,8 @@ void BytecodeGenerator::BuildVariableAssignment( } if (mode != VariableMode::kConst || op == Token::INIT) { - if (op == Token::INIT) { + if (op == Token::INIT && + variable->HasHoleCheckUseInSameClosureScope()) { // After initializing a variable it won't be the hole anymore, so // elide subsequent checks. RememberHoleCheckInCurrentBlock(variable); @@ -4072,7 +4073,8 @@ void BytecodeGenerator::BuildVariableAssignment( } if (mode != VariableMode::kConst || op == Token::INIT) { - if (op == Token::INIT) { + if (op == Token::INIT && + variable->HasHoleCheckUseInSameClosureScope()) { // After initializing a variable it won't be the hole anymore, so // elide subsequent checks. RememberHoleCheckInCurrentBlock(variable); diff --git a/test/mjsunit/regress/regress-crbug-1448545.js b/test/mjsunit/regress/regress-crbug-1448545.js new file mode 100644 index 000000000000..e6b15d2d0e28 --- /dev/null +++ b/test/mjsunit/regress/regress-crbug-1448545.js @@ -0,0 +1,153 @@ +// Copyright 2023 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. + +// Flags: --fuzzing + +// --fuzzing is required to trigger the bug since it does a second compile +// --that'll check for identical bytecode. + +// Make more lexical bindings that need hole checks due to uses in inner +// functions than a 64-bitmap can hold. +let v1 = 0; +let v2 = 0; +let v3 = 0; +let v4 = 0; +let v5 = 0; +let v6 = 0; +let v7 = 0; +let v8 = 0; +let v9 = 0; +let v10 = 0; +let v11 = 0; +let v12 = 0; +let v13 = 0; +let v14 = 0; +let v15 = 0; +let v16 = 0; +let v17 = 0; +let v18 = 0; +let v19 = 0; +let v20 = 0; +let v21 = 0; +let v22 = 0; +let v23 = 0; +let v24 = 0; +let v25 = 0; +let v26 = 0; +let v27 = 0; +let v28 = 0; +let v29 = 0; +let v30 = 0; +let v31 = 0; +let v32 = 0; +let v33 = 0; +let v34 = 0; +let v35 = 0; +let v36 = 0; +let v37 = 0; +let v38 = 0; +let v39 = 0; +let v40 = 0; +let v41 = 0; +let v42 = 0; +let v43 = 0; +let v44 = 0; +let v45 = 0; +let v46 = 0; +let v47 = 0; +let v48 = 0; +let v49 = 0; +let v50 = 0; +let v51 = 0; +let v52 = 0; +let v53 = 0; +let v54 = 0; +let v55 = 0; +let v56 = 0; +let v57 = 0; +let v58 = 0; +let v59 = 0; +let v60 = 0; +let v61 = 0; +let v62 = 0; +let v63 = 0; +let v64 = 0; + +function someUses() { + v1 = 0; + v2 = 0; + v3 = 0; + v4 = 0; + v5 = 0; + v6 = 0; + v7 = 0; + v8 = 0; + v9 = 0; + v10 = 0; + v11 = 0; + v12 = 0; + v13 = 0; + v14 = 0; + v15 = 0; + v16 = 0; + v17 = 0; + v18 = 0; + v19 = 0; + v20 = 0; + v21 = 0; + v22 = 0; + v23 = 0; + v24 = 0; + v25 = 0; + v26 = 0; + v27 = 0; + v28 = 0; + v29 = 0; + v30 = 0; + v31 = 0; + v32 = 0; + v33 = 0; + v34 = 0; + v35 = 0; + v36 = 0; + v37 = 0; + v38 = 0; + v39 = 0; + v40 = 0; + v41 = 0; + v42 = 0; + v43 = 0; + v44 = 0; + v45 = 0; + v46 = 0; + v47 = 0; + v48 = 0; + v49 = 0; + v50 = 0; + v51 = 0; + v52 = 0; + v53 = 0; + v54 = 0; + v55 = 0; + v56 = 0; + v57 = 0; + v58 = 0; + v59 = 0; + v60 = 0; + v61 = 0; + v62 = 0; + v63 = 0; + v64 = 0; +} + +// Make another lexical binding that needs hole checks in the same scope with +// some uses that can be elided. Both the first and second compiles should be +// able to elide the subsequent use. +try { + x = 42; + x = 42; +} catch (e) { +} + +let x = 0;