[clang] Check for mutability better (PR #127843)

[Devon Loehr via cfe-commits]

https://github.com/DKLoehr updated https://github.com/llvm/llvm-project/pull/127843

>From 7a919e29b221f1070c420e263760b7071dc01da8 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Thu, 20 Feb 2025 15:19:13 +0000
Subject: [PATCH 1/3] Implement mutable check in Sema

---
 clang/lib/Sema/SemaDecl.cpp | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 362df485a025c..3f3fea7e23814 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13440,6 +13440,23 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
   return true;
 }
 
+// Determine whether the object seems mutable for the purpose of diagnosing
+// possible unique object duplication, i.e. non-const-qualified, and
+// not an always-constant type like a function.
+// Not perfect: doesn't account for mutable members, for example, or
+// elements of container types.
+// For nested pointers, any individual level being non-const is sufficient.
+bool looksMutable(QualType T, const ASTContext &Ctx) {
+  T = T.getNonReferenceType();
+  if (T->isFunctionType())
+    return false;
+  if (!T.isConstant(Ctx))
+    return true;
+  if (T->isPointerType())
+    return looksMutable(T->getPointeeType(), Ctx);
+  return false;
+}
+
 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
@@ -13454,24 +13471,10 @@ void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) {
       !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())) {
+    QualType Type = VD->getType();
+    if (looksMutable(Type, 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

>From 2e9bafe0191e9c9c694d5bb70e188607daae8bf5 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Thu, 20 Feb 2025 15:20:06 +0000
Subject: [PATCH 2/3] Add test

---
 clang/test/SemaCXX/unique_object_duplication.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
index a59a8f91da8b8..861175766db70 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -99,6 +99,9 @@ inline void has_thread_local() {
   thread_local int disallowedThreadLocal = 0; // hidden-warning {{'disallowedThreadLocal' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
 }
 
+// Functions themselves are always immutable, so referencing them is okay
+inline auto& allowedFunctionReference = has_static_locals_external;
+
 } // namespace StaticLocalTest
 
 /******************************************************************************

>From be5482201c6d5004cc0a2cf48d1941f45905f5e7 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Fri, 21 Feb 2025 15:18:29 +0000
Subject: [PATCH 3/3] Make function static

---
 clang/lib/Sema/SemaDecl.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 3f3fea7e23814..7633651337aa3 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13446,7 +13446,7 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
 // Not perfect: doesn't account for mutable members, for example, or
 // elements of container types.
 // For nested pointers, any individual level being non-const is sufficient.
-bool looksMutable(QualType T, const ASTContext &Ctx) {
+static bool looksMutable(QualType T, const ASTContext &Ctx) {
   T = T.getNonReferenceType();
   if (T->isFunctionType())
     return false;