Enable -Wunique-object-duplication inside templated code

clang/include/clang/Sema/Sema.h

@@ -3669,6 +3669,7 @@ class Sema final : public SemaBase {
   /// cause problems if the variable is mutable, its initialization is
   /// effectful, or its address is taken.
   bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *Dcl);
+  void DiagnoseDangerousUniqueObjectDuplication(const VarDecl *Dcl);
 
   /// AddInitializerToDecl - Adds the initializer Init to the
   /// declaration dcl. If DirectInit is true, this is C++ direct

clang/lib/Sema/SemaDecl.cpp

@@ -13399,8 +13399,11 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
   // about the properties of the function containing it.
   const ValueDecl *Target = Dcl;
   // VarDecls and FunctionDecls have different functions for checking
-  // inline-ness, so we have to do it manually.
+  // inline-ness, and whether they were originally templated, so we have to
+  // call the appropriate functions manually.
   bool TargetIsInline = Dcl->isInline();
+  bool TargetWasTemplated =
+      Dcl->getTemplateSpecializationKind() != TSK_Undeclared;
 
   // Update the Target and TargetIsInline property if necessary
   if (Dcl->isStaticLocal()) {
@@ -13416,12 +13419,13 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
     Target = FunDcl;
     // IsInlined() checks for the C++ inline property
     TargetIsInline = FunDcl->isInlined();
+    TargetWasTemplated =
+        FunDcl->getTemplateSpecializationKind() != TSK_Undeclared;
   }
 
-  // Non-inline variables can only legally appear in one TU
-  // FIXME: This also applies to templated variables, but that can rarely lead
-  // to false positives so templates are disabled for now.
-  if (!TargetIsInline)
+  // Non-inline functions/variables can only legally appear in one TU,
+  // unless they were part of a template.
+  if (!TargetIsInline && !TargetWasTemplated)
     return false;
 
   // If the object isn't hidden, the dynamic linker will prevent duplication.
@@ -13436,6 +13440,55 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
   return true;
 }
 
+void Sema::DiagnoseDangerousUniqueObjectDuplication(const VarDecl *VD) {
+  // If this object has external linkage and hidden visibility, it might be
+  // duplicated when built into a shared library, which causes problems if it's
+  // mutable (since the copies won't be in sync) or its initialization has side
+  // effects (since it will run once per copy instead of once globally).
+  // FIXME: Windows uses dllexport/dllimport instead of visibility, and we don't
+  // handle that yet. Disable the warning on Windows for now.
+
+  // Don't diagnose if we're inside a template;
+  // we'll diagnose during instantiation instead.
+  if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
+      !VD->isTemplated() &&
+      GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
+
+    // Check mutability. For pointers, ensure that both the pointer and the
+    // pointee are (recursively) const.
+    QualType Type = VD->getType().getNonReferenceType();
+    if (!Type.isConstant(VD->getASTContext())) {
+      Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
+          << VD;
+    } else {
+      while (Type->isPointerType()) {
+        Type = Type->getPointeeType();
+        if (Type->isFunctionType())
+          break;
+        if (!Type.isConstant(VD->getASTContext())) {
+          Diag(VD->getLocation(),
+               diag::warn_possible_object_duplication_mutable)
+              << VD;
+          break;
+        }
+      }
+    }
+
+    // To keep false positives low, only warn if we're certain that the
+    // initializer has side effects. Don't warn on operator new, since a mutable
+    // pointer will trigger the previous warning, and an immutable pointer
+    // getting duplicated just results in a little extra memory usage.
+    const Expr *Init = VD->getAnyInitializer();
+    if (Init &&
+        Init->HasSideEffects(VD->getASTContext(),
+                             /*IncludePossibleEffects=*/false) &&
+        !isa<CXXNewExpr>(Init->IgnoreParenImpCasts())) {
+      Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init)
+          << VD;
+    }
+  }
+}
+
 void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
   // If there is no declaration, there was an error parsing it.  Just ignore
   // the initializer.
@@ -14655,6 +14708,8 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
     return;
   }
 
+  DiagnoseDangerousUniqueObjectDuplication(var);
+
   // Require the destructor.
   if (!type->isDependentType())
     if (const RecordType *recordType = baseType->getAs<RecordType>())
