diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index a128726b99917..4266d6c523ab9 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 abb575002e118..05e39899e6f25 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -694,6 +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"> { + 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 5876b2a6ae0ea..7b3b932c482ba 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 may be duplicated when built into a shared library: " + "it is mutable, has hidden visibility, and external linkage">, + InGroup, 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, DefaultIgnore; + def ext_redefinition_of_typedef : ExtWarn< "redefinition of typedef %0 is a C11 feature">, InGroup >; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 472a0e25adc97..59e29262e3504 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 1755b37fc8f29..74e0fcec2d911 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 (!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(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. @@ -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(Init->IgnoreParenImpCasts())) { + Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init) + << VD; + } + } + // FIXME: Warn on unused var template partial specializations. if (VD->isFileVarDecl() && !isa(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 0000000000000..8a19fb7b81187 --- /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 0000000000000..5b2002c31be7c --- /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@-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