Enable -Wunique-object-duplication inside templated code (#125902)

Followup to #125526. This allows the unique object duplication warning
to fire on code inside of templates. Previously, it was disabled there
to prevent false positives if the template was never instantiated.

The patch has two parts: first, we move the check from
`FinalizeDeclaration` (which is only called during parsing) to
`CheckCompleteVariableDeclaration` (which is also called during template
instantiation). Since the code we're moving is fairly bulky, we abstract
it into a separate function for convenience.

Second, we disable the warning during template parsing, and add a check
later to see if the variable we're acting on on originated from a
template. If so, it has the potential to be duplicated just like an
inline variable.

## Testing
Unit tests for template have been added to the existing test suite. To
evaluate the patch on real code, I ran it on chromium and on clang
itself. As expected, in both cases we got strictly more warnings than
before. I manually looked through each new warning, and they all seemed
legitimate.

In chromium, we found [79 new warnings across 55
files](https://github.com/user-attachments/files/18676635/new_warnings_chromium.txt),
mostly in third-party code (for a total of 234 warnings across 137
files). In clang, we found [8 new warnings across 6
files](https://github.com/user-attachments/files/18676658/new_warnings_clang.txt),
for a total of 17 warnings across 11 files.

Author: DKLoehr

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 DiagnoseUniqueObjectDuplication(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::DiagnoseUniqueObjectDuplication(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;
   }
 
+  DiagnoseUniqueObjectDuplication(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 <typename T>
+struct S {
+  static int staticMember;
+};
+
+template <typename T>
+int S<T>::staticMember = 0; // Never instantiated
+
+// T* specialization
+template <typename T>
+struct S<T*> {
+  static int staticMember;
+};
+
+template <typename 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 <typename T>
+struct S<T&> {
+  static int staticMember;
+};
+
+template <typename 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 <typename 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