[clang][bytecode] Stack-allocate bottom function frame

[tbaederr]

Instead of heap-allocating it. This is similar to what the current interpeter does. In C, we have no function calls, so the extra heap allocation never makes sense.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Instead of heap-allocating it. This is similar to what the current interpeter does. In C, we have no function calls, so the extra heap allocation never makes sense.

---

Full diff: https://github.com/llvm/llvm-project/pull/125253.diff

6 Files Affected:

• (modified) clang/lib/AST/ByteCode/EvalEmitter.cpp (+3-4)
• (modified) clang/lib/AST/ByteCode/EvalEmitter.h (+2)
• (modified) clang/lib/AST/ByteCode/Interp.h (+2-2)
• (modified) clang/lib/AST/ByteCode/InterpFrame.cpp (+4)
• (modified) clang/lib/AST/ByteCode/InterpFrame.h (+11)
• (modified) clang/lib/AST/ByteCode/InterpState.cpp (+1-1)

diff --git a/clang/lib/AST/ByteCode/EvalEmitter.cpp b/clang/lib/AST/ByteCode/EvalEmitter.cpp
index 9763fe89b73742..3f64aea4183f64 100644
--- a/clang/lib/AST/ByteCode/EvalEmitter.cpp
+++ b/clang/lib/AST/ByteCode/EvalEmitter.cpp
@@ -17,10 +17,9 @@ using namespace clang::interp;
 
 EvalEmitter::EvalEmitter(Context &Ctx, Program &P, State &Parent,
                          InterpStack &Stk)