@@ -14842,51 +14897,6 @@ void Sema::FinalizeDeclaration(Decl *ThisDecl) {
   if (DC->getRedeclContext()->isFileContext() && VD->isExternallyVisible())
     AddPushedVisibilityAttribute(VD);
 
-  // If this object has external linkage and hidden visibility, it might be
-  // duplicated when built into a shared library, which causes problems if it's
-  // mutable (since the copies won't be in sync) or its initialization has side
-  // effects (since it will run once per copy instead of once globally)
-  // FIXME: Windows uses dllexport/dllimport instead of visibility, and we don't
-  // handle that yet. Disable the warning on Windows for now.
-  // FIXME: Checking templates can cause false positives if the template in
-  // question is never instantiated (e.g. only specialized templates are used).
-  if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
-      !VD->isTemplated() &&
-      GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
-    // Check mutability. For pointers, ensure that both the pointer and the
-    // pointee are (recursively) const.
-    QualType Type = VD->getType().getNonReferenceType();
-    if (!Type.isConstant(VD->getASTContext())) {
-      Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
-          << VD;
-    } else {
-      while (Type->isPointerType()) {
-        Type = Type->getPointeeType();
-        if (Type->isFunctionType())
-          break;
-        if (!Type.isConstant(VD->getASTContext())) {
-          Diag(VD->getLocation(),
-               diag::warn_possible_object_duplication_mutable)
-              << VD;
-          break;
-        }
-      }
-    }
-
-    // To keep false positives low, only warn if we're certain that the
-    // initializer has side effects. Don't warn on operator new, since a mutable
-    // pointer will trigger the previous warning, and an immutable pointer
-    // getting duplicated just results in a little extra memory usage.
-    const Expr *Init = VD->getAnyInitializer();
-    if (Init &&
-        Init->HasSideEffects(VD->getASTContext(),
-                             /*IncludePossibleEffects=*/false) &&
-        !isa<CXXNewExpr>(Init->IgnoreParenImpCasts())) {
-      Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init)
-          << VD;
-    }
-  }
-
   // FIXME: Warn on unused var template partial specializations.
   if (VD->isFileVarDecl() && !isa<VarTemplatePartialSpecializationDecl>(VD))
     MarkUnusedFileScopedDecl(VD);

clang/test/SemaCXX/unique_object_duplication.h

@@ -154,4 +154,89 @@ namespace GlobalTest {
   };
 
   inline float Test::disallowedStaticMember2 = 2.3; // hidden-warning {{'disallowedStaticMember2' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
-} // namespace GlobalTest
+} // namespace GlobalTest
+
+/******************************************************************************
+ * Case three: Inside templates
+ ******************************************************************************/
+
+namespace TemplateTest {
+
+template <typename T>
+int disallowedTemplate1 = 0; // hidden-warning {{'disallowedTemplate1<int>' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+
+template int disallowedTemplate1<int>; // hidden-note {{in instantiation of}}
+
+
+// Should work for implicit instantiation as well
+template <typename T>
+int disallowedTemplate2 = 0; // hidden-warning {{'disallowedTemplate2<int>' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+
+int implicit_instantiate() {
+  return disallowedTemplate2<int>; // hidden-note {{in instantiation of}}
+}
+
+
+// Ensure we only get warnings for templates that are actually instantiated
+template <typename T>
+int maybeAllowedTemplate = 0; // Not instantiated, so no warning here
+
+template <typename T>
+int maybeAllowedTemplate<T*> = 1; // hidden-warning {{'maybeAllowedTemplate<int *>' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+
+template <>
+int maybeAllowedTemplate<bool> = 2; // hidden-warning {{'maybeAllowedTemplate<bool>' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+
+template int maybeAllowedTemplate<int*>; // hidden-note {{in instantiation of}}
+
+
+
+// Should work the same for static class members
+template <class T>
+struct S {
+  static int staticMember;
+};
+
+template <class T>
+int S<T>::staticMember = 0; // Never instantiated
+
+// T* specialization
+template <class T>
+struct S<T*> {
+  static int staticMember;
+};
+
+template <class T>
+int S<T*>::staticMember = 1; // hidden-warning {{'staticMember' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+
+template class S<int*>; // hidden-note {{in instantiation of}}
+
+// T& specialization, implicitly instantiated
+template <class T>
+struct S<T&> {
+  static int staticMember;
+};
+
+template <class T>
+int S<T&>::staticMember = 2; // hidden-warning {{'staticMember' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+
+int implicit_instantiate2() {
+  return S<bool&>::staticMember; // hidden-note {{in instantiation of}}
+}
+
+
+// Should work for static locals as well
+template <class T>
+int* wrapper() {
+  static int staticLocal; // hidden-warning {{'staticLocal' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+  return &staticLocal;
+}
+
+template <>
+int* wrapper<int*>() {
+  static int staticLocal; // hidden-warning {{'staticLocal' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+  return &staticLocal;
+}
+
+auto dummy = wrapper<bool>(); // hidden-note {{in instantiation of}}
+} // namespace TemplateTest