[clang] Warn when unique objects might be duplicated in shared libraries (PR #125526)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 3 08:17:07 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Devon Loehr (DKLoehr)
<details>
<summary>Changes</summary>
This is attempt 2 to merge this, the first one is #<!-- -->117622. This properly disables the tests when building for playstation, since the warning is disabled there.
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 data 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 semantically if one of the following is true:
The object is mutable (the copies won't be in sync), or
Its initialization has side effects (it may now run more than once), or
The value of its address is used (different copies have different addresses).
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.
Resolving the warning
The warning can be fixed in several ways:
If the object in question doesn't need to be mutable, it should be made const. Note that the variable must be completely immutable, e.g. we'll warn on const int* p because the pointer itself is mutable. To silence the warning, it should instead be const int* const p.
If the object must be mutable, it (or the enclosing function, in the case of static local variables) should be made visible using __attribute((visibility("default")))
If the object is supposed to be duplicated, it should be be given internal linkage.
Testing
I've tested the warning by running it on clang itself, as well as on chromium. Compiling clang resulted in [10 warnings across 6 files](https://github.com/user-attachments/files/17908069/clang-warnings.txt), while Chromium resulted in [160 warnings across 85 files](https://github.com/user-attachments/files/17908072/chromium-warnings.txt), mostly in third-party code. Almost all warnings were due to mutable variables.
I evaluated the warnings by manual inspection. I believe all the resulting warnings are true positives, i.e. they represent potentially-problematic code where duplication might cause a problem. For the clang warnings, I also validated them by either adding const or visibility annotations as appropriate.
Limitations
I am aware of four main limitations with the current warning:
We do not warn when the address of a duplicated object is taken, since doing so is prone to false positives. I'm hopeful that we can create a refined version in the future, however.
We only warn for side-effectful initialization if we are certain side effects exist. Warning on potential side effects produced a huge number of false positives; I don't expect there's much that can be done about this in modern C++ code bases, since proving a lack of side effects is difficult.
Windows uses a different system (declexport/import) instead of visibility. From manual testing, it seems to behave analogously to the visibility system for the purposes of this warning, but to keep things simple the warning is disabled on windows for now.
We don't warn on code inside templates. This is unfortuate, since it masks many real issues, e.g. a templated variable which is implicitly instantiated the same way in multiple TUs should be globally unique, but may accidentally be duplicated. Unfortunately, we found some potential false positives during testing that caused us to disable the warning for now.
---
Full diff: https://github.com/llvm/llvm-project/pull/125526.diff
7 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+4)
- (modified) clang/include/clang/Basic/DiagnosticGroups.td (+30)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+9)
- (modified) clang/include/clang/Sema/Sema.h (+6)
- (modified) clang/lib/Sema/SemaDecl.cpp (+101)
- (added) clang/test/SemaCXX/unique_object_duplication.cpp (+16)
- (added) clang/test/SemaCXX/unique_object_duplication.h (+157)
``````````diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7fafe2807bd3883..e607ef488f653e3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -103,6 +103,10 @@ Attribute Changes in Clang
Improvements to Clang's diagnostics
-----------------------------------
+- 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 209792f851b6aea..3bd34ac515d6468 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,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"> {
+ 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
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db911ed121e9514..aff2a811bc6b27b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6162,6 +6162,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 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<
+ "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<
"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 d0fc735be5f7636..7642ec28dfe4fc3 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3663,6 +3663,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 3cad9827fdab694..2e68be4d4eff70f 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13377,6 +13377,62 @@ void Sema::checkNonTrivialCUnion(QualType QT, SourceLocation Loc,
.visit(QT, nullptr, false);
}
+bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
+ const VarDecl *Dcl) {
+ if (!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())
+ 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;
+ // VarDecls and FunctionDecls have different functions for checking
+ // inline-ness, so we have to do it manually.
+ bool TargetIsInline = Dcl->isInline();
+
+ // Update the Target and TargetIsInline property if necessary
+ if (Dcl->isStaticLocal()) {
+ const DeclContext *Ctx = Dcl->getDeclContext();
+ if (!Ctx)
+ return false;
+
+ const FunctionDecl *FunDcl =
+ dyn_cast_if_present<FunctionDecl>(Ctx->getNonClosureAncestor());
+ if (!FunDcl)
+ return false;
+
+ Target = FunDcl;
+ // IsInlined() checks for the C++ inline property
+ 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 (!TargetIsInline)
+ 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.
@@ -14783,6 +14839,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 000000000000000..404ceda9fef4be2
--- /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{{.*}}, {{.*}}-ps{{4|5(-.*)?}}
+
+#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 000000000000000..5b2002c31be7c3c
--- /dev/null
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -0,0 +1,157 @@
+/**
+ * This file contains tests for the -Wunique_object_duplication warning.
+ * See the warning's documentation for more information.
+ */
+
+#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' 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 {{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;
+ 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;
+}
+
+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 = init_constexpr(3);
+}
+
+} // Anonymous namespace
+
+HIDDEN inline void static_local_always_hidden() {
+ 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 at -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' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+ // expected-warning at -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' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+ // expected-warning at -1 {{'disallowedStatic3' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+ };
+}
+
+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' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+}
+
+} // namespace StaticLocalTest
+
+/******************************************************************************
+ * Case two: Globals with external linkage
+ ******************************************************************************/
+namespace GlobalTest {
+ // Mutable
+ 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 {{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;
+ 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' 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' 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' 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' 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
+ 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' 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
``````````
</details>
https://github.com/llvm/llvm-project/pull/125526
More information about the cfe-commits
mailing list