Fix handling of constinit thread_locals with a forward-declared type.

[jyknight]

Summary

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.

[rsmith]

Your FIXME is a concern. I think it would be preferable for this function to require as a precondition that the type is complete. If the only call where that isn't true is the call from ItaniumCXXABI::usesThreadWrapperFunction, it seems reasonable to perform a complete-type check there before checking for trivial destructibility. When called from Sema, returning a conservatively-correct value here is likely to paper over bugs where a RequireCompleteType call is missing and a definition could be instantiated.

clang/lib/AST/Type.cpp

case Qualifiers::OCL_Weak:
     return DK_objc_weak_lifetime;
   }
 
   if (const auto *RT =
           type->getBaseElementTypeUnsafe()->getAs<RecordType>()) {
     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())
         return DK_nontrivial_c_struct;
     }
   }
 
   return DK_none;

clang/test/CodeGenCXX/cxx2a-thread-local-constinit.cpp

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -std=c++2a %s -emit-llvm -o - | FileCheck --check-prefix=CHECK --check-prefix=LINUX %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin12  -std=c++2a %s -emit-llvm -o - | FileCheck --check-prefix=CHECK --check-prefix=DARWIN %s
 
 // Check variable definitions/declarations. Note that on Darwin, typically the
 // variable's symbol is marked internal, and only the _ZTW function is
 // exported. Except: constinit variables do get exported, even on darwin.
 
 // CHECK-DAG:  @a = external thread_local global i32
 // CHECK-DAG:  @b = external thread_local global i32
 // LINUX-DAG:  @c = thread_local global i32 0, align 4
 // DARWIN-DAG: @c = internal thread_local global i32 0, align 4
 // 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;
 
 // CHECK-LABEL: define i32 @_Z5get_av()
 // CHECK: call {{(cxx_fast_tlscc )?}}i32* @_ZTW1a()
 // CHECK: }
 int get_a() { return a; }

 };
 
 extern thread_local constinit Destructed e;
 // CHECK-LABEL: define i32 @_Z5get_ev()
 // CHECK: call {{.*}}* @_ZTW1e()
 // 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]]