PR51079: Treat thread_local variables with an incomplete class type as

being not trivially destructible when determining if we can skip calling
their thread wrapper function.

Author: zygoloid

clang/lib/CodeGen/ItaniumCXXABI.cpp

@@ -334,6 +334,19 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
       ArrayRef<llvm::Function *> CXXThreadLocalInits,
       ArrayRef<const VarDecl *> CXXThreadLocalInitVars) override;
 
+  bool mayNeedDestruction(const VarDecl *VD) const {
+    if (VD->needsDestruction(getContext()))
+      return true;
+
+    // If the variable has an incomplete class type (or array thereof), it
+    // might need destruction.
+    const Type *T = VD->getType()->getBaseElementTypeUnsafe();
+    if (T->getAs<RecordType>() && T->isIncompleteType())
+      return true;
+
+    return false;
+  }
+
   /// Determine whether we will definitely emit this variable with a constant
   /// initializer, either because the language semantics demand it or because
   /// we know that the initializer is a constant.
@@ -364,7 +377,7 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
     // If we have the only definition, we don't need a thread wrapper if we
     // will emit the value as a constant.
     if (isUniqueGVALinkage(getContext().GetGVALinkageForVariable(VD)))
-      return !VD->needsDestruction(getContext()) && InitDecl->evaluateValue();
+      return !mayNeedDestruction(VD) && InitDecl->evaluateValue();
 
     // Otherwise, we need a thread wrapper unless we know that every
     // translation unit will emit the value as a constant. We rely on the
@@ -376,7 +389,7 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
 
   bool usesThreadWrapperFunction(const VarDecl *VD) const override {
     return !isEmittedWithConstantInitializer(VD) ||
-           VD->needsDestruction(getContext());
+           mayNeedDestruction(VD);
   }
   LValue EmitThreadLocalVarDeclLValue(CodeGenFunction &CGF, const VarDecl *VD,
                                       QualType LValType) override;
@@ -2963,7 +2976,7 @@ void ItaniumCXXABI::EmitThreadLocalInitFuncs(
     // also when the symbol is weak.
     if (CGM.getTriple().isOSAIX() && VD->hasDefinition() &&
         isEmittedWithConstantInitializer(VD, true) &&
-        !VD->needsDestruction(getContext())) {
+        !mayNeedDestruction(VD)) {
       // Init should be null.  If it were non-null, then the logic above would
       // either be defining the function to be an alias or declaring the
       // function with the expectation that the definition of the variable

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

@@ -57,6 +57,15 @@ int get_c() { return c; }
 
 thread_local int c = 0;
 
+// PR51079: We must assume an incomplete class type might have non-trivial
+// destruction, and so speculatively call the thread wrapper.
+
+// CHECK-LABEL: define {{.*}} @_Z6get_e3v(
+// CHECK: call {{.*}}* @_ZTW2e3()
+// CHECK-LABEL: }
+extern thread_local constinit struct DestructedFwdDecl e3;
+DestructedFwdDecl &get_e3() { return e3; }
+
 int d_init();
 
 // CHECK: define {{.*}}[[D_INIT:@__cxx_global_var_init[^(]*]](
@@ -84,3 +93,11 @@ thread_local constinit int f = 4;
 // CHECK-LABEL: define {{.*}}__tls_init
 // CHECK: call {{.*}} [[D_INIT]]
 // CHECK: call {{.*}} [[E2_INIT]]
+
+// Because the call wrapper may be called speculatively (and simply because
+// it's required by the ABI), it must always be emitted for an external linkage
+// variable, even if the variable has constant initialization and constant
+// destruction.
+struct NotDestructed { int n = 0; };
+thread_local constinit NotDestructed nd;
+// CHECK-LABEL: define {{.*}} @_ZTW2nd