Skip to content

Commit ca1bde0

Browse files
tbaederrtbaederr
authored
[clang][bytecode] Check dtor instance pointers for active-ness (#128732)
And diagnose if we're trying to destroy an inactive member of a union.
1 parent c53caae commit ca1bde0

File tree

5 files changed

+136
-65
lines changed

5 files changed

+136
-65
lines changed

‎clang/lib/AST/ByteCode/Compiler.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4787,8 +4787,12 @@ bool Compiler<Emitter>::VisitCallExpr(const CallExpr *E) {
47874787
}
47884788
// Explicit calls to trivial destructors
47894789
if (const auto *DD = dyn_cast_if_present<CXXDestructorDecl>(FuncDecl);
4790-
DD && DD->isTrivial())
4791-
return true;
4790+
DD && DD->isTrivial()) {
4791+
const auto *MemberCall = cast<CXXMemberCallExpr>(E);
4792+
if (!this->visit(MemberCall->getImplicitObjectArgument()))
4793+
return false;
4794+
return this->emitCheckDestruction(E) && this->emitPopPtr(E);
4795+
}
47924796

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

5836+
if (!this->emitCheckDestruction(Dtor))
5837+
return false;
5838+
58325839
assert(R);
58335840
if (!R->isUnion()) {
58345841
// First, destroy all fields.

‎clang/lib/AST/ByteCode/Interp.cpp

Lines changed: 69 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -203,68 +203,6 @@ static void diagnoseNonConstVariable(InterpState &S, CodePtr OpPC,
203203
S.Note(VD->getLocation(), diag::note_declared_at);
204204
}
205205

206-
static bool CheckActive(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
207-
AccessKinds AK) {
208-
if (Ptr.isActive())
209-
return true;
210-
211-
assert(Ptr.inUnion());
212-
assert(Ptr.isField() && Ptr.getField());
213-
214-
Pointer U = Ptr.getBase();
215-
Pointer C = Ptr;
216-
while (!U.isRoot() && !U.isActive()) {
217-
// A little arbitrary, but this is what the current interpreter does.
218-
// See the AnonymousUnion test in test/AST/ByteCode/unions.cpp.
219-
// GCC's output is more similar to what we would get without
220-
// this condition.
221-
if (U.getRecord() && U.getRecord()->isAnonymousUnion())
222-
break;
223-
224-
C = U;
225-
U = U.getBase();
226-
}
227-
assert(C.isField());
228-
229-
// Consider:
230-
// union U {
231-
// struct {
232-
// int x;
233-
// int y;
234-
// } a;
235-
// }
236-
//
237-
// When activating x, we will also activate a. If we now try to read
238-
// from y, we will get to CheckActive, because y is not active. In that
239-
// case, our U will be a (not a union). We return here and let later code
240-
// handle this.
241-
if (!U.getFieldDesc()->isUnion())
242-
return true;
243-
244-
// Get the inactive field descriptor.
245-
assert(!C.isActive());
246-
const FieldDecl *InactiveField = C.getField();
247-
assert(InactiveField);
248-
249-
// Find the active field of the union.
250-
const Record *R = U.getRecord();
251-
assert(R && R->isUnion() && "Not a union");
252-
253-
const FieldDecl *ActiveField = nullptr;
254-
for (const Record::Field &F : R->fields()) {
255-
const Pointer &Field = U.atField(F.Offset);
256-
if (Field.isActive()) {
257-
ActiveField = Field.getField();
258-
break;
259-
}
260-
}
261-
262-
const SourceInfo &Loc = S.Current->getSource(OpPC);
263-
S.FFDiag(Loc, diag::note_constexpr_access_inactive_union_member)
264-
<< AK << InactiveField << !ActiveField << ActiveField;
265-
return false;
266-
}
267-
268206
static bool CheckTemporary(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
269207
AccessKinds AK) {
270208
if (auto ID = Ptr.getDeclID()) {
@@ -376,7 +314,68 @@ bool CheckBCPResult(InterpState &S, const Pointer &Ptr) {
376314

377315
if (const Expr *Base = Ptr.getDeclDesc()->asExpr())
378316
return isa<StringLiteral>(Base);
317+
return false;
318+
}
319+
320+
bool CheckActive(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
321+
AccessKinds AK) {
322+
if (Ptr.isActive())
323+
return true;
379324

325+
assert(Ptr.inUnion());
326+
assert(Ptr.isField() && Ptr.getField());
327+
328+
Pointer U = Ptr.getBase();
329+
Pointer C = Ptr;
330+
while (!U.isRoot() && !U.isActive()) {
331+
// A little arbitrary, but this is what the current interpreter does.
332+
// See the AnonymousUnion test in test/AST/ByteCode/unions.cpp.
333+
// GCC's output is more similar to what we would get without
334+
// this condition.
335+
if (U.getRecord() && U.getRecord()->isAnonymousUnion())
336+
break;
337+
338+
C = U;
339+
U = U.getBase();
340+
}
341+
assert(C.isField());
342+
343+
// Consider:
344+
// union U {
345+
// struct {
346+
// int x;
347+
// int y;
348+
// } a;
349+
// }
350+
//
351+
// When activating x, we will also activate a. If we now try to read
352+
// from y, we will get to CheckActive, because y is not active. In that
353+
// case, our U will be a (not a union). We return here and let later code
354+
// handle this.
355+
if (!U.getFieldDesc()->isUnion())
356+
return true;
357+
358+
// Get the inactive field descriptor.
359+
assert(!C.isActive());
360+
const FieldDecl *InactiveField = C.getField();
361+
assert(InactiveField);
362+
363+
// Find the active field of the union.
364+
const Record *R = U.getRecord();
365+
assert(R && R->isUnion() && "Not a union");
366+
367+
const FieldDecl *ActiveField = nullptr;
368+
for (const Record::Field &F : R->fields()) {
369+
const Pointer &Field = U.atField(F.Offset);
370+
if (Field.isActive()) {
371+
ActiveField = Field.getField();
372+
break;
373+
}
374+
}
375+
376+
const SourceInfo &Loc = S.Current->getSource(OpPC);
377+
S.FFDiag(Loc, diag::note_constexpr_access_inactive_union_member)
378+
<< AK << InactiveField << !ActiveField << ActiveField;
380379
return false;
381380
}
382381

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

1360+
static bool checkDestructor(InterpState &S, CodePtr OpPC, const Function *Func,
1361+
const Pointer &ThisPtr) {
1362+
return CheckActive(S, OpPC, ThisPtr, AK_Destroy);
1363+
}
1364+
13611365
static void compileFunction(InterpState &S, const Function *Func) {
13621366
Compiler<ByteCodeEmitter>(S.getContext(), S.P)
13631367
.compileFunc(Func->getDecl(), const_cast<Function *>(Func));
@@ -1443,13 +1447,15 @@ bool Call(InterpState &S, CodePtr OpPC, const Function *Func,
14431447
} else {
14441448
if (!CheckInvoke(S, OpPC, ThisPtr))
14451449
return cleanup();
1446-
if (!Func->isConstructor() &&
1450+
if (!Func->isConstructor() && !Func->isDestructor() &&
14471451
!CheckActive(S, OpPC, ThisPtr, AK_MemberCall))
14481452
return false;
14491453
}
14501454

14511455
if (Func->isConstructor() && !checkConstructor(S, OpPC, Func, ThisPtr))
14521456
return false;
1457+
if (Func->isDestructor() && !checkDestructor(S, OpPC, Func, ThisPtr))
1458+
return false;
14531459
}
14541460

14551461
if (!Func->isFullyCompiled())

‎clang/lib/AST/ByteCode/Interp.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,9 @@ bool CheckNewDeleteForms(InterpState &S, CodePtr OpPC,
137137
bool CheckDeleteSource(InterpState &S, CodePtr OpPC, const Expr *Source,
138138
const Pointer &Ptr);
139139

140+
bool CheckActive(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
141+
AccessKinds AK);
142+
140143
/// Sets the given integral value to the pointer, which is of
141144
/// a std::{weak,partial,strong}_ordering type.
142145
bool SetThreeWayComparisonField(InterpState &S, CodePtr OpPC,
@@ -3105,6 +3108,11 @@ bool GetTypeid(InterpState &S, CodePtr OpPC, const Type *TypePtr,
31053108
bool GetTypeidPtr(InterpState &S, CodePtr OpPC, const Type *TypeInfoType);
31063109
bool DiagTypeid(InterpState &S, CodePtr OpPC);
31073110

3111+
inline bool CheckDestruction(InterpState &S, CodePtr OpPC) {
3112+
const auto &Ptr = S.Stk.peek<Pointer>();
3113+
return CheckActive(S, OpPC, Ptr, AK_Destroy);
3114+
}
3115+
31083116
//===----------------------------------------------------------------------===//
31093117
// Read opcode arguments
31103118
//===----------------------------------------------------------------------===//

‎clang/lib/AST/ByteCode/Opcodes.td

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -865,3 +865,5 @@ def BitCast : Opcode;
865865
def GetTypeid : Opcode { let Args = [ArgTypePtr, ArgTypePtr]; }
866866
def GetTypeidPtr : Opcode { let Args = [ArgTypePtr]; }
867867
def DiagTypeid : Opcode;
868+
869+
def CheckDestruction : Opcode;

‎clang/test/AST/ByteCode/unions.cpp

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -522,4 +522,52 @@ namespace MemberCalls {
522522
static_assert(foo()); // both-error {{not an integral constant expression}} \
523523
// both-note {{in call to}}
524524
}
525+
526+
namespace InactiveDestroy {
527+
struct A {
528+
constexpr ~A() {}
529+
};
530+
union U {
531+
A a;
532+
constexpr ~U() {
533+
}
534+
};
535+
536+
constexpr bool foo() { // both-error {{never produces a constant expression}}
537+
U u;
538+
u.a.~A(); // both-note 2{{destruction of member 'a' of union with no active member}}
539+
return true;
540+
}
541+
static_assert(foo()); // both-error {{not an integral constant expression}} \
542+
// both-note {{in call to}}
543+
}
544+
545+
namespace InactiveTrivialDestroy {
546+
struct A {};
547+
union U {
548+
A a;
549+
};
550+
551+
constexpr bool foo() { // both-error {{never produces a constant expression}}
552+
U u;
553+
u.a.~A(); // both-note 2{{destruction of member 'a' of union with no active member}}
554+
return true;
555+
}
556+
static_assert(foo()); // both-error {{not an integral constant expression}} \
557+
// both-note {{in call to}}
558+
}
559+
560+
namespace ActiveDestroy {
561+
struct A {};
562+
union U {
563+
A a;
564+
};
565+
constexpr bool foo2() {
566+
U u{};
567+
u.a.~A();
568+
return true;
569+
}
570+
static_assert(foo2());
571+
}
572+
525573
#endif

0 commit comments

Comments
 (0)