[clang][Interp] Avoid leaking init maps of local primitive arrays

[tbaeder]

Summary

This shows up when the interpretation is interrupted, in this case because we're reading an uninitialized value, and so we don't get to the destroy instruction for the scope.

This adds a destroyAll() that calls deallocates all the local variables of all the scopes in the function. This works, but I was wondering if it should instead be a cleanupLeftovers() that's only used by the caller _if_ the interpretation wasn't successful.

[erichkeane]

This is REALLY making me concerned again about ownership semantics in the interpreter. I don't have any real concerns with this patch-as-is, but I DO have concerns with the architecture that makes something like this necessary.

[shafik]

This looks good to me but I am wondering how "interpretation is interrupted" can happen.

[tbaeder]

It's interrupted whenever one of the emitXXX() functions returns false, or when one of the function in Interp.h returns false, e.g. when trying to read from a null pointer.

[tbaeder]

Ping

[aaron.ballman]

Generally LGTM though I had a suggestion that might help some of Erich's concerns. FWIW, I share his concerns about memory ownership in the interpreter becoming convoluted

clang/lib/AST/Interp/InterpFrame.h

51–53

Rather than allow anyone with access to the function to call it, would it make sense to put the functionality directly in the destructor? It's not a huge amount of code, and it reduces the cognitive overhead of figuring out who can destroy what.

[tbaeder]

Abandoning this since the approach is not feasible anymore with https://reviews.llvm.org/D145545

clang/lib/AST/Interp/InterpFrame.h

public:
   InterpFrame(InterpState &S, const Function *Func, CodePtr RetPC);
 
   /// Destroys the frame, killing all live pointers to stack slots.
   ~InterpFrame();
 
   /// Invokes the destructors for a scope.
   void destroy(unsigned Idx);
 
+  /// Invokes the destructors for all scopes. This is used in the
+  /// InterpFrame destructor to avoid memory leaks in case the
+  /// interpretation was not successful.
+  void destroyAll();
+
   /// Pops the arguments off the stack.
   void popArgs();
 
   /// Describes the frame with arguments for diagnostic purposes.
   void describe(llvm::raw_ostream &OS) override;
 
   /// Returns the parent frame object.
   Frame *getCaller() const override;

clang/lib/AST/Interp/InterpFrame.cpp

if (Func->hasThisPointer()) {
     if (Func->hasRVO())
       This = stackRef<Pointer>(sizeof(Pointer));
     else
       This = stackRef<Pointer>(0);
   }
 }
 
 InterpFrame::~InterpFrame() {
+  destroyAll();
   for (auto &Param : Params)
     S.deallocate(reinterpret_cast<Block *>(Param.second.get()));
 }
 
 void InterpFrame::destroy(unsigned Idx) {
   for (auto &Local : Func->getScope(Idx).locals()) {
     S.deallocate(reinterpret_cast<Block *>(localBlock(Local.Offset)));
   }
 }
 
+void InterpFrame::destroyAll() {
+  if (!Func)
+    return;
+  for (const Scope &Sc : Func->scopes()) {
+    for (auto &Local : Sc.locals()) {
+      S.destroy(reinterpret_cast<Block *>(localBlock(Local.Offset)));
+    }
+  }
+}
+
 void InterpFrame::popArgs() {
   for (PrimType Ty : Func->args_reverse())
     TYPE_SWITCH(Ty, S.Stk.discard<T>());
 }
 
 template <typename T>
 static void print(llvm::raw_ostream &OS, const T &V, ASTContext &, QualType) {
   OS << V;

clang/lib/AST/Interp/InterpState.h

public:
   void setFoldFailureDiagnostic(bool Flag) override {
     Parent.setFoldFailureDiagnostic(Flag);
   }
   bool hasPriorDiagnostic() override { return Parent.hasPriorDiagnostic(); }
 
   /// Reports overflow and return true if evaluation should continue.
   bool reportOverflow(const Expr *E, const llvm::APSInt &Value);
 
-  /// Deallocates a pointer.
+  /// Deallocates a block.
   void deallocate(Block *B);
 
+  /// Destroys a block, used during tear down.
+  void destroy(Block *B);
+
   /// Delegates source mapping to the mapper.
   SourceInfo getSource(const Function *F, CodePtr PC) const override {
     return M ? M->getSource(F, PC) : F->getSource(PC);
   }
 
 private:
   /// AST Walker state.
   State &Parent;

clang/lib/AST/Interp/InterpState.cpp

 }
 
 bool InterpState::reportOverflow(const Expr *E, const llvm::APSInt &Value) {
   QualType Type = E->getType();
   CCEDiag(E, diag::note_constexpr_overflow) << Value << Type;
   return noteUndefinedBehavior();
 }
 
+void InterpState::destroy(Block *B) {
+  assert(B);
+  Descriptor *Desc = B->getDescriptor();
+  assert(Desc);
+  // Free storage, if necessary.
+  if (Desc->DtorFn)
+    Desc->DtorFn(B, B->data(), Desc);
+}
+
 void InterpState::deallocate(Block *B) {
   Descriptor *Desc = B->getDescriptor();
   if (B->hasPointers()) {
     size_t Size = B->getSize();
 
     // Allocate a new block, transferring over pointers.
     char *Memory = reinterpret_cast<char *>(malloc(sizeof(DeadBlock) + Size));
     auto *D = new (Memory) DeadBlock(DeadBlocks, B);
 
     // Move data from one block to another.
     if (Desc->MoveFn)
       Desc->MoveFn(B, B->data(), D->data(), Desc);
   } else {
     // Free storage, if necessary.
-    if (Desc->DtorFn)
-      Desc->DtorFn(B, B->data(), Desc);
+    destroy(B);
   }
 }

clang/test/AST/Interp/cxx20.cpp

namespace Destructors {
   }
    constexpr int testInc2() {
     int i = 0;
     doInc2(i);
     return i;
   }
   static_assert(testInc2() == 1, "");
 };
+
+constexpr int NoLeakHere() {
+  int abc[2];
+
+  abc[0] = 1;
+  return abc[1]; // expected-note {{read of object outside its lifetime}} \
+                 // ref-note {{read of uninitialized object}}
+}
+static_assert(NoLeakHere() == 3); // expected-error {{not an integral constant expression}} \
+                                  // expected-note {{in call to}} \
+                                  // ref-error {{not an integral constant expression}} \
+                                  // ref-note {{in call to}}