Skip to content
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

[Sema] Instantiate destructors for initialized members #128866

Merged
merged 7 commits into from
Mar 13, 2025

Conversation

momo5502
Copy link
Contributor

@momo5502 momo5502 commented Feb 26, 2025

Initializing fields in a constructor that are part of an anonymous union requires their destructors to be instantiated.

In general, initialized members within non-delegating constructors need their destructor instantiated.

This fixes #93251

Copy link

github-actions bot commented Feb 26, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@momo5502 momo5502 force-pushed the main branch 4 times, most recently from 5b7b08b to 5e5184a Compare February 27, 2025 10:05
@momo5502 momo5502 changed the title Mark union member destructors referenced [Sema] Instantiate destructors for initialized anonymous union fields Feb 27, 2025
@momo5502 momo5502 marked this pull request as ready for review February 27, 2025 12:00
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 27, 2025

@llvm/pr-subscribers-clang

Author: Maurice Heumann (momo5502)

Changes

Initializing fields, that are part of an anonymous union, in a constructor, requires their destructors to be instantiated.

This fixes #93251


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

3 Files Affected:

  • (modified) clang/include/clang/Sema/Sema.h (+2)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+52-35)
  • (added) clang/test/SemaCXX/union-member-destructor.cpp (+14)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 476abe86cb2d2..6d3879985c6ce 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5432,6 +5432,8 @@ class Sema final : public SemaBase {
   void MarkBaseAndMemberDestructorsReferenced(SourceLocation Loc,
                                               CXXRecordDecl *Record);
 
+  void MarkFieldDestructorReferenced(SourceLocation Loc, FieldDecl *Field);
+
   /// Mark destructors of virtual bases of this class referenced. In the Itanium
   /// C++ ABI, this is done when emitting a destructor for any non-abstract
   /// class. In the Microsoft C++ ABI, this is done any time a class's
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 664d48ccbc382..842c4a866fc88 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -5451,10 +5451,23 @@ bool Sema::SetCtorInitializers(CXXConstructorDecl *Constructor, bool AnyErrors,
            NumInitializers * sizeof(CXXCtorInitializer*));
     Constructor->setCtorInitializers(baseOrMemberInitializers);
 
+    SourceLocation Location = Constructor->getLocation();
+
+    for (CXXCtorInitializer *Initializer : Info.AllToInit) {
+      FieldDecl *Field = Initializer->getAnyMember();
+      if (!Field)
+        continue;
+
+      RecordDecl *FieldRecordDecl = Field->getParent();
+      if (!FieldRecordDecl->isUnion() ||
+          !FieldRecordDecl->isAnonymousStructOrUnion())
+        continue;
+
+      MarkFieldDestructorReferenced(Location, Field);
+    }
     // Constructors implicitly reference the base and member
     // destructors.
-    MarkBaseAndMemberDestructorsReferenced(Constructor->getLocation(),
-                                           Constructor->getParent());
+    MarkBaseAndMemberDestructorsReferenced(Location, Constructor->getParent());
   }
 
   return HadError;
@@ -5759,6 +5772,42 @@ void Sema::ActOnMemInitializers(Decl *ConstructorDecl,
   DiagnoseUninitializedFields(*this, Constructor);
 }
 
