[Clang][CodeGen] Fix CanSkipVTablePointerInitialization for dynamic classes with a trivial anonymous union
#84651
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen Author: Max Winkler (MaxEW707) ChangesHit this when trying upgrade an old project of mine. I couldn't find a corresponding existing issue for this when spelunking the open issues here on github. Issue and Historyhttps://godbolt.org/z/EYnhce8MK for reference. Anonymous unions never have their destructor invoked automatically. Therefore we can skip vtable initialization of the destructor of a dynamic class if that destructor effectively does no work. This worked previously as the following check would be hit and return true for the trivial anonymous union, https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGClass.cpp#L1348, resulting in the code skipping vtable initialization. This was broken here 982bbf4 in relation to comments made on this review here https://reviews.llvm.org/D10508. FixesThe check the code is doing is correct however the return value is inverted. We want to return true here since a field with anonymous union never has its destructor invoked and thus effectively has a trivial destructor body from the perspective of requiring vtable init in the parent dynamic class. Also added some extra missing unit tests to test for this use case. Full diff: https://github.com/llvm/llvm-project/pull/84651.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index d18f186ce5b415..ca3ac7142af9c0 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -1395,7 +1395,7 @@ FieldHasTrivialDestructorBody(ASTContext &Context,
// The destructor for an implicit anonymous union member is never invoked.
if (FieldClassDecl->isUnion() && FieldClassDecl->isAnonymousStructOrUnion())
- return false;
+ return true;
return HasTrivialDestructorBody(Context, FieldClassDecl, FieldClassDecl);
}
diff --git a/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp b/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp
index eb8f21b57aa7b6..d99a45869fdb54 100644
--- a/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp
+++ b/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -emit-llvm -o - | FileCheck %s
// See Test9 for test description.
// CHECK: @_ZTTN5Test91BE = linkonce_odr unnamed_addr constant
@@ -198,3 +199,65 @@ struct C : virtual B {
C::~C() {}
}
+
+namespace Test10 {
+
+// Check that we don't initialize the vtable pointer in A::~A(), since the class has an anonymous union which
+// never has its destructor invoked.
+struct A {
+ virtual void f();
+ ~A();
+
+ union
+ {
+ int i;
+ unsigned u;
+ };
+};
+
+// CHECK-LABEL: define{{.*}} void @_ZN6Test101AD2Ev
+// CHECK-NOT: store ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6Test101AE, i32 0, inrange i32 0, i32 2), ptr
+A::~A() {
+}
+
+}
+
+namespace Test11 {
+
+// Check that we don't initialize the vtable pointer in A::~A(), even if the base class has a non trivial destructor.
+struct Field {
+ ~Field();
+};
+
+struct A : public Field {
+ virtual void f();
+ ~A();
+};
+
+// CHECK-LABEL: define{{.*}} void @_ZN6Test111AD2Ev
+// CHECK-NOT: store ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6Test111AE, i32 0, inrange i32 0, i32 2), ptr
+A::~A() {
+}
+
+}
+
+namespace Test12 {
+
+// Check that we don't initialize the vtable pointer in A::~A(), since the class has an anonymous struct with trivial fields.
+struct A {
+ virtual void f();
+ ~A();
+
+ struct
+ {
+ int i;
+ unsigned u;
+ };
+};
+
+// CHECK-LABEL: define{{.*}} void @_ZN6Test121AD2Ev
+// CHECK-NOT: store ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6Test121AE, i32 0, inrange i32 0, i32 2), ptr
+A::~A() {
+}
+
+}
|
|
@llvm/pr-subscribers-clang Author: Max Winkler (MaxEW707) ChangesHit this when trying upgrade an old project of mine. I couldn't find a corresponding existing issue for this when spelunking the open issues here on github. Issue and Historyhttps://godbolt.org/z/EYnhce8MK for reference. Anonymous unions never have their destructor invoked automatically. Therefore we can skip vtable initialization of the destructor of a dynamic class if that destructor effectively does no work. This worked previously as the following check would be hit and return true for the trivial anonymous union, https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGClass.cpp#L1348, resulting in the code skipping vtable initialization. This was broken here 982bbf4 in relation to comments made on this review here https://reviews.llvm.org/D10508. FixesThe check the code is doing is correct however the return value is inverted. We want to return true here since a field with anonymous union never has its destructor invoked and thus effectively has a trivial destructor body from the perspective of requiring vtable init in the parent dynamic class. Also added some extra missing unit tests to test for this use case. Full diff: https://github.com/llvm/llvm-project/pull/84651.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index d18f186ce5b415..ca3ac7142af9c0 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -1395,7 +1395,7 @@ FieldHasTrivialDestructorBody(ASTContext &Context,
// The destructor for an implicit anonymous union member is never invoked.
if (FieldClassDecl->isUnion() && FieldClassDecl->isAnonymousStructOrUnion())
- return false;
+ return true;
return HasTrivialDestructorBody(Context, FieldClassDecl, FieldClassDecl);
}
diff --git a/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp b/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp
index eb8f21b57aa7b6..d99a45869fdb54 100644
--- a/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp
+++ b/clang/test/CodeGenCXX/skip-vtable-pointer-initialization.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -emit-llvm -o - | FileCheck %s
// See Test9 for test description.
// CHECK: @_ZTTN5Test91BE = linkonce_odr unnamed_addr constant
@@ -198,3 +199,65 @@ struct C : virtual B {
C::~C() {}
}
+
+namespace Test10 {
+
+// Check that we don't initialize the vtable pointer in A::~A(), since the class has an anonymous union which
+// never has its destructor invoked.
+struct A {
+ virtual void f();
+ ~A();
+
+ union
+ {
+ int i;
+ unsigned u;
+ };
+};
+
+// CHECK-LABEL: define{{.*}} void @_ZN6Test101AD2Ev
+// CHECK-NOT: store ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6Test101AE, i32 0, inrange i32 0, i32 2), ptr
+A::~A() {
+}
+
+}
+
+namespace Test11 {
+
+// Check that we don't initialize the vtable pointer in A::~A(), even if the base class has a non trivial destructor.
+struct Field {
+ ~Field();
+};
+
+struct A : public Field {
+ virtual void f();
+ ~A();
+};
+
+// CHECK-LABEL: define{{.*}} void @_ZN6Test111AD2Ev
+// CHECK-NOT: store ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6Test111AE, i32 0, inrange i32 0, i32 2), ptr
+A::~A() {
+}
+
+}
+
+namespace Test12 {
+
+// Check that we don't initialize the vtable pointer in A::~A(), since the class has an anonymous struct with trivial fields.
+struct A {
+ virtual void f();
+ ~A();
+
+ struct
+ {
+ int i;
+ unsigned u;
+ };
+};
+
+// CHECK-LABEL: define{{.*}} void @_ZN6Test121AD2Ev
+// CHECK-NOT: store ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN6Test121AE, i32 0, inrange i32 0, i32 2), ptr
+A::~A() {
+}
+
+}
|
|
|
|
Pinging you two for a review. Looking through the history of Feel free to add anyone who may be a better fit. Thanks :). |
| @@ -1,4 +1,5 @@ | |||
| // RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - | FileCheck %s | |||
| // RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -emit-llvm -o - | FileCheck %s | |||
There was a problem hiding this comment.
Not sure we need the new RUN line? There shouldn't be any meaningful difference between Linux and Darwin here.
There was a problem hiding this comment.
Fair point since they are both Itanium ABI. I'll remove the line.
Primary motivation is that after #84651 msan will complain if fields accessed after ~__no_destroy. Previously msan assumed that __obj_ will have own destructor, were we will poison the field, but it never happened before. After #84651 msan will complain on any field access after `~__no_destroy`. As is Msan does validate fields destruction order for classes with trivial destructor. Additionally empty destructor will register __cxa_atexit with -O0. https://gcc.godbolt.org/z/hce587b65 We can not remove the destructor with union where _Tp can have non-trivial destructor. But we can remove destructor if we use in-place new instead of union. https://gcc.godbolt.org/z/Yqxx57eEd - empty even with -O0. Reviewers: #reviewers-libcxx Pull Request: #89882
Primary motivation is that after #84651 msan will complain if fields accessed after ~__no_destroy. Previously msan assumed that __obj_ will have own destructor, were we will poison the field, but it never happened before. After #84651 msan will complain on any field access after `~__no_destroy`. As is Msan does validate fields destruction order for classes with trivial destructor. Additionally empty destructor will register __cxa_atexit with -O0. https://gcc.godbolt.org/z/hce587b65 We can not remove the destructor with union where _Tp can have non-trivial destructor. But we can remove destructor if we use in-place new instead of union. https://gcc.godbolt.org/z/Yqxx57eEd - empty even with -O0. Reviewers: #reviewers-libcxx Pull Request: #89882
Primary motivation is that after #84651 msan will complain if fields accessed after ~__no_destroy. Previously msan assumed that __obj_ will have own destructor, were we will poison the field, but it never happened before. After #84651 msan will complain on any field access after `~__no_destroy`. As is Msan does validate fields destruction order for classes with trivial destructor. Additionally empty destructor will register __cxa_atexit with -O0. https://gcc.godbolt.org/z/hce587b65 We can not remove the destructor with union where _Tp can have non-trivial destructor. But we can remove destructor if we use in-place new instead of union. https://gcc.godbolt.org/z/Yqxx57eEd - empty even with -O0. Reviewers: #reviewers-libcxx Pull Request: #89882
Primary motivation is that after #84651 msan will complain if fields accessed after ~__no_destroy. Previously msan assumed that __obj_ will have own destructor, were we will poison the field, but it never happened before. After #84651 msan will complain on any field access after `~__no_destroy`. As is Msan does validate fields destruction order for classes with trivial destructor. Additionally empty destructor will register __cxa_atexit with -O0. https://gcc.godbolt.org/z/hce587b65 We can not remove the destructor with union where _Tp can have non-trivial destructor. But we can remove destructor if we use in-place new instead of union. https://gcc.godbolt.org/z/Yqxx57eEd - empty even with -O0. Reviewers: #reviewers-libcxx Pull Request: #89882
Primary motivation is that after #84651 msan will complain if fields accessed after ~__no_destroy. Previously msan assumed that __obj_ will have own destructor, were we will poison the field, but it never happened before. After #84651 msan will complain on any field access after `~__no_destroy`. As is Msan does validate fields destruction order for classes with trivial destructor. Additionally empty destructor will register __cxa_atexit with -O0. https://gcc.godbolt.org/z/hce587b65 We can not remove the destructor with union where _Tp can have non-trivial destructor. But we can remove destructor if we use in-place new instead of union. https://gcc.godbolt.org/z/Yqxx57eEd - empty even with -O0. Reviewers: #reviewers-libcxx Pull Request: #89882
Primary motivation: is that after #84651 msan will complain if fields accessed after ~__no_destroy. My understanding of the https://eel.is/c++draft/basic.life#10 Static object with trivial destruction has program lifetime. Static object with empty destuctor has implicit lifetime, and accessing the object after lifetime is UB. It was UB before #84651, it's just msan ignored union members. Existing code with unions uses empty destructor, so accessing after the main() can cause UB. "placement new" version can have trivial destructor, so there is no end of lifetime. Secondary motivation: empty destructor will register __cxa_atexit with -O0. https://gcc.godbolt.org/z/hce587b65 We can not remove the destructor with union where _Tp can have non-trivial destructor. But we can remove destructor if we use in-place new instead of union. https://gcc.godbolt.org/z/Yqxx57eEd - empty even with -O0. New test fails without the patch on https://lab.llvm.org/buildbot/#/builders/sanitizer-x86_64-linux-bootstrap-msan
Hit this when trying upgrade an old project of mine. I couldn't find a corresponding existing issue for this when spelunking the open issues here on github.
Thankfully I can work-around it today with the
[[clang::no_destroy]]attribute for my use case. However it should still be properly fixed.Issue and History
https://godbolt.org/z/EYnhce8MK for reference.
All subsequent text below refers to the example in the godbolt above.
Anonymous unions never have their destructor invoked automatically. Therefore we can skip vtable initialization of the destructor of a dynamic class if that destructor effectively does no work.
This worked previously as the following check would be hit and return true for the trivial anonymous union, https://github.com/llvm/llvm-project/blob/release/18.x/clang/lib/CodeGen/CGClass.cpp#L1348, resulting in the code skipping vtable initialization.
This was broken here 982bbf4 in relation to comments made on this review here https://reviews.llvm.org/D10508.
Fixes
The check the code is doing is correct however the return value is inverted. We want to return true here since a field with anonymous union never has its destructor invoked and thus effectively has a trivial destructor body from the perspective of requiring vtable init in the parent dynamic class.
Also added some extra missing unit tests to test for this use case and a couple others.