Skip to content

Revert "[clang][bytecode] Stack-allocate bottom function frame" #125325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

slackito
Copy link
Collaborator

@slackito slackito commented Feb 1, 2025

Reverts #125253

It introduced an msan failure. Caught by a buildbot here: https://lab.llvm.org/buildbot/#/builders/164/builds/6922/steps/17/logs/stdio

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 1, 2025

@llvm/pr-subscribers-clang

Author: Jorge Gorbe Moya (slackito)

Changes

Reverts llvm/llvm-project#125253

It introduced an msan failure. Caught by a buildbot here: https://lab.llvm.org/buildbot/#/builders/164/builds/6922/steps/17/logs/stdio


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

6 Files Affected:

  • (modified) clang/lib/AST/ByteCode/EvalEmitter.cpp (+4-3)
  • (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 e857e239560575..8134bbf270363e 100644
--- a/clang/lib/AST/ByteCode/EvalEmitter.cpp
+++ b/clang/lib/AST/ByteCode/EvalEmitter.cpp
@@ -17,9 +17,10 @@ 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),
-      BottomFrame(S) {
-  S.Current = &BottomFrame;
+    : 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);
 }
 
 EvalEmitter::~EvalEmitter() {
diff --git a/clang/lib/AST/ByteCode/EvalEmitter.h b/clang/lib/AST/ByteCode/EvalEmitter.h
index 784553b6c056cf..2cac2ba2ef2212 100644
--- a/clang/lib/AST/ByteCode/EvalEmitter.h
+++ b/clang/lib/AST/ByteCode/EvalEmitter.h
@@ -122,8 +122,6 @@ 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 91a82a25944fb5..063970afec9e35 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();
-    InterpFrame::free(S.Current);
+    delete S.Current;
     S.Current = Caller;
     S.Stk.push<T>(Ret);
   } else {
-    InterpFrame::free(S.Current);
+    delete 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 dff9b211ad198c..48a3db055c6c9b 100644
--- a/clang/lib/AST/ByteCode/InterpFrame.cpp
+++ b/clang/lib/AST/ByteCode/InterpFrame.cpp
@@ -23,10 +23,6 @@
 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 8cabb9cd06fac0..7cfc3ac68b4f3e 100644
--- a/clang/lib/AST/ByteCode/InterpFrame.h
+++ b/clang/lib/AST/ByteCode/InterpFrame.h
@@ -28,9 +28,6 @@ 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);
@@ -45,11 +42,6 @@ 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);
@@ -127,8 +119,6 @@ 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;
 
@@ -177,7 +167,6 @@ 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 8aa44e48842e96..287c3bd3bca3a5 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 && !Current->isBottomFrame()) {
+  while (Current) {
     InterpFrame *Next = Current->Caller;
     delete Current;
     Current = Next;

@slackito slackito merged commit 386af4a into main Feb 1, 2025
11 checks passed
@slackito slackito deleted the revert-125253-bottom-frame branch February 1, 2025 04:09
tbaederr added a commit to tbaederr/llvm-project that referenced this pull request Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants