Skip to content

[clang] Avoid triggering vtable instantiation for C++23 constexpr dtor #102605

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 2 commits into from
Aug 12, 2024
Merged

[clang] Avoid triggering vtable instantiation for C++23 constexpr dtor #102605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb97afb
[clang] Avoid triggering vtable instantiation for C++23 constexpr dtor
Fznamznon Aug 9, 2024
41857fe
Add issue link, merge ifs
Fznamznon Aug 9, 2024
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
29 changes: 28 additions & 1 deletion clang/lib/Sema/SemaDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7042,11 +7042,38 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
}
}

bool EffectivelyConstexprDestructor = true;
// Avoid triggering vtable instantiation due to a dtor that is not
// "effectively constexpr" for better compatibility.
Comment on lines +7046 to +7047
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want more comment here (and/or a reference to the issue)

// See https://github.com/llvm/llvm-project/issues/102293 for more info.
if (isa<CXXDestructorDecl>(M)) {
auto Check = [](QualType T, auto &&Check) -> bool {
const CXXRecordDecl *RD =
T->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
if (!RD || !RD->isCompleteDefinition())
return true;

if (!RD->hasConstexprDestructor())
return false;

for (const CXXBaseSpecifier &B : RD->bases())
if (!Check(B.getType(), Check))
return false;
for (const FieldDecl *FD : RD->fields())
if (!Check(FD->getType(), Check))
return false;
return true;
};
EffectivelyConstexprDestructor =
Check(QualType(Record->getTypeForDecl(), 0), Check);
}

Comment on lines +7049 to +7070
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Record->defaultedDestructorIsConstexpr() would give the right result even if C++23, so I think we can simplify that to CSM != CXXSpecialMemberKind::Destructor || Record->defaultedDestructorIsConstexpr()

Could you try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, unfortunately Record->defaultedDestructorIsConstexpr() returns false for C++23.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, because it's not effectively constexpr :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, woops, I meant it returns true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to this

llvm-project/clang/lib/AST/DeclCXX.cpp

Line 1342 in 52126dc

data().DefaultedDefaultConstructorIsConstexpr =

I think

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. That's unfortunate. Oh well, good enough!

// Define defaulted constexpr virtual functions that override a base class
// function right away.
// FIXME: We can defer doing this until the vtable is marked as used.
if (CSM != CXXSpecialMemberKind::Invalid && !M->isDeleted() &&
M->isDefaulted() && M->isConstexpr() && M->size_overridden_methods())
M->isDefaulted() && M->isConstexpr() && M->size_overridden_methods() &&
EffectivelyConstexprDestructor)
DefineDefaultedFunction(*this, M, M->getLocation());

if (!Incomplete)
Expand Down
22 changes: 22 additions & 0 deletions clang/test/SemaCXX/gh102293.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify %s
// expected-no-diagnostics

template <typename T> static void destroy() {
T t;
++t;
}

struct Incomplete;

template <typename = int> struct HasD {
~HasD() { destroy<Incomplete*>(); }
};

struct HasVT {
virtual ~HasVT();
};

struct S : HasVT {
HasD<> v;
};

Copy link
Contributor

@cor3ntin cor3ntin Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What happen if you do

static_assert((S{}, 1));

(and do we care that it's probably going to complain about the destructor not being defined?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It complains

gh102293.cpp:23:15: error: static assertion expression is not an integral constant expression
   23 | static_assert((S{}, 1));
      |               ^~~~~~~~
gh102293.cpp:23:16: note: non-constexpr function '~HasD' cannot be used in a constant expression
   23 | static_assert((S{}, 1));
      |                ^
gh102293.cpp:23:16: note: in call to 'S{}.~S()'
   23 | static_assert((S{}, 1));
      |                ^
gh102293.cpp:12:3: note: declared here
   12 |   ~HasD() { destroy<Incomplete*>(); }
      |   ^
gh102293.cpp:6:5: error: arithmetic on a pointer to an incomplete type 'Incomplete'
    6 |     ++t;
      |     ^ ~
gh102293.cpp:12:13: note: in instantiation of function template specialization 'destroy<Incomplete *>' requested here
   12 |   ~HasD() { destroy<Incomplete*>(); }
      |             ^
gh102293.cpp:19:8: note: in instantiation of member function 'HasD<>::~HasD' requested here
   19 | struct S : HasVT {
      |        ^
gh102293.cpp:9:8: note: forward declaration of 'Incomplete'
    9 | struct Incomplete;
      |        ^

I suppose expectedly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there NO tests checking that complain?

Loading