[PATCH] D81313: Fix handling of constinit thread_locals with a forward-declared type.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

ItaniumCXXABI::usesThreadWrapperFunction calls
VarDecl::needsDestruction, which calls QualType::isDestructedType,
which checks CXXRecordDecl::hasTrivialDestructor -- but only if the
type has a definition.

Most (maybe all other?) callers of isDestructedType call it on a
complete type, but in this particular codepath, it can be called on an
incomplete type, and thus return false for a type which, when the
definition is available, does turn out to have a non-trivial
destructor.

FIXME: I'm not sure if this fix is the correct fix -- whether this
function ought to be changed like this, or if the fix belongs
somewhere else.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81313

Files:
  clang/lib/AST/Type.cpp
  clang/test/CodeGenCXX/cxx2a-thread-local-constinit.cpp


Index: clang/test/CodeGenCXX/cxx2a-thread-local-constinit.cpp
===================================================================
--- clang/test/CodeGenCXX/cxx2a-thread-local-constinit.cpp
+++ clang/test/CodeGenCXX/cxx2a-thread-local-constinit.cpp
@@ -12,8 +12,9 @@
 // LINUX-DAG:  @d = thread_local global i32 0, align 4
 // DARWIN-DAG: @d = internal thread_local global i32 0, align 4
 // CHECK-DAG:  @e = external thread_local global %struct.Destructed, align 4
-// CHECK-DAG:  @e2 = thread_local global %struct.Destructed zeroinitializer, align 4
-// CHECK-DAG:  @f = thread_local global i32 4, align 4
+// CHECK-DAG:  @f = external thread_local global %struct.MaybeDestructed, align 1
+// CHECK-DAG:  @g = thread_local global %struct.Destructed zeroinitializer, align 4
+// CHECK-DAG:  @h = thread_local global i32 4, align 4
 
 extern thread_local int a;
 extern thread_local constinit int b;
@@ -74,13 +75,20 @@
 // CHECK: }
 int get_e() { return e.n; }
 
-// CHECK: define {{.*}}[[E2_INIT:@__cxx_global_var_init[^(]*]](
-// LINUX: call {{.*}} @__cxa_thread_atexit({{.*}} @_ZN10DestructedD1Ev {{.*}} @e2
-// DARWIN: call {{.*}} @_tlv_atexit({{.*}} @_ZN10DestructedD1Ev {{.*}} @e2
-thread_local constinit Destructed e2;
+struct MaybeDestructed; // forward-declared, can't tell if it has a destructor.
+extern thread_local constinit MaybeDestructed f;
+// CHECK-LABEL: define %struct.MaybeDestructed* @_Z5get_fv()
+// CHECK: call {{.*}}* @_ZTW1f()
+// CHECK: }
+MaybeDestructed* get_f() { return &f; }
+
+// CHECK: define {{.*}}[[G_INIT:@__cxx_global_var_init[^(]*]](
+// LINUX: call {{.*}} @__cxa_thread_atexit({{.*}} @_ZN10DestructedD1Ev {{.*}} @g
+// DARWIN: call {{.*}} @_tlv_atexit({{.*}} @_ZN10DestructedD1Ev {{.*}} @g
+thread_local constinit Destructed g;
 
-thread_local constinit int f = 4;
+thread_local constinit int h = 4;
 
 // CHECK-LABEL: define {{.*}}__tls_init
 // CHECK: call {{.*}} [[D_INIT]]
-// CHECK: call {{.*}} [[E2_INIT]]
+// CHECK: call {{.*}} [[G_INIT]]
Index: clang/lib/AST/Type.cpp
===================================================================
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -4287,9 +4287,14 @@
     const RecordDecl *RD = RT->getDecl();
     if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
       /// Check if this is a C++ object with a non-trivial destructor.
-      if (CXXRD->hasDefinition() && !CXXRD->hasTrivialDestructor())
+      /// If the type is only forward-declared, assume it might need destruction.
+      if (!CXXRD->hasDefinition() || !CXXRD->hasTrivialDestructor())
         return DK_cxx_destructor;
     } else {
+      // FIXME: should this also return true for forward declared types?  It
+      // seems a bit weird, as C doesn't support destructors...but objc-arc now
+      // does.
+
       /// Check if this is a C struct that is non-trivial to destroy or an array
       /// that contains such a struct.
       if (RD->isNonTrivialToPrimitiveDestroy())


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81313.268955.patch
Type: text/x-patch
Size: 2955 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200605/33bc60e6/attachment.bin>