[PATCH] D142277: [clang][Interp] Clear metadata when destroying locals

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, tahonermann, shafik.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I've had problems with 5b54cf1a2892767fe949826a32d7820732028a38 <https://reviews.llvm.org/rG5b54cf1a2892767fe949826a32d7820732028a38> because we were emitting a `Destroy` op for a local variable before we were done using that local variable. This moves the local's `Block` to a `DeadBlock` but leaves the metadata (a `InlineDescriptor` in the case of local variables) uninitialized, so we ran into failed builds on the msan builders.

This patch simply zeroes the metadata after calling destroying the local variable. The only difference is that we will now run into an assertion because the `Descriptor*` is `nullptr`, etc.

The added code could be in a `#ifndef _NDEBUG` block, and that would be better for performance of course, but... that wouldn't have helped in the case of the failing msan builders anyway I think.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142277

Files:
  clang/lib/AST/Interp/EvalEmitter.cpp
  clang/lib/AST/Interp/EvalEmitter.h
  clang/lib/AST/Interp/InterpFrame.cpp


Index: clang/lib/AST/Interp/InterpFrame.cpp
===================================================================
--- clang/lib/AST/Interp/InterpFrame.cpp
+++ clang/lib/AST/Interp/InterpFrame.cpp
@@ -75,6 +75,10 @@
 void InterpFrame::destroy(unsigned Idx) {
   for (auto &Local : Func->getScope(Idx).locals()) {
     S.deallocate(reinterpret_cast<Block *>(localBlock(Local.Offset)));
+    // Just make sure we're runnnig into some kind of error when a
+    // local variable is used after being destroyed.
+    InlineDescriptor *ID = localInlineDesc(Local.Offset);
+    std::memset(ID, 0, sizeof(InlineDescriptor));
   }
 }
 
Index: clang/lib/AST/Interp/EvalEmitter.h
===================================================================
--- clang/lib/AST/Interp/EvalEmitter.h
+++ clang/lib/AST/Interp/EvalEmitter.h
@@ -92,6 +92,8 @@
   /// Temporaries which require storage.
   llvm::DenseMap<unsigned, std::unique_ptr<char[]>> Locals;
 
+  InlineDescriptor *localInlineDesc(unsigned LocalIndex);
+
   // The emitter always tracks the current instruction and sets OpPC to a token
   // value which is mapped to the location of the opcode being evaluated.
   CodePtr OpPC;
Index: clang/lib/AST/Interp/EvalEmitter.cpp
===================================================================
--- clang/lib/AST/Interp/EvalEmitter.cpp
+++ clang/lib/AST/Interp/EvalEmitter.cpp
@@ -49,25 +49,33 @@
 
 EvalEmitter::LabelTy EvalEmitter::getLabel() { return NextLabel++; }
 
+InlineDescriptor *EvalEmitter::localInlineDesc(unsigned LocalIndex) {
+  auto It = Locals.find(LocalIndex);
+  assert(It != Locals.end() && "Missing local variable");
+  Block *B = reinterpret_cast<Block *>(It->second.get());
+  return reinterpret_cast<InlineDescriptor *>(B->rawData());
+}
+
 Scope::Local EvalEmitter::createLocal(Descriptor *D) {
   // Allocate memory for a local.
   auto Memory = std::make_unique<char[]>(sizeof(Block) + D->getAllocSize());
   auto *B = new (Memory.get()) Block(D, /*isStatic=*/false);
   B->invokeCtor();
 
-  // Initialize local variable inline descriptor.
-  InlineDescriptor &Desc = *reinterpret_cast<InlineDescriptor *>(B->rawData());
-  Desc.Desc = D;
-  Desc.Offset = sizeof(InlineDescriptor);
-  Desc.IsActive = true;
-  Desc.IsBase = false;
-  Desc.IsMutable = false;
-  Desc.IsConst = false;
-  Desc.IsInitialized = false;
-
   // Register the local.
   unsigned Off = Locals.size();
   Locals.insert({Off, std::move(Memory)});
+
+  // Initialize local variable inline descriptor.
+  InlineDescriptor *Desc = localInlineDesc(Off);
+  Desc->Desc = D;
+  Desc->Offset = sizeof(InlineDescriptor);
+  Desc->IsActive = true;
+  Desc->IsBase = false;
+  Desc->IsMutable = false;
+  Desc->IsConst = false;
+  Desc->IsInitialized = false;
+
   return {Off, D};
 }
 
@@ -240,8 +248,8 @@
   assert(It != Locals.end() && "Missing local variable");
   auto *B = reinterpret_cast<Block *>(It->second.get());
   *reinterpret_cast<T *>(B->data()) = S.Stk.pop<T>();
-  InlineDescriptor &Desc = *reinterpret_cast<InlineDescriptor *>(B->rawData());
-  Desc.IsInitialized = true;
+  InlineDescriptor *Desc = localInlineDesc(I);
+  Desc->IsInitialized = true;
 
   return true;
 }
@@ -254,6 +262,10 @@
     auto It = Locals.find(Local.Offset);
     assert(It != Locals.end() && "Missing local variable");
     S.deallocate(reinterpret_cast<Block *>(It->second.get()));
+    // Just make sure we're runnnig into some kind of error when a
+    // local variable is used after being destroyed.
+    InlineDescriptor *ID = localInlineDesc(I);
+    std::memset(ID, 0, sizeof(InlineDescriptor));
   }
 
   return true;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142277.491053.patch
Type: text/x-patch
Size: 3599 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230121/13097b6f/attachment-0001.bin>