Enable -Wunique-object-duplication inside templated code

[DKLoehr]

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, mostly in third-party code (for a total of 234 warnings across 137 files). In clang, we found 8 new warnings across 6 files, for a total of 17 warnings across 11 files.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Devon Loehr (DKLoehr)

Changes

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, mostly in third-party code (for a total of 234 warnings across 137 files). In clang, we found 8 new warnings across 6 files, for a total of 17 warnings across 11 files.

---

Full diff: https://github.com/llvm/llvm-project/pull/125902.diff

3 Files Affected:

• (modified) clang/include/clang/Sema/Sema.h (+1)
• (modified) clang/lib/Sema/SemaDecl.cpp (+60-50)
• (modified) clang/test/SemaCXX/unique_object_duplication.h (+86-1)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 1870d1271c556c..4aa5d82a7b535e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/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
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 74e0fcec2d911b..a3936937e61961 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/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);
diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
index 5b2002c31be7c3..6ac5828d644842 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/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
\ No newline at end of file
+} // 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
\ No newline at end of file

[zmodem]

Seems reasonable to me. Just a few very minor nits.

[DKLoehr]

Thanks for the review!

[zmodem]

lgtm

I'll wait a day or so before merging in case other reviewers want to chime in.