-    : Ctx(Ctx), P(P), S(Parent, P, Stk, Ctx, this), EvalResult(&Ctx) {
-  // Create a dummy frame for the interpreter which does not have locals.
-  S.Current =
-      new InterpFrame(S, /*Func=*/nullptr, /*Caller=*/nullptr, CodePtr(), 0);
+    : Ctx(Ctx), P(P), S(Parent, P, Stk, Ctx, this), EvalResult(&Ctx),
+      BottomFrame(S) {
+  S.Current = &BottomFrame;
 }
 
 EvalEmitter::~EvalEmitter() {
diff --git a/clang/lib/AST/ByteCode/EvalEmitter.h b/clang/lib/AST/ByteCode/EvalEmitter.h
index e7c9e80d75d934..0b58b100f85fad 100644
--- a/clang/lib/AST/ByteCode/EvalEmitter.h
+++ b/clang/lib/AST/ByteCode/EvalEmitter.h
@@ -125,6 +125,8 @@ class EvalEmitter : public SourceMapper {
   /// Active block which should be executed.
   LabelTy ActiveLabel = 0;
 
+  InterpFrame BottomFrame;
+
 protected:
 #define GET_EVAL_PROTO
 #include "Opcodes.inc"
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 063970afec9e35..91a82a25944fb5 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -325,11 +325,11 @@ bool Ret(InterpState &S, CodePtr &PC) {
 
   if (InterpFrame *Caller = S.Current->Caller) {
     PC = S.Current->getRetPC();
-    delete S.Current;
+    InterpFrame::free(S.Current);
     S.Current = Caller;
     S.Stk.push<T>(Ret);
   } else {
-    delete S.Current;
+    InterpFrame::free(S.Current);
     S.Current = nullptr;
     // The topmost frame should come from an EvalEmitter,
     // which has its own implementation of the Ret<> instruction.
diff --git a/clang/lib/AST/ByteCode/InterpFrame.cpp b/clang/lib/AST/ByteCode/InterpFrame.cpp
index 20f67d9b1fd425..cfcc9dc80d5353 100644
--- a/clang/lib/AST/ByteCode/InterpFrame.cpp
+++ b/clang/lib/AST/ByteCode/InterpFrame.cpp
@@ -23,6 +23,10 @@
 using namespace clang;
 using namespace clang::interp;
 
+InterpFrame::InterpFrame(InterpState &S)
+    : Caller(nullptr), S(S), Depth(0), Func(nullptr), RetPC(CodePtr()),
+      ArgSize(0), Args(nullptr), FrameOffset(0), IsBottom(true) {}
+
 InterpFrame::InterpFrame(InterpState &S, const Function *Func,
                          InterpFrame *Caller, CodePtr RetPC, unsigned ArgSize)
     : Caller(Caller), S(S), Depth(Caller ? Caller->Depth + 1 : 0), Func(Func),
diff --git a/clang/lib/AST/ByteCode/InterpFrame.h b/clang/lib/AST/ByteCode/InterpFrame.h
index 7cfc3ac68b4f3e..8cabb9cd06fac0 100644
--- a/clang/lib/AST/ByteCode/InterpFrame.h
+++ b/clang/lib/AST/ByteCode/InterpFrame.h
@@ -28,6 +28,9 @@ class InterpFrame final : public Frame {
   /// The frame of the previous function.
   InterpFrame *Caller;
 
+  /// Bottom Frame.
+  InterpFrame(InterpState &S);
+
   /// Creates a new frame for a method call.
   InterpFrame(InterpState &S, const Function *Func, InterpFrame *Caller,
               CodePtr RetPC, unsigned ArgSize);
@@ -42,6 +45,11 @@ class InterpFrame final : public Frame {
   /// Destroys the frame, killing all live pointers to stack slots.
   ~InterpFrame();
 
+  static void free(InterpFrame *F) {
+    if (!F->isBottomFrame())
+      delete F;
+  }
+
   /// Invokes the destructors for a scope.
   void destroy(unsigned Idx);
   void initScope(unsigned Idx);
@@ -119,6 +127,8 @@ class InterpFrame final : public Frame {
 
   bool isStdFunction() const;
 
+  bool isBottomFrame() const { return IsBottom; }
+
   void dump() const { dump(llvm::errs(), 0); }
   void dump(llvm::raw_ostream &OS, unsigned Indent = 0) const;
 
@@ -167,6 +177,7 @@ class InterpFrame final : public Frame {
   const size_t FrameOffset;
   /// Mapping from arg offsets to their argument blocks.
   llvm::DenseMap<unsigned, std::unique_ptr<char[]>> Params;
+  bool IsBottom = false;
 };
 
 } // namespace interp
diff --git a/clang/lib/AST/ByteCode/InterpState.cpp b/clang/lib/AST/ByteCode/InterpState.cpp
index 287c3bd3bca3a5..8aa44e48842e96 100644
--- a/clang/lib/AST/ByteCode/InterpState.cpp
+++ b/clang/lib/AST/ByteCode/InterpState.cpp
@@ -27,7 +27,7 @@ bool InterpState::inConstantContext() const {
 }
 
 InterpState::~InterpState() {
-  while (Current) {
+  while (Current && !Current->isBottomFrame()) {
     InterpFrame *Next = Current->Caller;
     delete Current;
     Current = Next;

[slackito]

This is causing use-after-destruction errors for me when running clang tests under MemorySanitizer.

If I understand correctly, after the body of ~EvalEmitter() finishes running, its members are destroyed in reverse order of declaration, and BottomFrame gets destroyed before the destructor for InterpState S; runs.

[vitalybuka]

Some reordering is needed for "InterpState S;" and "InterpFrame BottomFrame;".

[slackito]

The problem described above also reproduced in one of the buildbots: https://lab.llvm.org/buildbot/#/builders/164/builds/6922/steps/17/logs/stdio

I'll revert this patch.

[tbaederr]

Damn, thanks for the revert.

[vitalybuka]

Looks like from this patch https://lab.llvm.org/buildbot/#/builders/94/builds/4194/steps/17/logs/stdio

[vitalybuka]

Looks like from this patch https://lab.llvm.org/buildbot/#/builders/94/builds/4194/steps/17/logs/stdio

I see it's alread reverted and green.
Still the URL with stack traces may help to fix the issue.