Disable unique-object-duplication warning in templates

[DKLoehr]

I've been trying to resolve instances of the unique-object-duplication warning in chromium code. Unfortunately, I've found that practically speaking, it's near-impossible to actually fix the problem when templates are involved.

My understanding is that the warning is correct -- the variables it's flagging are indeed duplicated and potentially causing bugs as a result. The problem is that hiddenness is contagious: if a templated class or variable depends on something hidden, then it itself must also be hidden, even if the user explicitly marked it visible. In order to make it actually visible, the user must manually figure out everything that it depends on, mark them as visible, and do so recursively until all of its ancestors are visible.

This process is extremely difficult and unergonomic, negating much of the benefits of templates since now each new use requires additional work. Furthermore, the process doesn't work if the user can't edit some of the files, e.g. if they're in a third-party library.

Since a warning that can't practically be fixed isn't useful, this PR disables the warning for all templated code by inverting the check. The warning remains active (and, in my experience, easily fixable) in non-templated code.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Devon Loehr (DKLoehr)

Changes

I've been trying to resolve instances of the unique-object-duplication warning in chromium code. Unfortunately, I've found that practically speaking, it's near-impossible to actually fix the problem when templates are involved.

My understanding is that the warning is correct -- the variables it's flagging are indeed duplicated and potentially causing bugs as a result. The problem is that hiddenness is contagious: if a templated class or variable depends on something hidden, then it itself must also be hidden, even if the user explicitly marked it visible. In order to make it actually visible, the user must manually figure out everything that it depends on, mark them as visible, and do so recursively until all of its ancestors are visible.

This process is extremely difficult and unergonomic, negating much of the benefits of templates since now each new use requires additional work. Furthermore, the process doesn't work if the user can't edit some of the files, e.g. if they're in a third-party library.

Since a warning that can't practically be fixed isn't useful, this PR disables the warning for all templated code by inverting the check. The warning remains active (and, in my experience, easily fixable) in non-templated code.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+9-5)
• (modified) clang/test/SemaCXX/unique_object_duplication.h (+6-70)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 285bd27a35a76..86e65e56accc8 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13427,9 +13427,13 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
         FunDcl->getTemplateSpecializationKind() != TSK_Undeclared;
   }
 
-  // Non-inline functions/variables can only legally appear in one TU,
-  // unless they were part of a template.
-  if (!TargetIsInline && !TargetWasTemplated)
+  // Non-inline functions/variables can only legally appear in one TU
+  // unless they were part of a template. Unfortunately, making complex
+  // template instantiations visible is infeasible in practice, since
+  // everything the template depends on also has to be visible. To avoid
+  // giving impractical-to-fix warnings, don't warn if we're inside
+  // something that was templated, even on inline stuff.
+  if (!TargetIsInline || TargetWasTemplated)
     return false;
 
   // If the object isn't hidden, the dynamic linker will prevent duplication.
@@ -13469,8 +13473,8 @@ void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) {
   // 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.
+  // Don't diagnose if we're inside a template, because it's not practical to
+  // fix the warning in most cases.
   if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
       !VD->isTemplated() &&
       GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
index 861175766db70..e5c63efbf918c 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -165,81 +165,17 @@ namespace GlobalTest {
 
 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}}
-}
-
+// We never warn inside templates because it's frequently infeasible to actually
+// fix the warning.
 
-// 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}}
+int allowedTemplate1 = 0;
 
-
-
-// Should work the same for static class members
-template <typename T>
-struct S {
-  static int staticMember;
-};
+template int allowedTemplate1<int>;
 
 template <typename T>
-int S<T>::staticMember = 0; // Never instantiated
+inline int allowedTemplate2 = 0;
 
-// 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;
-}
+template int allowedTemplate2<int>;
 
-auto dummy = wrapper<bool>(); // hidden-note {{in instantiation of}}
 } // namespace TemplateTest
\ No newline at end of file

[zmodem]

lgtm