https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/125526
>From 486c3297f1a316a103c6583daf732af2d00d0b96 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dlo...@google.com>
Date: Thu, 21 Nov 2024 19:29:00 +0000
Subject: [PATCH 1/6] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td | 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++++++++++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++++++++++++++++++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index abb575002e1182..d63827cf39b06c 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -694,6 +694,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5876b2a6ae0eab..6b7fdfbb3cbb6c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6169,6 +6169,15 @@ 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 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup<UniqueObjectDuplication>, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup<UniqueObjectDuplication>, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup<DiagGroup<"typedef-redefinition"> >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 472a0e25adc975..f04f5dccc39401 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3664,6 +3664,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+ /// effectful, or its address is taken.
+ bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *dcl);
+
/// AddInitializerToDecl - Adds the initializer Init to the
/// declaration dcl. If DirectInit is true, this is C++ direct
/// initialization rather than copy initialization.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 1755b37fc8f295..ace56225b2f4c0 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13380,6 +13380,62 @@ void Sema::checkNonTrivialCUnion(QualType QT,
SourceLocation Loc,
.visit(QT, nullptr, false);
}
+bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
+ const VarDecl *dcl) {
+ if (!dcl || !getLangOpts().CPlusPlus)
+ return false;
+
+ // If an object is defined in a source file, its definition can't get
+ // duplicated since it will never appear in more than one TU.
+ if (dcl->getASTContext().getSourceManager().isInMainFile(dcl->getLocation()))
+ return false;
+
+ // We only need to warn if the definition is in a header file, so wait to
+ // diagnose until we've seen the definition.
+ if (!dcl->isThisDeclarationADefinition())
+ return false;
+
+ // If the variable we're looking at is a static local, then we actually care
+ // 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.
+ bool target_is_inline = dcl->isInline();
+
+ // Update the target and target_is_inline property if necessary
+ if (dcl->isStaticLocal()) {
+ const DeclContext *ctx = dcl->getDeclContext();
+ if (!ctx)
+ return false;
+
+ const FunctionDecl *f_dcl =
+ dyn_cast_if_present<FunctionDecl>(ctx->getNonClosureAncestor());
+ if (!f_dcl)
+ return false;
+
+ target = f_dcl;
+ // IsInlined() checks for the C++ inline property
+ target_is_inline = f_dcl->isInlined();
+ }
+
+ // 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 (!target_is_inline)
+ return false;
+
+ // If the object isn't hidden, the dynamic linker will prevent duplication.
+ clang::LinkageInfo lnk = target->getLinkageAndVisibility();
+ if (lnk.getVisibility() != HiddenVisibility)
+ return false;
+
+ // If the obj doesn't have external linkage, it's supposed to be duplicated.
+ if (!isExternalFormalLinkage(lnk.getLinkage()))
+ return false;
+
+ return true;
+}
+
void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
// If there is no declaration, there was an error parsing it. Just ignore
// the initializer.
@@ -14786,6 +14842,51 @@ 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);
diff --git a/clang/test/SemaCXX/unique_object_duplication.cpp
b/clang/test/SemaCXX/unique_object_duplication.cpp
new file mode 100644
index 00000000000000..d08d053c84ae3c
--- /dev/null
+++ b/clang/test/SemaCXX/unique_object_duplication.cpp
@@ -0,0 +1,16 @@
+// 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. The test should fail because
we're not getting the expected warnings.
+// XFAIL: target={{.*}}-windows{{.*}}
+
+#include "unique_object_duplication.h"
+
+// Everything in these namespaces here is defined in the cpp file,
+// so won't get duplicated
+
+namespace GlobalTest {
+ float Test::allowedStaticMember1 = 2.3;
+}
+
+bool disallowed4 = true;
+constexpr inline bool disallowed5 = true;
\ No newline at end of file
diff --git a/clang/test/SemaCXX/unique_object_duplication.h
b/clang/test/SemaCXX/unique_object_duplication.h
new file mode 100644
index 00000000000000..39afa67ca2ca6c
--- /dev/null
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -0,0 +1,187 @@
+/**
+ * When building shared libraries, hidden objects which are defined in header
+ * files will be duplicated, with one copy in each shared library. If the
object
+ * was meant to be globally unique (one copy per program), this can cause very
+ * subtle bugs. This file contains tests for the -Wunique-object-duplication
+ * warning, which is meant to detect this.
+ *
+ * Roughly, an object might be incorrectly duplicated if:
+ * - Is defined in a header (so it might appear in multiple TUs), and
+ * - Has external linkage (otherwise it's supposed to be duplicated), and
+ * - Has hidden visibility (or else the dynamic linker will handle it)
+ *
+ * Duplication becomes an issue only if one of the following is true:
+ * - The object is mutable (the copies won't be in sync), or
+ * - Its initialization may has side effects (it may now run more than once),
or
+ * - The value of its address is used.
+ *
+ * Currently, we only detect the first two, and only warn on effectful
+ * initialization if we're certain there are side effects. Warning if the
+ * address is taken is prone to false positives, so we don't warn for now.
+ *
+ * The check is also disabled on Windows for now, since it uses
+ * dllimport/dllexport instead of visibility.
+ */
+
+#define HIDDEN __attribute__((visibility("hidden")))
+#define DEFAULT __attribute__((visibility("default")))
+
+// Helper functions
+constexpr int init_constexpr(int x) { return x; };
+extern double init_dynamic(int);
+
+/******************************************************************************
+ * Case one: Static local variables in an externally-visible function
+
******************************************************************************/
+namespace StaticLocalTest {
+
+inline void has_static_locals_external() {
+ // Mutable
+ static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is
mutable, has hidden visibility, and external linkage; it may be duplicated when
built into a shared library}}
+ // Initialization might run more than once
+ static const double disallowedStatic2 = disallowedStatic1++; //
hidden-warning {{'disallowedStatic2' has hidden visibility, and external
linkage; its initialization may run more than once when built into a shared
library}}
+
+ // OK, because immutable and compile-time-initialized
+ static constexpr int allowedStatic1 = 0;
+ static const float allowedStatic2 = 1;
+ static constexpr int allowedStatic3 = init_constexpr(2);
+ static const int allowedStatic4 = init_constexpr(3);
+}
+
+// Don't warn for non-inline functions, since they can't (legally) appear
+// in more than one TU in the first place.
+void has_static_locals_non_inline() {
+ // Mutable
+ static int allowedStatic1 = 0;
+ // Initialization might run more than once
+ static const double allowedStatic2 = allowedStatic1++;
+}
+
+// Everything in this function is OK because the function is TU-local
+static void has_static_locals_internal() {
+ static int allowedStatic1 = 0;
+ static double allowedStatic2 = init_dynamic(2);
+ static char allowedStatic3 = []() { return allowedStatic1++; }();
+
+ static constexpr int allowedStatic4 = 0;
+ static const float allowedStatic5 = 1;
+ static constexpr int allowedStatic6 = init_constexpr(2);
+ static const int allowedStatic7 = init_constexpr(3);
+}
+
+namespace {
+
+// Everything in this function is OK because the function is also TU-local
+void has_static_locals_anon() {
+ static int allowedStatic1 = 0;
+ static double allowedStatic2 = init_dynamic(2);
+ static char allowedStatic3 = []() { return allowedStatic1++; }();
+
+ static constexpr int allowedStatic4 = 0;
+ static const float allowedStatic5 = 1;
+ static constexpr int allowedStatic6 = init_constexpr(2);
+ static const int allowedStatic7 = init_constexpr(3);
+}
+
+} // Anonymous namespace
+
+HIDDEN inline void static_local_always_hidden() {
+ static int disallowedStatic1 = 3; // hidden-warning {{'disallowedStatic1'
is mutable, has hidden visibility, and external linkage; it may be duplicated
when built into a shared library}}
+ // expected-warning@-1
{{'disallowedStatic1' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ {
+ static int disallowedStatic2 = 3; // hidden-warning
{{'disallowedStatic2' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ // expected-warning@-1
{{'disallowedStatic2' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ }
+
+ auto lmb = []() {
+ static int disallowedStatic3 = 3; // hidden-warning
{{'disallowedStatic3' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ // expected-warning@-1
{{'disallowedStatic3' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ };
+}
+
+DEFAULT void static_local_never_hidden() {
+ static int allowedStatic1 = 3;
+
+ {
+ static int allowedStatic2 = 3;
+ }
+
+ auto lmb = []() {
+ static int allowedStatic3 = 3;
+ };
+}
+
+// Don't warn on this because it's not in a function
+const int setByLambda = ([]() { static int x = 3; return x++; })();
+
+inline void has_extern_local() {
+ extern int allowedAddressExtern; // Not a definition
+}
+
+inline void has_regular_local() {
+ int allowedAddressLocal = 0;
+}
+
+inline void has_thread_local() {
+ // thread_local variables are static by default
+ thread_local int disallowedThreadLocal = 0; // hidden-warning
{{'disallowedThreadLocal' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+}
+
+} // namespace StaticLocalTest
+
+/******************************************************************************
+ * Case two: Globals with external linkage
+
******************************************************************************/
+namespace GlobalTest {
+ // Mutable
+ inline float disallowedGlobal1 = 3.14; // hidden-warning
{{'disallowedGlobal1' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ // Same as above, but explicitly marked inline
+ inline float disallowedGlobal4 = 3.14; // hidden-warning
{{'disallowedGlobal4' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+
+ // Initialization might run more than once
+ inline const double disallowedGlobal5 = disallowedGlobal1++; //
hidden-warning {{'disallowedGlobal5' has hidden visibility, and external
linkage; its initialization may run more than once when built into a shared
library}}
+
+ // OK because internal linkage, so duplication is intended
+ static float allowedGlobal1 = 3.14;
+ const double allowedGlobal2 = init_dynamic(2);
+ static const char allowedGlobal3 = []() { return disallowedGlobal1++; }();
+ static inline double allowedGlobal4 = init_dynamic(2);
+
+ // OK, because immutable and compile-time-initialized
+ constexpr int allowedGlobal5 = 0;
+ const float allowedGlobal6 = 1;
+ constexpr int allowedGlobal7 = init_constexpr(2);
+ const int allowedGlobal8 = init_constexpr(3);
+
+ // We don't warn on this because non-inline variables can't (legally) appear
+ // in more than one TU.
+ float allowedGlobal9 = 3.14;
+
+ // Pointers need to be double-const-qualified
+ inline float& nonConstReference = disallowedGlobal1; // hidden-warning
{{'nonConstReference' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ const inline int& constReference = allowedGlobal5;
+
+ inline int* nonConstPointerToNonConst = nullptr; // hidden-warning
{{'nonConstPointerToNonConst' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ inline int const* nonConstPointerToConst = nullptr; // hidden-warning
{{'nonConstPointerToConst' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ inline int* const constPointerToNonConst = nullptr; // hidden-warning
{{'constPointerToNonConst' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ inline int const* const constPointerToConst = nullptr;
+ // Don't warn on new because it tends to generate false positives
+ inline int const* const constPointerToConstNew = new int(7);
+
+ inline int const * const * const * const nestedConstPointer = nullptr;
+ inline int const * const ** const * const nestedNonConstPointer = nullptr;
// hidden-warning {{'nestedNonConstPointer' is mutable, has hidden visibility,
and external linkage; it may be duplicated when built into a shared library}}
+
+ struct Test {
+ static inline float disallowedStaticMember1; // hidden-warning
{{'disallowedStaticMember1' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ // Defined below, in the header file
+ static float disallowedStaticMember2;
+ // Defined in the cpp file, so won't get duplicated
+ static float allowedStaticMember1;
+
+ // 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' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+} // namespace GlobalTest
\ No newline at end of file
>From 31ae747d3b83faea6b8bfd8d63a38147fd5bf828 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dlo...@google.com>
Date: Fri, 17 Jan 2025 16:21:45 +0000
Subject: [PATCH 2/6] Address first feedback
---
clang/lib/Sema/SemaDecl.cpp | 46 ++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index ace56225b2f4c0..a9730f60a34994 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13381,56 +13381,56 @@ void Sema::checkNonTrivialCUnion(QualType QT,
SourceLocation Loc,
}
bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
- const VarDecl *dcl) {
- if (!dcl || !getLangOpts().CPlusPlus)
- return false;
-
- // If an object is defined in a source file, its definition can't get
- // duplicated since it will never appear in more than one TU.
- if (dcl->getASTContext().getSourceManager().isInMainFile(dcl->getLocation()))
+ const VarDecl *Dcl) {
+ if (!Dcl || !getLangOpts().CPlusPlus)
return false;
// We only need to warn if the definition is in a header file, so wait to
// diagnose until we've seen the definition.
- if (!dcl->isThisDeclarationADefinition())
+ if (!Dcl->isThisDeclarationADefinition())
+ return false;
+
+ // If an object is defined in a source file, its definition can't get
+ // duplicated since it will never appear in more than one TU.
+ if (Dcl->getASTContext().getSourceManager().isInMainFile(Dcl->getLocation()))
return false;
// If the variable we're looking at is a static local, then we actually care
// about the properties of the function containing it.
- const ValueDecl *target = dcl;
+ const ValueDecl *Target = Dcl;
// VarDecls and FunctionDecls have different functions for checking
// inline-ness, so we have to do it manually.
- bool target_is_inline = dcl->isInline();
+ bool TargetIsInline = Dcl->isInline();
- // Update the target and target_is_inline property if necessary
- if (dcl->isStaticLocal()) {
- const DeclContext *ctx = dcl->getDeclContext();
- if (!ctx)
+ // Update the Target and TargetIsInline property if necessary
+ if (Dcl->isStaticLocal()) {
+ const DeclContext *Ctx = Dcl->getDeclContext();
+ if (!Ctx)
return false;
- const FunctionDecl *f_dcl =
- dyn_cast_if_present<FunctionDecl>(ctx->getNonClosureAncestor());
- if (!f_dcl)
+ const FunctionDecl *FunDcl =
+ dyn_cast_if_present<FunctionDecl>(Ctx->getNonClosureAncestor());
+ if (!FunDcl)
return false;
- target = f_dcl;
+ Target = FunDcl;
// IsInlined() checks for the C++ inline property
- target_is_inline = f_dcl->isInlined();
+ TargetIsInline = FunDcl->isInlined();
}
// 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 (!target_is_inline)
+ if (!TargetIsInline)
return false;
// If the object isn't hidden, the dynamic linker will prevent duplication.
- clang::LinkageInfo lnk = target->getLinkageAndVisibility();
- if (lnk.getVisibility() != HiddenVisibility)
+ clang::LinkageInfo Lnk = Target->getLinkageAndVisibility();
+ if (Lnk.getVisibility() != HiddenVisibility)
return false;
// If the obj doesn't have external linkage, it's supposed to be duplicated.
- if (!isExternalFormalLinkage(lnk.getLinkage()))
+ if (!isExternalFormalLinkage(Lnk.getLinkage()))
return false;
return true;
>From 6356a4f66a0982b22c693c03a2328480a62cf63e Mon Sep 17 00:00:00 2001
From: Devon Loehr <dlo...@google.com>
Date: Fri, 24 Jan 2025 16:40:16 +0000
Subject: [PATCH 3/6] Add documentation and a release note
---
clang/docs/ReleaseNotes.rst | 3 ++
clang/include/clang/Basic/DiagnosticGroups.td | 31 ++++++++++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a128726b999173..4266d6c523ab97 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -116,6 +116,9 @@ Improvements to Clang's diagnostics
- Improve the diagnostics for deleted default constructor errors for C++ class
initializer lists that don't explicitly list a class member and thus attempt
to implicitly default construct that member.
+- The ``-Wunique-object-duplication`` warning has been added to warn about
objects
+ which are supposed to only exist once per program, but may get duplicated
when
+ built into a shared library.
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index d63827cf39b06c..05e39899e6f257 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -694,7 +694,36 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
-def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication"> {
+ code Documentation = [{
+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
+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
+once per copy, etc.
+
+Specifically, this warning fires when it detects an object which:
+ 1. Appears 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.
+
+As well as one of the following:
+ 1. The object is mutable, or
+ 2. The object's initializer definitely has side effects.
+
+The warning is best resolved by making the object ``const`` (if possible), or
by explicitly
+giving the object non-hidden visibility, e.g. using
``__attribute((visibility("default")))``.
+Note that all levels of a pointer variable must be constant; ``const int*``
will
+trigger the warning because the pointer itself is mutable.
+
+This warning is currently disabled on Windows since it uses import/export rules
+instead of visibility.
+}];
+}
+
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
>From cde5d15161378e21e7478c4f3eaa90e9337d5d0e Mon Sep 17 00:00:00 2001
From: Devon Loehr <dlo...@google.com>
Date: Wed, 29 Jan 2025 19:23:27 +0000
Subject: [PATCH 4/6] Address feedback again
---
.../clang/Basic/DiagnosticSemaKinds.td | 8 +--
clang/include/clang/Sema/Sema.h | 2 +-
.../test/SemaCXX/unique_object_duplication.h | 72 ++++++-------------
3 files changed, 26 insertions(+), 56 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6b7fdfbb3cbb6c..7b3b932c482baa 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6170,12 +6170,12 @@ def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
def warn_possible_object_duplication_mutable : Warning<
- "%0 is mutable, has hidden visibility, and external linkage; it may be "
- "duplicated when built into a shared library">,
+ "%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<
- "%0 has hidden visibility, and external linkage; its initialization may run "
- "more than once when built into a shared library">,
+ "initializeation of %0 may run twice when built into a shared library: "
+ "it has hidden visibility and external linkage">,
InGroup<UniqueObjectDuplication>, DefaultIgnore;
def ext_redefinition_of_typedef : ExtWarn<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f04f5dccc39401..59e29262e35047 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3668,7 +3668,7 @@ class Sema final : public SemaBase {
/// built into multiple shared libraries with hidden visibility. This can
/// cause problems if the variable is mutable, its initialization is
/// effectful, or its address is taken.
- bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *dcl);
+ bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *Dcl);
/// AddInitializerToDecl - Adds the initializer Init to the
/// declaration dcl. If DirectInit is true, this is C++ direct
diff --git a/clang/test/SemaCXX/unique_object_duplication.h
b/clang/test/SemaCXX/unique_object_duplication.h
index 39afa67ca2ca6c..5b2002c31be7c3 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -1,26 +1,6 @@
/**
- * When building shared libraries, hidden objects which are defined in header
- * files will be duplicated, with one copy in each shared library. If the
object
- * was meant to be globally unique (one copy per program), this can cause very
- * subtle bugs. This file contains tests for the -Wunique-object-duplication
- * warning, which is meant to detect this.
- *
- * Roughly, an object might be incorrectly duplicated if:
- * - Is defined in a header (so it might appear in multiple TUs), and
- * - Has external linkage (otherwise it's supposed to be duplicated), and
- * - Has hidden visibility (or else the dynamic linker will handle it)
- *
- * Duplication becomes an issue only if one of the following is true:
- * - The object is mutable (the copies won't be in sync), or
- * - Its initialization may has side effects (it may now run more than once),
or
- * - The value of its address is used.
- *
- * Currently, we only detect the first two, and only warn on effectful
- * initialization if we're certain there are side effects. Warning if the
- * address is taken is prone to false positives, so we don't warn for now.
- *
- * The check is also disabled on Windows for now, since it uses
- * dllimport/dllexport instead of visibility.
+ * This file contains tests for the -Wunique_object_duplication warning.
+ * See the warning's documentation for more information.
*/
#define HIDDEN __attribute__((visibility("hidden")))
@@ -37,9 +17,9 @@ namespace StaticLocalTest {
inline void has_static_locals_external() {
// Mutable
- static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is
mutable, has hidden visibility, and external linkage; it may be duplicated when
built into a shared library}}
+ static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1'
may be duplicated when built into a shared library: it is mutable, has hidden
visibility, and external linkage}}
// Initialization might run more than once
- static const double disallowedStatic2 = disallowedStatic1++; //
hidden-warning {{'disallowedStatic2' has hidden visibility, and external
linkage; its initialization may run more than once when built into a shared
library}}
+ static const double disallowedStatic2 = disallowedStatic1++; //
hidden-warning {{initializeation of 'disallowedStatic2' may run twice when
built into a shared library: it has hidden visibility and external linkage}}
// OK, because immutable and compile-time-initialized
static constexpr int allowedStatic1 = 0;
@@ -62,11 +42,7 @@ static void has_static_locals_internal() {
static int allowedStatic1 = 0;
static double allowedStatic2 = init_dynamic(2);
static char allowedStatic3 = []() { return allowedStatic1++; }();
-
static constexpr int allowedStatic4 = 0;
- static const float allowedStatic5 = 1;
- static constexpr int allowedStatic6 = init_constexpr(2);
- static const int allowedStatic7 = init_constexpr(3);
}
namespace {
@@ -76,26 +52,22 @@ void has_static_locals_anon() {
static int allowedStatic1 = 0;
static double allowedStatic2 = init_dynamic(2);
static char allowedStatic3 = []() { return allowedStatic1++; }();
-
- static constexpr int allowedStatic4 = 0;
- static const float allowedStatic5 = 1;
- static constexpr int allowedStatic6 = init_constexpr(2);
- static const int allowedStatic7 = init_constexpr(3);
+ static constexpr int allowedStatic4 = init_constexpr(3);
}
} // Anonymous namespace
HIDDEN inline void static_local_always_hidden() {
- static int disallowedStatic1 = 3; // hidden-warning {{'disallowedStatic1'
is mutable, has hidden visibility, and external linkage; it may be duplicated
when built into a shared library}}
- // expected-warning@-1
{{'disallowedStatic1' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ static int disallowedStatic1 = 3; // hidden-warning {{'disallowedStatic1'
may be duplicated when built into a shared library: it is mutable, has hidden
visibility, and external linkage}}
+ // expected-warning@-1
{{'disallowedStatic1' may be duplicated when built into a shared library: it is
mutable, has hidden visibility, and external linkage}}
{
- static int disallowedStatic2 = 3; // hidden-warning
{{'disallowedStatic2' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
- // expected-warning@-1
{{'disallowedStatic2' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ static int disallowedStatic2 = 3; // hidden-warning
{{'disallowedStatic2' may be duplicated when built into a shared library: it is
mutable, has hidden visibility, and external linkage}}
+ // expected-warning@-1
{{'disallowedStatic2' may be duplicated when built into a shared library: it is
mutable, has hidden visibility, and external linkage}}
}
auto lmb = []() {
- static int disallowedStatic3 = 3; // hidden-warning
{{'disallowedStatic3' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
- // expected-warning@-1
{{'disallowedStatic3' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ static int disallowedStatic3 = 3; // hidden-warning
{{'disallowedStatic3' may be duplicated when built into a shared library: it is
mutable, has hidden visibility, and external linkage}}
+ // expected-warning@-1
{{'disallowedStatic3' may be duplicated when built into a shared library: it is
mutable, has hidden visibility, and external linkage}}
};
}
@@ -124,7 +96,7 @@ inline void has_regular_local() {
inline void has_thread_local() {
// thread_local variables are static by default
- thread_local int disallowedThreadLocal = 0; // hidden-warning
{{'disallowedThreadLocal' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ 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}}
}
} // namespace StaticLocalTest
@@ -134,12 +106,10 @@ inline void has_thread_local() {
******************************************************************************/
namespace GlobalTest {
// Mutable
- inline float disallowedGlobal1 = 3.14; // hidden-warning
{{'disallowedGlobal1' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
- // Same as above, but explicitly marked inline
- inline float disallowedGlobal4 = 3.14; // hidden-warning
{{'disallowedGlobal4' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ inline float disallowedGlobal1 = 3.14; // hidden-warning
{{'disallowedGlobal1' may be duplicated when built into a shared library: it is
mutable, has hidden visibility, and external linkage}}
// Initialization might run more than once
- inline const double disallowedGlobal5 = disallowedGlobal1++; //
hidden-warning {{'disallowedGlobal5' has hidden visibility, and external
linkage; its initialization may run more than once when built into a shared
library}}
+ inline const double disallowedGlobal5 = disallowedGlobal1++; //
hidden-warning {{initializeation of 'disallowedGlobal5' may run twice when
built into a shared library: it has hidden visibility and external linkage}}
// OK because internal linkage, so duplication is intended
static float allowedGlobal1 = 3.14;
@@ -158,21 +128,21 @@ namespace GlobalTest {
float allowedGlobal9 = 3.14;
// Pointers need to be double-const-qualified
- inline float& nonConstReference = disallowedGlobal1; // hidden-warning
{{'nonConstReference' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+ inline float& nonConstReference = disallowedGlobal1; // hidden-warning
{{'nonConstReference' may be duplicated when built into a shared library: it is
mutable, has hidden visibility, and external linkage}}
const inline int& constReference = allowedGlobal5;
- inline int* nonConstPointerToNonConst = nullptr; // hidden-warning
{{'nonConstPointerToNonConst' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
- inline int const* nonConstPointerToConst = nullptr; // hidden-warning
{{'nonConstPointerToConst' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
- inline int* const constPointerToNonConst = nullptr; // hidden-warning
{{'constPointerToNonConst' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ inline int* nonConstPointerToNonConst = nullptr; // hidden-warning
{{'nonConstPointerToNonConst' may be duplicated when built into a shared
library: it is mutable, has hidden visibility, and external linkage}}
+ inline int const* nonConstPointerToConst = nullptr; // hidden-warning
{{'nonConstPointerToConst' may be duplicated when built into a shared library:
it is mutable, has hidden visibility, and external linkage}}
+ inline int* const constPointerToNonConst = nullptr; // hidden-warning
{{'constPointerToNonConst' may be duplicated when built into a shared library:
it is mutable, has hidden visibility, and external linkage}}
inline int const* const constPointerToConst = nullptr;
// Don't warn on new because it tends to generate false positives
inline int const* const constPointerToConstNew = new int(7);
inline int const * const * const * const nestedConstPointer = nullptr;
- inline int const * const ** const * const nestedNonConstPointer = nullptr;
// hidden-warning {{'nestedNonConstPointer' is mutable, has hidden visibility,
and external linkage; it may be duplicated when built into a shared library}}
+ inline int const * const ** const * const nestedNonConstPointer = nullptr;
// hidden-warning {{'nestedNonConstPointer' may be duplicated when built into a
shared library: it is mutable, has hidden visibility, and external linkage}}
struct Test {
- static inline float disallowedStaticMember1; // hidden-warning
{{'disallowedStaticMember1' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ static inline float disallowedStaticMember1; // hidden-warning
{{'disallowedStaticMember1' may be duplicated when built into a shared library:
it is mutable, has hidden visibility, and external linkage}}
// Defined below, in the header file
static float disallowedStaticMember2;
// Defined in the cpp file, so won't get duplicated
@@ -183,5 +153,5 @@ namespace GlobalTest {
// may still cause problems if their address is taken.
};
- inline float Test::disallowedStaticMember2 = 2.3; // hidden-warning
{{'disallowedStaticMember2' is mutable, has hidden visibility, and external
linkage; it may be duplicated when built into a shared library}}
+ 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
>From d7e4ba745e8b3049ea9dd98ffeaadd18eeda8fa9 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dlo...@google.com>
Date: Thu, 30 Jan 2025 20:14:17 +0000
Subject: [PATCH 5/6] Remove redundant null check
---
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 a9730f60a34994..74e0fcec2d911b 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13382,7 +13382,7 @@ void Sema::checkNonTrivialCUnion(QualType QT,
SourceLocation Loc,
bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
const VarDecl *Dcl) {
- if (!Dcl || !getLangOpts().CPlusPlus)
+ if (!getLangOpts().CPlusPlus)
return false;
// We only need to warn if the definition is in a header file, so wait to
>From 71ef635c2cccfe542e900e7061ea0a48b916cf17 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dlo...@google.com>
Date: Mon, 3 Feb 2025 16:13:37 +0000
Subject: [PATCH 6/6] Expect test failures on PS4/PS5 as well because they also
use import/export semantics
---
clang/test/SemaCXX/unique_object_duplication.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/SemaCXX/unique_object_duplication.cpp
b/clang/test/SemaCXX/unique_object_duplication.cpp
index d08d053c84ae3c..8a19fb7b81187a 100644
--- a/clang/test/SemaCXX/unique_object_duplication.cpp
+++ b/clang/test/SemaCXX/unique_object_duplication.cpp
@@ -1,7 +1,7 @@
// 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. The test should fail because
we're not getting the expected warnings.
-// XFAIL: target={{.*}}-windows{{.*}}
+// XFAIL: target={{.*}}-windows{{.*}}, {{.*}}-ps{{(4|5)(-.+)?}}
#include "unique_object_duplication.h"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
