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@-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@-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@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
