Reapply "[DebugInfo] Correctly track SDNode dependencies for list debug values"

Fixed memory leak error by using BumpAllocator for SDDbgValue arrays.

This reverts commit 1b58917.

Author: SLTozer

llvm/lib/CodeGen/SelectionDAG/SDNodeDbgValue.h

@@ -131,14 +131,17 @@ class SDDbgOperand {
 /// We do not use SDValue here to avoid including its header.
 class SDDbgValue {
 public:
-  // FIXME: These SmallVector sizes were chosen without any kind of performance
-  // testing.
-  using LocOpVector = SmallVector<SDDbgOperand, 2>;
-  using SDNodeVector = SmallVector<SDNode *, 2>;
 
 private:
-  LocOpVector LocationOps;
-  SDNodeVector SDNodes;
+  // SDDbgValues are allocated by a BumpPtrAllocator, which means the destructor
+  // may not be called; therefore all member arrays must also be allocated by
+  // that BumpPtrAllocator, to ensure that they are correctly freed.
+  size_t NumLocationOps;
+  SDDbgOperand *LocationOps;
+  // SDNode dependencies will be calculated as SDNodes that appear in
+  // LocationOps plus these AdditionalDependencies.
+  size_t NumAdditionalDependencies;
+  SDNode **AdditionalDependencies;
   DIVariable *Var;
   DIExpression *Expr;
   DebugLoc DL;
@@ -149,30 +152,58 @@ class SDDbgValue {
   bool Emitted = false;
 
 public:
-  SDDbgValue(DIVariable *Var, DIExpression *Expr, ArrayRef<SDDbgOperand> L,
-             ArrayRef<SDNode *> Dependencies, bool IsIndirect, DebugLoc DL,
-             unsigned O, bool IsVariadic)
-      : LocationOps(L.begin(), L.end()),
-        SDNodes(Dependencies.begin(), Dependencies.end()), Var(Var), Expr(Expr),
-        DL(DL), Order(O), IsIndirect(IsIndirect), IsVariadic(IsVariadic) {
+  SDDbgValue(BumpPtrAllocator &Alloc, DIVariable *Var, DIExpression *Expr,
+             ArrayRef<SDDbgOperand> L, ArrayRef<SDNode *> Dependencies,
+             bool IsIndirect, DebugLoc DL, unsigned O, bool IsVariadic)
+      : NumLocationOps(L.size()),
+        LocationOps(Alloc.Allocate<SDDbgOperand>(L.size())),
+        NumAdditionalDependencies(Dependencies.size()),
+        AdditionalDependencies(Alloc.Allocate<SDNode *>(Dependencies.size())),
+        Var(Var), Expr(Expr), DL(DL), Order(O), IsIndirect(IsIndirect),
+        IsVariadic(IsVariadic) {
     assert(IsVariadic || L.size() == 1);
     assert(!(IsVariadic && IsIndirect));
+    std::copy(L.begin(), L.end(), LocationOps);
+    std::copy(Dependencies.begin(), Dependencies.end(), AdditionalDependencies);
   }
 
+  // We allocate arrays with the BumpPtrAllocator and never free or copy them,
+  // for LocationOps and AdditionalDependencies, as we never expect to copy or
+  // destroy an SDDbgValue. If we ever start copying or destroying instances, we
+  // should manage the allocated memory appropriately.
+  SDDbgValue(const SDDbgValue &Other) = delete;
+  SDDbgValue &operator=(const SDDbgValue &Other) = delete;
+  ~SDDbgValue() = delete;
+
   /// Returns the DIVariable pointer for the variable.
   DIVariable *getVariable() const { return Var; }
 
   /// Returns the DIExpression pointer for the expression.
   DIExpression *getExpression() const { return Expr; }
 
-  ArrayRef<SDDbgOperand> getLocationOps() const { return LocationOps; }
+  ArrayRef<SDDbgOperand> getLocationOps() const {
+    return ArrayRef<SDDbgOperand>(LocationOps, NumLocationOps);
+  }
 
-  LocOpVector copyLocationOps() const { return LocationOps; }
+  SmallVector<SDDbgOperand> copyLocationOps() const {
+    return SmallVector<SDDbgOperand>(LocationOps, LocationOps + NumLocationOps);
+  }
 
   // Returns the SDNodes which this SDDbgValue depends on.
-  ArrayRef<SDNode *> getSDNodes() const { return SDNodes; }
+  SmallVector<SDNode *> getSDNodes() const {
+    SmallVector<SDNode *> Dependencies;
+    for (SDDbgOperand DbgOp : getLocationOps())
+      if (DbgOp.getKind() == SDDbgOperand::SDNODE)
+        Dependencies.push_back(DbgOp.getSDNode());
+    for (SDNode *Node : getAdditionalDependencies())
+      Dependencies.push_back(Node);
+    return Dependencies;
+  }
 
-  SDNodeVector copySDNodes() const { return SDNodes; }
+  ArrayRef<SDNode *> getAdditionalDependencies() const {
+    return ArrayRef<SDNode *>(AdditionalDependencies,
+                              NumAdditionalDependencies);
+  }
 
   /// Returns whether this is an indirect value.
   bool isIndirect() const { return IsIndirect; }

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

@@ -8583,7 +8583,8 @@ SDDbgValue *SelectionDAG::getDbgValue(DIVariable *Var, DIExpression *Expr,
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
   return new (DbgInfo->getAlloc())
-      SDDbgValue(Var, Expr, SDDbgOperand::fromNode(N, R), N, IsIndirect, DL, O,
+      SDDbgValue(DbgInfo->getAlloc(), Var, Expr, SDDbgOperand::fromNode(N, R),
+                 {}, IsIndirect, DL, O,
                  /*IsVariadic=*/false);
 }
 
@@ -8594,9 +8595,10 @@ SDDbgValue *SelectionDAG::getConstantDbgValue(DIVariable *Var,
                                               const DebugLoc &DL, unsigned O) {
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
-  return new (DbgInfo->getAlloc()) SDDbgValue(
-      Var, Expr, SDDbgOperand::fromConst(C), {}, /*IsIndirect=*/false, DL, O,
-      /*IsVariadic=*/false);
+  return new (DbgInfo->getAlloc())
+      SDDbgValue(DbgInfo->getAlloc(), Var, Expr, SDDbgOperand::fromConst(C), {},
+                 /*IsIndirect=*/false, DL, O,
+                 /*IsVariadic=*/false);
 }
 
 /// FrameIndex
@@ -8620,8 +8622,8 @@ SDDbgValue *SelectionDAG::getFrameIndexDbgValue(DIVariable *Var,
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
   return new (DbgInfo->getAlloc())
-      SDDbgValue(Var, Expr, SDDbgOperand::fromFrameIdx(FI), Dependencies,
-                 IsIndirect, DL, O,
+      SDDbgValue(DbgInfo->getAlloc(), Var, Expr, SDDbgOperand::fromFrameIdx(FI),
+                 Dependencies, IsIndirect, DL, O,
                  /*IsVariadic=*/false);
 }
 
@@ -8632,7 +8634,8 @@ SDDbgValue *SelectionDAG::getVRegDbgValue(DIVariable *Var, DIExpression *Expr,
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
   return new (DbgInfo->getAlloc())
-      SDDbgValue(Var, Expr, SDDbgOperand::fromVReg(VReg), {}, IsIndirect, DL, O,
+      SDDbgValue(DbgInfo->getAlloc(), Var, Expr, SDDbgOperand::fromVReg(VReg),
+                 {}, IsIndirect, DL, O,
                  /*IsVariadic=*/false);
 }
 
@@ -8644,7 +8647,8 @@ SDDbgValue *SelectionDAG::getDbgValueList(DIVariable *Var, DIExpression *Expr,
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
   return new (DbgInfo->getAlloc())
-      SDDbgValue(Var, Expr, Locs, Dependencies, IsIndirect, DL, O, IsVariadic);
+      SDDbgValue(DbgInfo->getAlloc(), Var, Expr, Locs, Dependencies, IsIndirect,
+                 DL, O, IsVariadic);
 }
 
 void SelectionDAG::transferDbgValues(SDValue From, SDValue To,
@@ -8707,12 +8711,10 @@ void SelectionDAG::transferDbgValues(SDValue From, SDValue To,
       Expr = *Fragment;
     }
 
-    auto NewDependencies = Dbg->copySDNodes();
-    std::replace(NewDependencies.begin(), NewDependencies.end(), FromNode,
-                 ToNode);
+    auto AdditionalDependencies = Dbg->getAdditionalDependencies();
     // Clone the SDDbgValue and move it to To.
     SDDbgValue *Clone = getDbgValueList(
-        Var, Expr, NewLocOps, NewDependencies, Dbg->isIndirect(),
+        Var, Expr, NewLocOps, AdditionalDependencies, Dbg->isIndirect(),
         Dbg->getDebugLoc(), std::max(ToNode->getIROrder(), Dbg->getOrder()),
         Dbg->isVariadic());
     ClonedDVs.push_back(Clone);
@@ -8772,11 +8774,9 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
         (void)Changed;
         assert(Changed && "Salvage target doesn't use N");
 
-        auto NewDependencies = DV->copySDNodes();
-        std::replace(NewDependencies.begin(), NewDependencies.end(), &N,
-                     N0.getNode());
+        auto AdditionalDependencies = DV->getAdditionalDependencies();
         SDDbgValue *Clone = getDbgValueList(DV->getVariable(), DIExpr,
-                                            NewLocOps, NewDependencies,
+                                            NewLocOps, AdditionalDependencies,
                                             DV->isIndirect(), DV->getDebugLoc(),
                                             DV->getOrder(), DV->isVariadic());
         ClonedDVs.push_back(Clone);

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

@@ -1302,8 +1302,8 @@ bool SelectionDAGBuilder::handleDebugValue(ArrayRef<const Value *> Values,
                                            bool IsVariadic) {
   if (Values.empty())
     return true;
-  SDDbgValue::LocOpVector LocationOps;
-  SDDbgValue::SDNodeVector Dependencies;
+  SmallVector<SDDbgOperand> LocationOps;
+  SmallVector<SDNode *> Dependencies;
   for (const Value *V : Values) {
     // Constant value.
     if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V) ||
@@ -1331,7 +1331,6 @@ bool SelectionDAGBuilder::handleDebugValue(ArrayRef<const Value *> Values,
       // Only emit func arg dbg value for non-variadic dbg.values for now.
       if (!IsVariadic && EmitFuncArgumentDbgValue(V, Var, Expr, dl, false, N))
         return true;
-      Dependencies.push_back(N.getNode());
       if (auto *FISDN = dyn_cast<FrameIndexSDNode>(N.getNode())) {
         // Construct a FrameIndexDbgValue for FrameIndexSDNodes so we can
         // describe stack slot locations.
@@ -1343,6 +1342,7 @@ bool SelectionDAGBuilder::handleDebugValue(ArrayRef<const Value *> Values,
         //   dbg.value(i32* %px, !"int x", !DIExpression(DW_OP_deref))
         //
         // Both describe the direct values of their associated variables.
+        Dependencies.push_back(N.getNode());
         LocationOps.emplace_back(SDDbgOperand::fromFrameIdx(FISDN->getIndex()));
         continue;
       }