Skip to content

[clang][bytecode] Check dtor instance pointers for active-ness #128732

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.

Sign up for GitHub

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
Mar 17, 2025
Merged

[clang][bytecode] Check dtor instance pointers for active-ness #128732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions clang/lib/AST/ByteCode/Compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4787,8 +4787,12 @@ bool Compiler<Emitter>::VisitCallExpr(const CallExpr *E) {
}
// Explicit calls to trivial destructors
if (const auto *DD = dyn_cast_if_present<CXXDestructorDecl>(FuncDecl);
DD && DD->isTrivial())
return true;
DD && DD->isTrivial()) {
const auto *MemberCall = cast<CXXMemberCallExpr>(E);
if (!this->visit(MemberCall->getImplicitObjectArgument()))
return false;
return this->emitCheckDestruction(E) && this->emitPopPtr(E);
}

QualType ReturnType = E->getCallReturnType(Ctx.getASTContext());
std::optional<PrimType> T = classify(ReturnType);
Expand Down Expand Up @@ -5829,6 +5833,9 @@ bool Compiler<Emitter>::compileDestructor(const CXXDestructorDecl *Dtor) {
if (!this->emitThis(Dtor))
return false;

if (!this->emitCheckDestruction(Dtor))
return false;

assert(R);
if (!R->isUnion()) {
// First, destroy all fields.
Expand Down
132 changes: 69 additions & 63 deletions clang/lib/AST/ByteCode/Interp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,68 +203,6 @@ static void diagnoseNonConstVariable(InterpState &S, CodePtr OpPC,
S.Note(VD->getLocation(), diag::note_declared_at);
}

static bool CheckActive(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
AccessKinds AK) {
if (Ptr.isActive())
return true;

assert(Ptr.inUnion());
assert(Ptr.isField() && Ptr.getField());

Pointer U = Ptr.getBase();
Pointer C = Ptr;
while (!U.isRoot() && !U.isActive()) {
// A little arbitrary, but this is what the current interpreter does.
// See the AnonymousUnion test in test/AST/ByteCode/unions.cpp.
// GCC's output is more similar to what we would get without
// this condition.
if (U.getRecord() && U.getRecord()->isAnonymousUnion())
break;

C = U;
U = U.getBase();
}
assert(C.isField());

// Consider:
// union U {
// struct {
// int x;
// int y;
// } a;
// }
//
// When activating x, we will also activate a. If we now try to read
// from y, we will get to CheckActive, because y is not active. In that
// case, our U will be a (not a union). We return here and let later code
// handle this.
if (!U.getFieldDesc()->isUnion())
return true;

// Get the inactive field descriptor.
assert(!C.isActive());
const FieldDecl *InactiveField = C.getField();
assert(InactiveField);

// Find the active field of the union.
const Record *R = U.getRecord();
assert(R && R->isUnion() && "Not a union");

const FieldDecl *ActiveField = nullptr;
for (const Record::Field &F : R->fields()) {
const Pointer &Field = U.atField(F.Offset);
if (Field.isActive()) {
ActiveField = Field.getField();
break;
}
}

const SourceInfo &Loc = S.Current->getSource(OpPC);
S.FFDiag(Loc, diag::note_constexpr_access_inactive_union_member)
<< AK << InactiveField << !ActiveField << ActiveField;
return false;
}

static bool CheckTemporary(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
AccessKinds AK) {
if (auto ID = Ptr.getDeclID()) {
Expand Down Expand Up @@ -376,7 +314,68 @@ bool CheckBCPResult(InterpState &S, const Pointer &Ptr) {

if (const Expr *Base = Ptr.getDeclDesc()->asExpr())
return isa<StringLiteral>(Base);
return false;
}

bool CheckActive(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
AccessKinds AK) {
if (Ptr.isActive())
return true;

assert(Ptr.inUnion());
assert(Ptr.isField() && Ptr.getField());

Pointer U = Ptr.getBase();
Pointer C = Ptr;
while (!U.isRoot() && !U.isActive()) {
// A little arbitrary, but this is what the current interpreter does.
// See the AnonymousUnion test in test/AST/ByteCode/unions.cpp.
// GCC's output is more similar to what we would get without
// this condition.
if (U.getRecord() && U.getRecord()->isAnonymousUnion())
break;

C = U;
U = U.getBase();
}
assert(C.isField());

// Consider:
// union U {
// struct {
// int x;
// int y;
// } a;
// }
//
// When activating x, we will also activate a. If we now try to read
// from y, we will get to CheckActive, because y is not active. In that
// case, our U will be a (not a union). We return here and let later code
// handle this.
if (!U.getFieldDesc()->isUnion())
return true;

// Get the inactive field descriptor.
assert(!C.isActive());
const FieldDecl *InactiveField = C.getField();
assert(InactiveField);

// Find the active field of the union.
const Record *R = U.getRecord();
assert(R && R->isUnion() && "Not a union");

const FieldDecl *ActiveField = nullptr;
for (const Record::Field &F : R->fields()) {
const Pointer &Field = U.atField(F.Offset);
if (Field.isActive()) {
ActiveField = Field.getField();
break;
}
}

const SourceInfo &Loc = S.Current->getSource(OpPC);
S.FFDiag(Loc, diag::note_constexpr_access_inactive_union_member)
<< AK << InactiveField << !ActiveField << ActiveField;
return false;
}

Expand Down Expand Up @@ -1358,6 +1357,11 @@ static bool checkConstructor(InterpState &S, CodePtr OpPC, const Function *Func,
return false;
}

static bool checkDestructor(InterpState &S, CodePtr OpPC, const Function *Func,
const Pointer &ThisPtr) {
return CheckActive(S, OpPC, ThisPtr, AK_Destroy);
}

static void compileFunction(InterpState &S, const Function *Func) {
Compiler<ByteCodeEmitter>(S.getContext(), S.P)
.compileFunc(Func->getDecl(), const_cast<Function *>(Func));
Expand Down Expand Up @@ -1443,13 +1447,15 @@ bool Call(InterpState &S, CodePtr OpPC, const Function *Func,
} else {
if (!CheckInvoke(S, OpPC, ThisPtr))
return cleanup();
if (!Func->isConstructor() &&
if (!Func->isConstructor() && !Func->isDestructor() &&
!CheckActive(S, OpPC, ThisPtr, AK_MemberCall))
return false;
}

if (Func->isConstructor() && !checkConstructor(S, OpPC, Func, ThisPtr))
return false;
if (Func->isDestructor() && !checkDestructor(S, OpPC, Func, ThisPtr))
return false;
}

if (!Func->isFullyCompiled())
Expand Down
8 changes: 8 additions & 0 deletions clang/lib/AST/ByteCode/Interp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ bool CheckNewDeleteForms(InterpState &S, CodePtr OpPC,
bool CheckDeleteSource(InterpState &S, CodePtr OpPC, const Expr *Source,
const Pointer &Ptr);

bool CheckActive(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
AccessKinds AK);

/// Sets the given integral value to the pointer, which is of
/// a std::{weak,partial,strong}_ordering type.
bool SetThreeWayComparisonField(InterpState &S, CodePtr OpPC,
Expand Down Expand Up @@ -3105,6 +3108,11 @@ bool GetTypeid(InterpState &S, CodePtr OpPC, const Type *TypePtr,
bool GetTypeidPtr(InterpState &S, CodePtr OpPC, const Type *TypeInfoType);
bool DiagTypeid(InterpState &S, CodePtr OpPC);

inline bool CheckDestruction(InterpState &S, CodePtr OpPC) {
const auto &Ptr = S.Stk.peek<Pointer>();
return CheckActive(S, OpPC, Ptr, AK_Destroy);
}

//===----------------------------------------------------------------------===//
// Read opcode arguments
//===----------------------------------------------------------------------===//
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/AST/ByteCode/Opcodes.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,3 +865,5 @@ def BitCast : Opcode;
def GetTypeid : Opcode { let Args = [ArgTypePtr, ArgTypePtr]; }
def GetTypeidPtr : Opcode { let Args = [ArgTypePtr]; }
def DiagTypeid : Opcode;

def CheckDestruction : Opcode;
48 changes: 48 additions & 0 deletions clang/test/AST/ByteCode/unions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,4 +522,52 @@ namespace MemberCalls {
static_assert(foo()); // both-error {{not an integral constant expression}} \
// both-note {{in call to}}
}

namespace InactiveDestroy {
struct A {
constexpr ~A() {}
};
union U {
A a;
constexpr ~U() {
Copy link
Collaborator

@shafik shafik Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a test to check cases where inside ~U we have a.~A(); and we attempt to double destruct.

Copy link
Contributor Author

@tbaederr tbaederr Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not related to unions, is it?

}
};

constexpr bool foo() { // both-error {{never produces a constant expression}}
U u;
u.a.~A(); // both-note 2{{destruction of member 'a' of union with no active member}}
return true;
}
static_assert(foo()); // both-error {{not an integral constant expression}} \
// both-note {{in call to}}
}

namespace InactiveTrivialDestroy {
struct A {};
union U {
A a;
};

constexpr bool foo() { // both-error {{never produces a constant expression}}
U u;
u.a.~A(); // both-note 2{{destruction of member 'a' of union with no active member}}
return true;
}
static_assert(foo()); // both-error {{not an integral constant expression}} \
// both-note {{in call to}}
}

namespace ActiveDestroy {
struct A {};
union U {
A a;
};
constexpr bool foo2() {
U u{};
u.a.~A();
return true;
}
static_assert(foo2());
}

#endif