[clang] cdeeb39 - Revert "Warn when unique objects might be duplicated in shared libraries (#117622)"
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 3 04:59:14 PST 2025
Author: Hans Wennborg
Date: 2025-02-03T13:56:46+01:00
New Revision: cdeeb390a9ea21540fc44ba10dede66fbc0b2fc8
URL: https://github.com/llvm/llvm-project/commit/cdeeb390a9ea21540fc44ba10dede66fbc0b2fc8
DIFF: https://github.com/llvm/llvm-project/commit/cdeeb390a9ea21540fc44ba10dede66fbc0b2fc8.diff
LOG: Revert "Warn when unique objects might be duplicated in shared libraries (#117622)"
There are some buildbot breakages, see the PR. Reverting for now.
This reverts commit d5684b8402d2175e80a2792db58e9a8e960a8544.
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
Removed:
clang/test/SemaCXX/unique_object_duplication.cpp
clang/test/SemaCXX/unique_object_duplication.h
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a0cafd68c677df..30364a747f9c19 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -113,10 +113,6 @@ 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 05e39899e6f257..abb575002e1182 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -694,36 +694,6 @@ 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
diff erentiation 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 b35c48e7104dc7..00a94eb7a30367 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6167,15 +6167,6 @@ 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 59e29262e35047..472a0e25adc975 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3664,12 +3664,6 @@ 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 74e0fcec2d911b..1755b37fc8f295 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13380,62 +13380,6 @@ 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
diff erent 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.
@@ -14842,51 +14786,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);
diff --git a/clang/test/SemaCXX/unique_object_duplication.cpp b/clang/test/SemaCXX/unique_object_duplication.cpp
deleted file mode 100644
index d08d053c84ae3c..00000000000000
--- a/clang/test/SemaCXX/unique_object_duplication.cpp
+++ /dev/null
@@ -1,16 +0,0 @@
-// 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
deleted file mode 100644
index 5b2002c31be7c3..00000000000000
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ /dev/null
@@ -1,157 +0,0 @@
-/**
- * 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
More information about the cfe-commits
mailing list