llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Devon Loehr (DKLoehr) <details> <summary>Changes</summary> Followup to #<!-- -->125526. This expands the logic of the unique-object-duplication warning so that it also works for windows code. For the most part, the logic is unchanged, merely substituting "has no import/export annotation" in place of "has hidden visibility". However, there are some small inconsistencies between the two; namely, visibility is propagated through nested classes, while import/export annotations aren't. This PR: 1. Updates the logic for the warning to account for the differences between posix and windows 2. Changes the warning message and documentation appropriately 3. Updates the tests to cover windows, and adds new test cases for the places where behavior differs. This PR was tested by building chromium (cross compiling linux->windows) with the changes in place. After accounting for the differences in semantics, no new warnings were discovered. --- Patch is 22.69 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143537.diff 5 Files Affected: - (modified) clang/include/clang/Basic/DiagnosticGroups.td (+10-6) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+13-8) - (modified) clang/lib/Sema/SemaDecl.cpp (+25-7) - (modified) clang/test/SemaCXX/unique_object_duplication.cpp (+6-4) - (modified) clang/test/SemaCXX/unique_object_duplication.h (+61-29) ``````````diff diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index beda73e675fc6..dcf14dd0e9719 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -806,7 +806,9 @@ def UniqueObjectDuplication : DiagGroup<"unique-object-duplication"> { 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 +This can occur to objects which are hidden from the dynamic linker, due to +having hidden visibility (on posix) or lacking an import/export annotation (on +windows). If such an object 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 @@ -815,7 +817,7 @@ once per copy, etc. Specifically, this warning fires when it detects an object which: 1. Is defined as ``inline`` 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. +3. Has hidden visibility (posix) or lacks an import/export annotation (windows). As well as one of the following: 1. The object is mutable, or @@ -825,13 +827,15 @@ The warning can be resolved by removing one of the conditions above. In rough order of preference, this may be done by: 1. Marking the object ``const`` (if possible) 2. Moving the object's definition to a source file -3. Giving the object non-hidden visibility, e.g. using ``__attribute((visibility("default")))``. +3. Making the object visible using ``__attribute((visibility("default")))``, + ``__declspec(dllimport)``, or ``__declspec(dllexport)``. + +When annotating an object with ``__declspec(dllimport)`` or ``__declspec(dllexport)``, +take care to ensure that the object is only imported in one dll, and is exported +everywhere else. Note that for (2), all levels of a pointer variable must be constant; ``const int*`` will trigger the warning because the pointer itself is mutable. - -This warning is not yet implemented for Windows, since Windows uses -import/export rules instead of visibility. }]; } diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 1f283b776a02c..017c315b9ed2d 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6260,14 +6260,19 @@ 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< - "initialization of %0 may run twice when built into a shared library: " - "it has hidden visibility and external linkage">, - InGroup<UniqueObjectDuplication>, DefaultIgnore; +def warn_possible_object_duplication_mutable + : Warning<"%0 may be duplicated when built into a shared library: " + "it is mutable, with external linkage and " + "%select{hidden visibility|no import/export annotation}1">, + InGroup<UniqueObjectDuplication>, + DefaultIgnore; +def warn_possible_object_duplication_init + : Warning<"initialization of %0 may run twice when built into a shared " + "library: " + "it has external linkage and " + "%select{hidden visibility|no import/export annotation}1">, + InGroup<UniqueObjectDuplication>, + DefaultIgnore; def ext_redefinition_of_typedef : ExtWarn< "redefinition of typedef %0 is a C11 feature">, diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index bbd63372c168b..f4c3d399c4158 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -13518,8 +13518,29 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated( // If the object isn't hidden, the dynamic linker will prevent duplication. clang::LinkageInfo Lnk = Target->getLinkageAndVisibility(); - if (Lnk.getVisibility() != HiddenVisibility) + + // The target is "hidden" (from the dynamic linker) if: + // 1. On posix, it has hidden visibility, or + // 2. On windows, it has no import/export annotation + if (Context.getTargetInfo().shouldDLLImportComdatSymbols()) { + if (Target->hasAttr<DLLExportAttr>() || Target->hasAttr<DLLImportAttr>()) + return false; + + // If the variable isn't directly annotated, check to see if it's a member + // of an annotated class. + const VarDecl *VD = dyn_cast_if_present<VarDecl>(Target); + + if (VD && VD->isStaticDataMember()) { + const CXXRecordDecl *Ctx = + dyn_cast_if_present<CXXRecordDecl>(VD->getDeclContext()); + if (Ctx && + (Ctx->hasAttr<DLLExportAttr>() || Ctx->hasAttr<DLLImportAttr>())) + return false; + } + } else if (Lnk.getVisibility() != HiddenVisibility) { + // Posix case return false; + } // If the obj doesn't have external linkage, it's supposed to be duplicated. if (!isExternalFormalLinkage(Lnk.getLinkage())) @@ -13550,19 +13571,16 @@ void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) { // 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. // Don't diagnose if we're inside a template, because it's not practical to // fix the warning in most cases. - if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() && - !VD->isTemplated() && + if (!VD->isTemplated() && GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) { QualType Type = VD->getType(); if (looksMutable(Type, VD->getASTContext())) { Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable) - << VD; + << VD << Context.getTargetInfo().shouldDLLImportComdatSymbols(); } // To keep false positives low, only warn if we're certain that the @@ -13575,7 +13593,7 @@ void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) { /*IncludePossibleEffects=*/false) && !isa<CXXNewExpr>(Init->IgnoreParenImpCasts())) { Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init) - << VD; + << VD << Context.getTargetInfo().shouldDLLImportComdatSymbols(); } } } diff --git a/clang/test/SemaCXX/unique_object_duplication.cpp b/clang/test/SemaCXX/unique_object_duplication.cpp index 4b41bfbfdc2f7..ff3b85d19fa67 100644 --- a/clang/test/SemaCXX/unique_object_duplication.cpp +++ b/clang/test/SemaCXX/unique_object_duplication.cpp @@ -1,7 +1,9 @@ -// 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 in MSVC-like environments. The test should fail because we're not getting the expected warnings. -// XFAIL: target={{.*}}-windows-msvc, {{.*}}-ps{{(4|5)(-.+)?}} +// RUN: %clang_cc1 -fsyntax-only -Wunique-object-duplication -Wno-unused-value \ +// RUN: -verify -triple=x86_64-pc-linux-gnu %s +// RUN: %clang_cc1 -fsyntax-only -Wunique-object-duplication -Wno-unused-value \ +// RUN: -verify=hidden -triple=x86_64-pc-linux-gnu -fvisibility=hidden %s +// RUN: %clang_cc1 -fsyntax-only -Wunique-object-duplication -Wno-unused-value \ +// RUN: -verify=windows -triple=x86_64-windows-msvc -DWINDOWS_TEST -fdeclspec %s #include "unique_object_duplication.h" diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h index 537429d9ebdaa..bd0ee6bd14d64 100644 --- a/clang/test/SemaCXX/unique_object_duplication.h +++ b/clang/test/SemaCXX/unique_object_duplication.h @@ -3,8 +3,14 @@ * See the warning's documentation for more information. */ +#ifdef WINDOWS_TEST +#define HIDDEN +// dllimport also suffices for visibility, but those can't have definitions +#define VISIBLE __declspec(dllexport) +#else #define HIDDEN __attribute__((visibility("hidden"))) -#define DEFAULT __attribute__((visibility("default"))) +#define VISIBLE __attribute__((visibility("default"))) +#endif // Helper functions constexpr int init_constexpr(int x) { return x; }; @@ -17,10 +23,11 @@ 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}} + static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-1 {{'disallowedStatic1' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} // Initialization might run more than once - static const double disallowedStatic2 = disallowedStatic1++; // hidden-warning {{initialization of 'disallowedStatic2' may run twice when built into a shared library: it has hidden visibility and external linkage}} - + static const double disallowedStatic2 = disallowedStatic1++; // hidden-warning {{initialization of 'disallowedStatic2' may run twice when built into a shared library: it has external linkage and hidden visibility}} + // windows-warning@-1 {{initialization of 'disallowedStatic2' may run twice when built into a shared library: it has external linkage and no import/export annotation}} // OK, because immutable and compile-time-initialized static constexpr int allowedStatic1 = 0; static const float allowedStatic2 = 1; @@ -53,29 +60,33 @@ void has_static_locals_anon() { 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 disallowedStatic1 = 3; // hidden-warning {{'disallowedStatic1' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // expected-warning@-1 {{'disallowedStatic1' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-2 {{'disallowedStatic1' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} { - 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}} + static int disallowedStatic2 = 3; // hidden-warning {{'disallowedStatic2' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // expected-warning@-1 {{'disallowedStatic2' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-2 {{'disallowedStatic2' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} } 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}} + static int disallowedStatic3 = 3; // hidden-warning {{'disallowedStatic3' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // expected-warning@-1 {{'disallowedStatic3' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-2 {{'disallowedStatic3' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} }; } -DEFAULT void static_local_never_hidden() { - static int allowedStatic1 = 3; +// Always visible +VISIBLE void static_local_never_hidden() { + static int allowedStatic1 = 3; { - static int allowedStatic2 = 3; + static int allowedStatic2 = 3; } auto lmb = []() { @@ -96,7 +107,8 @@ 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' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}} + thread_local int disallowedThreadLocal = 0; // hidden-warning {{'disallowedThreadLocal' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-1 {{'disallowedThreadLocal' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} } // Functions themselves are always immutable, so referencing them is okay @@ -109,11 +121,13 @@ inline auto& allowedFunctionReference = has_static_locals_external; ******************************************************************************/ 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 {{initialization of 'disallowedGlobal5' may run twice when built into a shared library: it has hidden visibility and external linkage}} + inline float disallowedGlobal1 = 3.14; // hidden-warning {{'disallowedGlobal1' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-1 {{'disallowedGlobal1' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} + + // Initialization might run more than once + inline const double disallowedGlobal5 = disallowedGlobal1++; // hidden-warning {{initialization of 'disallowedGlobal5' may run twice when built into a shared library: it has external linkage and hidden visibility}} + // windows-warning@-1 {{initialization of 'disallowedGlobal5' may run twice when built into a shared library: it has external linkage and no import/export annotation}} // OK because internal linkage, so duplication is intended static float allowedGlobal1 = 3.14; const double allowedGlobal2 = init_dynamic(2); @@ -129,34 +143,52 @@ namespace GlobalTest { // 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}} + inline float& nonConstReference = disallowedGlobal1; // hidden-warning {{'nonConstReference' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-1 {{'nonConstReference' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} 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* nonConstPointerToNonConst = nullptr; // hidden-warning {{'nonConstPointerToNonConst' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-1 {{'nonConstPointerToNonConst' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} + inline int const* nonConstPointerToConst = nullptr; // hidden-warning {{'nonConstPointerToConst' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-1 {{'nonConstPointerToConst' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} + inline int* const constPointerToNonConst = nullptr; // hidden-warning {{'constPointerToNonConst' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + // windows-warning@-1 {{'constPointerToNonConst' may be duplicated when built into a shared library: it is mutable, with external linkage and no import/export annotation}} 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}} + inline int const * const ** const * const nestedNonConstPointer = nullptr; // hidden-warning {{'nestedNonConstPointer' may be duplicated when built into a shared library: it is mutable, with external linkage and hidden visibility}} + ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/143537 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