+void Sema::MarkFieldDestructorReferenced(SourceLocation Location,
+                                         FieldDecl *Field) {
+  if (Field->isInvalidDecl())
+    return;
+
+  // Don't destroy incomplete or zero-length arrays.
+  if (isIncompleteOrZeroLengthArrayType(Context, Field->getType()))
+    return;
+
+  QualType FieldType = Context.getBaseElementType(Field->getType());
+
+  const RecordType *RT = FieldType->getAs<RecordType>();
+  if (!RT)
+    return;
+
+  CXXRecordDecl *FieldClassDecl = cast<CXXRecordDecl>(RT->getDecl());
+  if (FieldClassDecl->isInvalidDecl())
+    return;
+  if (FieldClassDecl->hasIrrelevantDestructor())
+    return;
+  // The destructor for an implicit anonymous union member is never invoked.
+  if (FieldClassDecl->isUnion() && FieldClassDecl->isAnonymousStructOrUnion())
+    return;
+
+  CXXDestructorDecl *Dtor = LookupDestructor(FieldClassDecl);
+  // Dtor might still be missing, e.g because it's invalid.
+  if (!Dtor)
+    return;
+  CheckDestructorAccess(Field->getLocation(), Dtor,
+                        PDiag(diag::err_access_dtor_field)
+                            << Field->getDeclName() << FieldType);
+
+  MarkFunctionReferenced(Location, Dtor);
+  DiagnoseUseOfDecl(Dtor, Location);
+}
+
 void
 Sema::MarkBaseAndMemberDestructorsReferenced(SourceLocation Location,
                                              CXXRecordDecl *ClassDecl) {
@@ -5774,39 +5823,7 @@ Sema::MarkBaseAndMemberDestructorsReferenced(SourceLocation Location,
 
   // Non-static data members.
   for (auto *Field : ClassDecl->fields()) {
-    if (Field->isInvalidDecl())
-      continue;
-
-    // Don't destroy incomplete or zero-length arrays.
-    if (isIncompleteOrZeroLengthArrayType(Context, Field->getType()))
-      continue;
-
-    QualType FieldType = Context.getBaseElementType(Field->getType());
-
-    const RecordType* RT = FieldType->getAs<RecordType>();
-    if (!RT)
-      continue;
-
-    CXXRecordDecl *FieldClassDecl = cast<CXXRecordDecl>(RT->getDecl());
-    if (FieldClassDecl->isInvalidDecl())
-      continue;
-    if (FieldClassDecl->hasIrrelevantDestructor())
-      continue;
-    // The destructor for an implicit anonymous union member is never invoked.
-    if (FieldClassDecl->isUnion() && FieldClassDecl->isAnonymousStructOrUnion())
-      continue;
-
-    CXXDestructorDecl *Dtor = LookupDestructor(FieldClassDecl);
-    // Dtor might still be missing, e.g because it's invalid.
-    if (!Dtor)
-      continue;
-    CheckDestructorAccess(Field->getLocation(), Dtor,
-                          PDiag(diag::err_access_dtor_field)
-                            << Field->getDeclName()
-                            << FieldType);
-
-    MarkFunctionReferenced(Location, Dtor);
-    DiagnoseUseOfDecl(Dtor, Location);
+    MarkFieldDestructorReferenced(Location, Field);
   }
 
   // We only potentially invoke the destructors of potentially constructed
diff --git a/clang/test/SemaCXX/union-member-destructor.cpp b/clang/test/SemaCXX/union-member-destructor.cpp
new file mode 100644
index 0000000000000..ba64983220ea5
--- /dev/null
+++ b/clang/test/SemaCXX/union-member-destructor.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+
+template <class T> struct VSX {
+  ~VSX() { static_assert(sizeof(T) != 4, ""); } // expected-error {{static assertion failed due to requirement 'sizeof(int) != 4':}} \
+                                                // expected-note {{expression evaluates to '4 != 4'}}
+};
+struct VS {
+  union {
+    VSX<int> _Tail;
+  };
+  ~VS() { }
+  VS(short);
+};
+VS::VS(short) : _Tail() { } // expected-note {{in instantiation of member function 'VSX<int>::~VSX' requested here}}

@momo5502 momo5502 force-pushed the main branch 2 times, most recently from 4353dbd to 9749a04 Compare February 28, 2025 08:29
@momo5502 momo5502 force-pushed the main branch 2 times, most recently from 03314e9 to 4215a22 Compare March 3, 2025 09:32
Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM with a couple minor comments

@momo5502
Copy link
Contributor Author

momo5502 commented Mar 4, 2025

@efriedma-quic thank you for your help and the review. I have no merge rights, would you mind merging this for me?

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

This requires a release note, especially b/c this is a conformance fix.

@momo5502 momo5502 changed the title [Sema] Instantiate destructors for initialized anonymous union fields [Sema] Instantiate destructors for initialized members Mar 5, 2025
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Only nits from me, but I think this LG otherwise.

@momo5502 momo5502 force-pushed the main branch 4 times, most recently from 779bcd0 to fa2910b Compare March 6, 2025 09:39
@momo5502 momo5502 force-pushed the main branch 2 times, most recently from f5d4f48 to fd9a337 Compare March 6, 2025 14:38
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

there was a lot of conversation, so give others a day or two before committing this.

@momo5502
Copy link
Contributor Author

momo5502 commented Mar 6, 2025

there was a lot of conversation, so give others a day or two before committing this.

yeah, no worries, I have no merge permissions anyways :D thank you for your review

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

LGTM besides the last comment I made on adding a comment to the test.

@cor3ntin
Copy link
Contributor

@shafik can you merge once you are happy?

@momo5502
Copy link
Contributor Author

momo5502 commented Mar 13, 2025

@shafik can you merge once you are happy?

@shafik has already approved. the only open comment was from you @cor3ntin. judging from your comment here, I assume it's ok to keep the fixme and resolve the comment (if not, feel free to reopen it).

It would be nice to see this getting merged soon, as I need the fix :D

@cor3ntin cor3ntin merged commit cb28ec6 into llvm:main Mar 13, 2025
9 of 11 checks passed
frederik-h pushed a commit to frederik-h/llvm-project that referenced this pull request Mar 18, 2025
Initializing fields, that are part of an anonymous union, in a
constructor, requires their destructors to be instantiated.

In general, initialized members within non-delegating constructors, need
their destructor instantiated.

This fixes llvm#93251
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.

clang-cl /EHa with MSVC std::variant emits linker error LNK2019: unresolved external symbol
7 participants