Expand annotation check for -Wunique-object-duplication on Windows.

[DKLoehr]

Since dllexport/dllimport annotations don't propagate the same way as visibility, the unique object duplication warning needs to check both the object in question and its containing class. Previously, we restricted this check to static data members, but it applies to all objects inside a class, including functions. Not checking functions leads to false positives, so remove that restriction.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Devon Loehr (DKLoehr)

Changes

Since dllexport/dllimport annotations don't propagate the same way as visibility, the unique object duplication warning needs to check both the object in question and its containing class. Previously, we restricted this check to static data members, but it applies to all objects inside a class, including functions. Not checking functions leads to false positives, so remove that restriction.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+6-8)
• (modified) clang/test/SemaCXX/unique_object_duplication.h (+41-4)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index a60a558155e69..246e0395435bb 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13524,21 +13524,19 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
 
   // 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
+  // 2. On windows, it has no import/export annotation, and neither does the
+  // class which directly contains it.
   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<VarDecl>(Target);
+    const CXXRecordDecl *Ctx =
+        dyn_cast<CXXRecordDecl>(Target->getDeclContext());
+    if (Ctx && (Ctx->hasAttr<DLLExportAttr>() || Ctx->hasAttr<DLLImportAttr>()))
+      return false;
 
-    if (VD && VD->isStaticDataMember()) {
-      const CXXRecordDecl *Ctx = dyn_cast<CXXRecordDecl>(VD->getDeclContext());
-      if (Ctx &&
-          (Ctx->hasAttr<DLLExportAttr>() || Ctx->hasAttr<DLLImportAttr>()))
-        return false;
-    }
   } else if (Lnk.getVisibility() != HiddenVisibility) {
     // Posix case
     return false;
diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
index bd0ee6bd14d64..cac2e3378ae70 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -173,10 +173,6 @@ namespace GlobalTest {
 
     // Always visible
     VISIBLE static inline float allowedStaticMember2 = 0.0;
-
-    // Tests here are sparse because the AddrTest case below will define plenty
-    // more, which aren't problematic to define (because they're immutable), but
-    // may still cause problems if their address is taken.
   };
 
   inline float Test::disallowedStaticMember2 = 2.3; // hidden-warning {{'disallowedStaticMember2' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}}
@@ -211,3 +207,44 @@ inline int allowedTemplate2 = 0;
 template int allowedTemplate2<int>;
 
 } // namespace TemplateTest
+
+/******************************************************************************
+ * Case four: Nested classes
+ ******************************************************************************/
+
+namespace NestedClassTest {
+// Unlike visibility, declexport annotations do not propagate down to nested
+// classes. Hence on windows, we only get warnings for class members if their
+// immediate class is annotated. On posix, we get warnings if any containing
+// class is annotated.
+
+class VISIBLE Outer {
+  // Visible because their class is exported
+  inline static int allowedOuterMember = 0;
+  int* allowedOuterFunction() {
+    static int allowed = 0;
+    return &allowed;
+  }
+
+  // No annotation, and visibility is only propagated on posix.
+  class HiddenOnWindows {
+    inline static int disallowedInnerMember = 0; // windows-warning {{'disallowedInnerMember' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}}
+
+
+    int* disallowedInnerFunction() {
+      static int disallowed = 0; // windows-warning {{'disallowed' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}}
+      return &disallowed;
+    }
+  };
+
+  class VISIBLE AlwaysVisible {
+    inline static int allowedInnerMember = 0;
+
+    int* allowedInnerFunction() {
+      static int allowed = 0;
+      return &allowed;
+    }
+  };
+};
+
+}
\ No newline at end of file

[zmodem]

lgtm