Enable UOD warning for windows

DKLoehr · DKLoehr · commit 8c48be860464 · 2025-06-10T13:57:17.000Z
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -806,7 +806,9 @@ def UniqueObjectDuplication : DiagGroup<"unique-object-duplication"> {
 Warns when objects which are supposed to be globally unique might get duplicated
 when built into a shared library.
 
-If an object with hidden visibility is built into a shared library, each instance
+This can occur to objects which are hidden from the dynamic linker, due to
+having hidden visibility (on posix) or lacking an import/export annotation (on
+windows). If such an object is built into a shared library, each instance
 of the library will get its own copy. This can cause very subtle bugs if there was
 only supposed to be one copy of the object in question: singletons aren't single,
 changes to one object won't affect the others, the object's initializer will run
@@ -815,7 +817,7 @@ once per copy, etc.
 Specifically, this warning fires when it detects an object which:
 1. Is defined as ``inline`` in a header file (so it might get compiled into multiple libaries), and
 2. Has external linkage (otherwise it's supposed to be duplicated), and
-3. Has hidden visibility.
+3. Has hidden visibility (posix) or lacks an import/export annotation (windows).
 
 As well as one of the following:
 1. The object is mutable, or
@@ -825,13 +827,15 @@ The warning can be resolved by removing one of the conditions above. In rough
 order of preference, this may be done by:
 1. Marking the object ``const`` (if possible)
 2. Moving the object's definition to a source file
-3. Giving the object non-hidden visibility, e.g. using ``__attribute((visibility("default")))``.
+3. Making the object visible using ``__attribute((visibility("default")))``,
+   ``__declspec(dllimport)``, or ``__declspec(dllexport)``.
+
+When annotating an object with ``__declspec(dllimport)`` or ``__declspec(dllexport)``,
+take care to ensure that the object is only imported in one dll, and is exported
+everywhere else.
 
 Note that for (2), all levels of a pointer variable must be constant;
 ``const int*`` will trigger the warning because the pointer itself is mutable.
-
-This warning is not yet implemented for Windows, since Windows uses
-import/export rules instead of visibility.
 }];
 }
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6260,14 +6260,19 @@ def warn_static_local_in_extern_inline : Warning<
 def note_convert_inline_to_static : Note<
   "use 'static' to give inline function %0 internal linkage">;
 
-def warn_possible_object_duplication_mutable : Warning<
-  "%0 may be duplicated when built into a shared library: "
-  "it is mutable, has hidden visibility, and external linkage">,
-  InGroup<UniqueObjectDuplication>, DefaultIgnore;
-def warn_possible_object_duplication_init : Warning<
-  "initialization of %0 may run twice when built into a shared library: "
-  "it has hidden visibility and external linkage">,
-  InGroup<UniqueObjectDuplication>, DefaultIgnore;
+def warn_possible_object_duplication_mutable
+    : Warning<"%0 may be duplicated when built into a shared library: "
+              "it is mutable, with external linkage and "
+              "%select{hidden visibility|no import/export annotation}1">,
+      InGroup<UniqueObjectDuplication>,
+      DefaultIgnore;
+def warn_possible_object_duplication_init
+    : Warning<"initialization of %0 may run twice when built into a shared "
+              "library: "
+              "it has external linkage and "
+              "%select{hidden visibility|no import/export annotation}1">,
+      InGroup<UniqueObjectDuplication>,
+      DefaultIgnore;
 
 def ext_redefinition_of_typedef : ExtWarn<
   "redefinition of typedef %0 is a C11 feature">,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
@@ -13518,8 +13518,29 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
 
   // If the object isn't hidden, the dynamic linker will prevent duplication.
   clang::LinkageInfo Lnk = Target->getLinkageAndVisibility();
-  if (Lnk.getVisibility() != HiddenVisibility)
+
+  // The target is "hidden" (from the dynamic linker) if:
+  // 1. On posix, it has hidden visibility, or
+  // 2. On windows, it has no import/export annotation
+  if (Context.getTargetInfo().shouldDLLImportComdatSymbols()) {
+    if (Target->hasAttr<DLLExportAttr>() || Target->hasAttr<DLLImportAttr>())
+      return false;
+
+    // If the variable isn't directly annotated, check to see if it's a member
+    // of an annotated class.
+    const VarDecl *VD = dyn_cast_if_present<VarDecl>(Target);
+
+    if (VD && VD->isStaticDataMember()) {
+      const CXXRecordDecl *Ctx =
+          dyn_cast_if_present<CXXRecordDecl>(VD->getDeclContext());
+      if (Ctx &&
+          (Ctx->hasAttr<DLLExportAttr>() || Ctx->hasAttr<DLLImportAttr>()))
+        return false;
+    }
+  } else if (Lnk.getVisibility() != HiddenVisibility) {
+    // Posix case
     return false;
+  }
 
   // If the obj doesn't have external linkage, it's supposed to be duplicated.
   if (!isExternalFormalLinkage(Lnk.getLinkage()))
@@ -13550,19 +13571,16 @@ void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) {
   // 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, because it's not practical to
   // fix the warning in most cases.
-  if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
-      !VD->isTemplated() &&
+  if (!VD->isTemplated() &&
       GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
 
     QualType Type = VD->getType();
     if (looksMutable(Type, VD->getASTContext())) {
       Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
-          << VD;
+          << VD << Context.getTargetInfo().shouldDLLImportComdatSymbols();
     }
 
     // To keep false positives low, only warn if we're certain that the
@@ -13575,7 +13593,7 @@ void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) {
                              /*IncludePossibleEffects=*/false) &&
         !isa<CXXNewExpr>(Init->IgnoreParenImpCasts())) {
       Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init)
-          << VD;
+          << VD << Context.getTargetInfo().shouldDLLImportComdatSymbols();
     }
   }
 }
diff --git a/clang/test/SemaCXX/unique_object_duplication.cpp b/clang/test/SemaCXX/unique_object_duplication.cpp
@@ -1,7 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -verify=hidden -Wunique-object-duplication -fvisibility=hidden -Wno-unused-value %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wunique-object-duplication -Wno-unused-value %s
-// The check is currently disabled on windows in MSVC-like environments. The test should fail because we're not getting the expected warnings.
-// XFAIL: target={{.*}}-windows-msvc, {{.*}}-ps{{(4|5)(-.+)?}}
+// RUN: %clang_cc1 -fsyntax-only -Wunique-object-duplication -Wno-unused-value \
+// RUN:   -verify -triple=x86_64-pc-linux-gnu %s
+// RUN: %clang_cc1 -fsyntax-only -Wunique-object-duplication -Wno-unused-value \
+// RUN:   -verify=hidden -triple=x86_64-pc-linux-gnu -fvisibility=hidden  %s
+// RUN: %clang_cc1 -fsyntax-only -Wunique-object-duplication -Wno-unused-value \
+// RUN:   -verify=windows -triple=x86_64-windows-msvc -DWINDOWS_TEST -fdeclspec %s
 
 #include "unique_object_duplication.h"
 
diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
