[clang] Enable -Wunique-object-duplication inside templated code (PR #125902)
Devon Loehr via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 11 08:27:48 PST 2025
https://github.com/DKLoehr updated https://github.com/llvm/llvm-project/pull/125902
>From d95344cf393bcf0a8580e81f4848f5f72c67a652 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Tue, 4 Feb 2025 16:47:01 +0000
Subject: [PATCH 1/5] Move into separate function, call in
CheckCompleteVariableDeclaration
---
clang/include/clang/Sema/Sema.h | 1 +
clang/lib/Sema/SemaDecl.cpp | 94 +++++++++++++++++----------------
2 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 1870d1271c556..4aa5d82a7b535 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 74e0fcec2d911..eb8fbf424d264 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13436,6 +13436,53 @@ 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.
+ // 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;
+ }
+ }
+}
+
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 +14702,8 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
return;
}
+ DiagnoseDangerousUniqueObjectDuplication(var);
+
// Require the destructor.
if (!type->isDependentType())
if (const RecordType *recordType = baseType->getAs<RecordType>())
@@ -14842,51 +14891,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);
>From a7a2fbea6446dc17f4763e629524ae9e92bf8736 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Tue, 4 Feb 2025 17:24:20 +0000
Subject: [PATCH 2/5] Enable for templates
---
clang/lib/Sema/SemaDecl.cpp | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index eb8fbf424d264..a3936937e6196 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,18 +13440,20 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
return true;
}
-void Sema::DiagnoseDangerousUniqueObjectDuplication(const VarDecl* VD) {
+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)
+ // 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).
+
+ // 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();
>From 074f5e52597fa997cc015235483580e3084b84e1 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Wed, 5 Feb 2025 17:48:34 +0000
Subject: [PATCH 3/5] Add tests for templates
---
.../test/SemaCXX/unique_object_duplication.h | 87 ++++++++++++++++++-
1 file changed, 86 insertions(+), 1 deletion(-)
diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
index 5b2002c31be7c..6ac5828d64484 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
>From 6081efa1a76278082d65c12b3fe290244d4cbc0e Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Tue, 11 Feb 2025 15:36:46 +0000
Subject: [PATCH 4/5] Change class->typename in test templates
---
clang/test/SemaCXX/unique_object_duplication.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
index 6ac5828d64484..a59a8f91da8b8 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -192,32 +192,32 @@ template int maybeAllowedTemplate<int*>; // hidden-note {{in instantiation of}}
// Should work the same for static class members
-template <class T>
+template <typename T>
struct S {
static int staticMember;
};
-template <class T>
+template <typename T>
int S<T>::staticMember = 0; // Never instantiated
// T* specialization
-template <class T>
+template <typename T>
struct S<T*> {
static int staticMember;
};
-template <class T>
+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 <class T>
+template <typename T>
struct S<T&> {
static int staticMember;
};
-template <class T>
+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() {
@@ -226,7 +226,7 @@ int implicit_instantiate2() {
// Should work for static locals as well
-template <class T>
+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;
>From 12345719ccd6952486a12a7899495c6018d471ac Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Tue, 11 Feb 2025 16:24:25 +0000
Subject: [PATCH 5/5] Address more feedback
---
clang/include/clang/Sema/Sema.h | 2 +-
clang/lib/Sema/SemaDecl.cpp | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 4aa5d82a7b535..a501b901862b6 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3669,7 +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);
+ void DiagnoseUniqueObjectDuplication(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 a3936937e6196..4ed80327291ff 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13440,7 +13440,7 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
return true;
}
-void Sema::DiagnoseDangerousUniqueObjectDuplication(const VarDecl *VD) {
+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
// mutable (since the copies won't be in sync) or its initialization has side
@@ -14708,7 +14708,7 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
return;
}
- DiagnoseDangerousUniqueObjectDuplication(var);
+ DiagnoseUniqueObjectDuplication(var);
// Require the destructor.
if (!type->isDependentType())
More information about the cfe-commits
mailing list