Merged: CLs related to TDZ elision var numbering bug

Merged: [interpreter] Don't number non-lexicals in TDZ elision Revision: 260b62d Merged: [interpreter] Refine hole check numbering for initialization Revision: f72cbd5 Merged: [interpreter] Use |= in Variable::ForceHoleInitialization Revision: dc628cc 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 <[email protected]> Reviewed-by: Leszek Swirski <[email protected]> Cr-Commit-Position: refs/branch-heads/11.5@{v8#16} Cr-Branched-From: 0c4044b-refs/heads/11.5.150@{#1} Cr-Branched-From: b71d303-refs/heads/main@{#87781}
CTSRD-CHERI · Jun 14, 2023 · 50dfeab · 50dfeab
1 parent 44c8785
commit 50dfeab
Show file tree

Hide file tree

Showing 5 changed files with 222 additions and 27 deletions.
diff --git 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
@@ -49,7 +49,8 @@ void Variable::AssignHoleCheckBitmapIndex(ZoneVector<Variable*>& 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
@@ -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<HoleCheckBitmap>::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<Variable*>& 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<Variable*>& list,
                                   uint8_t next_index);
 
@@ -321,11 +348,14 @@ class Variable final : public ZoneObject {
   using ForceContextAllocationBit = LocationField::Next<bool, 1>;
   using IsUsedField = ForceContextAllocationBit::Next<bool, 1>;
   using InitializationFlagField = IsUsedField::Next<InitializationFlag, 1>;
-  using ForceHoleInitializationField = InitializationFlagField::Next<bool, 1>;
   using MaybeAssignedFlagField =
-      ForceHoleInitializationField::Next<MaybeAssignedFlag, 1>;
+      InitializationFlagField::Next<MaybeAssignedFlag, 1>;
   using IsStaticFlagField = MaybeAssignedFlagField::Next<IsStaticFlag, 1>;
 
+  using HoleCheckBitmapIndexField = base::BitField16<uint8_t, 0, 8>;
+  using ForceHoleInitializationFlagField =
+      HoleCheckBitmapIndexField::Next<ForceHoleInitializationFlag, 2>;
+
   Variable** next() { return &next_; }
   friend List;
   friend base::ThreadedListTraits<Variable>;

diff --git 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
@@ -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;