[clang][bytecode] Stack-allocate bottom function frame#125253
[clang][bytecode] Stack-allocate bottom function frame#125253
Conversation
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.
26c07a4 to
797ce10
Compare
|
@llvm/pr-subscribers-clang Author: Timm Baeder (tbaederr) ChangesInstead 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:
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;
|
|
|
||
| InterpState::~InterpState() { | ||
| while (Current) { | ||
| while (Current && !Current->isBottomFrame()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Some reordering is needed for "InterpState S;" and "InterpFrame BottomFrame;".
|
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. |
…)" This reverts commit f354981.
) 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
…rame" (#125325) 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
|
Damn, thanks for the revert. |
|
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. |
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.