Skip to content

Commit 9acba88

Browse files
committed
Warn when unique objects might be duplicated in shared libraries
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 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 if one of the following is true: 1. The object is mutable (the copies won't be in sync), or 2. Its initialization has side effects (it may now run more than once), or 3. The value of its address is used (which one?). 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.
1 parent b0ca543 commit 9acba88

File tree

6 files changed

+345
-0
lines changed

6 files changed

+345
-0
lines changed

clang/include/clang/Basic/DiagnosticGroups.td

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -690,6 +690,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
690690
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
691691
def StaticInInline : DiagGroup<"static-in-inline">;
692692
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
693+
def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
693694
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
694695
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
695696
// Allow differentiation between GNU statement expressions in a macro versus

clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6104,6 +6104,15 @@ def warn_static_local_in_extern_inline : Warning<
61046104
def note_convert_inline_to_static : Note<
61056105
"use 'static' to give inline function %0 internal linkage">;
61066106

6107+
def warn_possible_object_duplication_mutable : Warning<
6108+
"%0 is mutable, has hidden visibility, and external linkage; it may be "
6109+
"duplicated when built into a shared library">,
6110+
InGroup<UniqueObjectDuplication>;
6111+
def warn_possible_object_duplication_init : Warning<
6112+
"%0 has hidden visibility, and external linkage; its initialization may run "
6113+
"more than once when built into a shared library">,
6114+
InGroup<UniqueObjectDuplication>;
6115+
61076116
def ext_redefinition_of_typedef : ExtWarn<
61086117
"redefinition of typedef %0 is a C11 feature">,
61096118
InGroup<DiagGroup<"typedef-redefinition"> >;

clang/include/clang/Sema/Sema.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3643,6 +3643,12 @@ class Sema final : public SemaBase {
36433643
NonTrivialCUnionContext UseContext,
36443644
unsigned NonTrivialKind);
36453645

3646+
/// Certain globally-unique variables might be accidentally duplicated if
3647+
/// built into multiple shared libraries with hidden visibility. This can
3648+
/// cause problems if the variable is mutable, its initialization is
3649+
/// effectful, or its address is taken.
3650+
bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *dcl);
3651+
36463652
/// AddInitializerToDecl - Adds the initializer Init to the
36473653
/// declaration dcl. If DirectInit is true, this is C++ direct
36483654
/// initialization rather than copy initialization.

clang/lib/Sema/SemaDecl.cpp

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13373,6 +13373,62 @@ void Sema::checkNonTrivialCUnion(QualType QT, SourceLocation Loc,
1337313373
.visit(QT, nullptr, false);
1337413374
}
1337513375

13376+
bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
13377+
const VarDecl *dcl) {
13378+
if (!dcl || !getLangOpts().CPlusPlus)
13379+
return false;
13380+
13381+
// If an object is defined in a source file, its definition can't get
13382+
// duplicated since it will never appear in more than one TU.
13383+
if (dcl->getASTContext().getSourceManager().isInMainFile(dcl->getLocation()))
13384+
return false;
13385+
13386+
// We only need to warn if the definition is in a header file, so wait to
13387+
// diagnose until we've seen the definition.
13388+
if (!dcl->isThisDeclarationADefinition())
13389+
return false;
13390+
13391+
// If the variable we're looking at is a static local, then we actually care
13392+
// about the properties of the function containing it.
13393+
const ValueDecl *target = dcl;
13394+
// VarDecls and FunctionDecls have different functions for checking
13395+
// inline-ness, so we have to do it manually.
13396+
bool target_is_inline = dcl->isInline();
13397+
13398+
// Update the target and target_is_inline property if necessary
13399+
if (dcl->isStaticLocal()) {
13400+
const DeclContext *ctx = dcl->getDeclContext();
13401+
if (!ctx)
13402+
return false;
13403+
13404+
const FunctionDecl *f_dcl =
13405+
dyn_cast_if_present<FunctionDecl>(ctx->getNonClosureAncestor());
13406+
if (!f_dcl)
13407+
return false;
13408+
13409+
target = f_dcl;
13410+
// IsInlined() checks for the C++ inline property
13411+
target_is_inline = f_dcl->isInlined();
13412+
}
13413+
13414+
// Non-inline variables can only legally appear in one TU
13415+
// FIXME: This also applies to templated variables, but that can rarely lead
13416+
// to false positives so templates are disabled for now.
13417+
if (!target_is_inline)
13418+
return false;
13419+
13420+
// If the object isn't hidden, the dynamic linker will prevent duplication.
13421+
clang::LinkageInfo lnk = target->getLinkageAndVisibility();
13422+
if (lnk.getVisibility() != HiddenVisibility)
13423+
return false;
13424+
13425+
// If the obj doesn't have external linkage, it's supposed to be duplicated.
13426+
if (!isExternalFormalLinkage(lnk.getLinkage()))
13427+
return false;
13428+
13429+
return true;
13430+
}
13431+
1337613432
void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
1337713433
// If there is no declaration, there was an error parsing it. Just ignore
1337813434
// the initializer.
@@ -14777,6 +14833,51 @@ void Sema::FinalizeDeclaration(Decl *ThisDecl) {
1477714833
if (DC->getRedeclContext()->isFileContext() && VD->isExternallyVisible())
1477814834
AddPushedVisibilityAttribute(VD);
1477914835

14836+
// If this object has external linkage and hidden visibility, it might be
14837+
// duplicated when built into a shared library, which causes problems if it's
14838+
// mutable (since the copies won't be in sync) or its initialization has side
14839+
// effects (since it will run once per copy instead of once globally)
14840+
// FIXME: Windows uses dllexport/dllimport instead of visibility, and we don't
14841+
// handle that yet. Disable the warning on Windows for now.
14842+
// FIXME: Checking templates can cause false positives if the template in
14843+
// question is never instantiated (e.g. only specialized templates are used).
14844+
if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
14845+
!VD->isTemplated() &&
14846+
GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
14847+
// Check mutability. For pointers, ensure that both the pointer and the
14848+
// pointee are (recursively) const.
14849+
QualType Type = VD->getType().getNonReferenceType();
14850+
if (!Type.isConstant(VD->getASTContext())) {
14851+
Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
14852+
<< VD;
14853+
} else {
14854+
while (Type->isPointerType()) {
14855+
Type = Type->getPointeeType();
14856+
if (Type->isFunctionType())
14857+
break;
14858+
if (!Type.isConstant(VD->getASTContext())) {
14859+
Diag(VD->getLocation(),
14860+
diag::warn_possible_object_duplication_mutable)
14861+
<< VD;
14862+
break;
14863+
}
14864+
}
14865+
}
14866+
14867+
// To keep false positives low, only warn if we're certain that the
14868+
// initializer has side effects. Don't warn on operator new, since a mutable
14869+
// pointer will trigger the previous warning, and an immutable pointer
14870+
// getting duplicated just results in a little extra memory usage.
14871+
const Expr *Init = VD->getAnyInitializer();
14872+
if (Init &&
14873+
Init->HasSideEffects(VD->getASTContext(),
14874+
/*IncludePossibleEffects=*/false) &&
14875+
!isa<CXXNewExpr>(Init->IgnoreParenImpCasts())) {
14876+
Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init)
14877+
<< VD;
14878+
}
14879+
}
14880+
1478014881
// FIXME: Warn on unused var template partial specializations.
1478114882
if (VD->isFileVarDecl() && !isa<VarTemplatePartialSpecializationDecl>(VD))
1478214883
MarkUnusedFileScopedDecl(VD);
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// RUN: %clang_cc1 -fsyntax-only -verify=hidden -Wunique-object-duplication -fvisibility=hidden -Wno-unused-value %s
2+
// RUN: %clang_cc1 -fsyntax-only -verify -Wunique-object-duplication -Wno-unused-value %s
3+
// The check is currently disabled on windows. The test should fail because we're not getting the expected warnings.
4+
// XFAIL: target={{.*}}-windows{{.*}}
5+
6+
#include "unique_object_duplication.h"
7+
8+
// Everything in these namespaces here is defined in the cpp file,
9+
// so won't get duplicated
10+
11+
namespace GlobalTest {
12+
float Test::allowedStaticMember1 = 2.3;
13+
}
14+
15+
bool disallowed4 = true;
16+
constexpr inline bool disallowed5 = true;
Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
/**
2+
* When building shared libraries, hidden objects which are defined in header
3+
* files will be duplicated, with one copy in each shared library. If the object
4+
* was meant to be globally unique (one copy per program), this can cause very
5+
* subtle bugs. This file contains tests for the -Wunique-object-duplication
6+
* warning, which is meant to detect this.
7+
*
8+
* Roughly, an object might be incorrectly duplicated if:
9+
* - Is defined in a header (so it might appear in multiple TUs), and
10+
* - Has external linkage (otherwise it's supposed to be duplicated), and
11+
* - Has hidden visibility (or else the dynamic linker will handle it)
12+
*
13+
* Duplication becomes an issue only if one of the following is true:
14+
* - The object is mutable (the copies won't be in sync), or
15+
* - Its initialization may has side effects (it may now run more than once), or
16+
* - The value of its address is used.
17+
*
18+
* Currently, we only detect the first two, and only warn on effectful
19+
* initialization if we're certain there are side effects. Warning if the
20+
* address is taken is prone to false positives, so we don't warn for now.
21+
*
22+
* The check is also disabled on Windows for now, since it uses
23+
* dllimport/dllexport instead of visibility.
24+
*/
25+
26+
#define HIDDEN __attribute__((visibility("hidden")))
27+
#define DEFAULT __attribute__((visibility("default")))
28+
29+
// Helper functions
30+
constexpr int init_constexpr(int x) { return x; };
31+
extern double init_dynamic(int);
32+
33+
/******************************************************************************
34+
* Case one: Static local variables in an externally-visible function
35+
******************************************************************************/
36+
namespace StaticLocalTest {
37+
38+
inline void has_static_locals_external() {
39+
// Mutable
40+
static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
41+
// Initialization might run more than once
42+
static const double disallowedStatic2 = disallowedStatic1++; // hidden-warning {{'disallowedStatic2' has hidden visibility, and external linkage; its initialization may run more than once when built into a shared library}}
43+
44+
// OK, because immutable and compile-time-initialized
45+
static constexpr int allowedStatic1 = 0;
46+
static const float allowedStatic2 = 1;
47+
static constexpr int allowedStatic3 = init_constexpr(2);
48+
static const int allowedStatic4 = init_constexpr(3);
49+
}
50+
51+
// Don't warn for non-inline functions, since they can't (legally) appear
52+
// in more than one TU in the first place.
53+
void has_static_locals_non_inline() {
54+
// Mutable
55+
static int allowedStatic1 = 0;
56+
// Initialization might run more than once
57+
static const double allowedStatic2 = allowedStatic1++;
58+
}
59+
60+
template<typename T>
61+
void has_static_locals_templated() {
62+
// Mutable
63+
static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
64+
// Initialization might run more than once
65+
static const double disallowedStatic2 = disallowedStatic1++; // hidden-warning {{'disallowedStatic2' has hidden visibility, and external linkage; its initialization may run more than once when built into a shared library}}
66+
67+
// OK, because immutable and compile-time-initialized
68+
static constexpr int allowedStatic1 = 0;
69+
static const float allowedStatic2 = 1;
70+
static constexpr int allowedStatic3 = init_constexpr(2);
71+
static const int allowedStatic4 = init_constexpr(3);
72+
}
73+
74+
// Everything in this function is OK because the function is TU-local
75+
static void has_static_locals_internal() {
76+
static int allowedStatic1 = 0;
77+
static double allowedStatic2 = init_dynamic(2);
78+
static char allowedStatic3 = []() { return allowedStatic1++; }();
79+
80+
static constexpr int allowedStatic4 = 0;
81+
static const float allowedStatic5 = 1;
82+
static constexpr int allowedStatic6 = init_constexpr(2);
83+
static const int allowedStatic7 = init_constexpr(3);
84+
}
85+
86+
namespace {
87+
88+
// Everything in this function is OK because the function is also TU-local
89+
void has_static_locals_anon() {
90+
static int allowedStatic1 = 0;
91+
static double allowedStatic2 = init_dynamic(2);
92+
static char allowedStatic3 = []() { return allowedStatic1++; }();
93+
94+
static constexpr int allowedStatic4 = 0;
95+
static const float allowedStatic5 = 1;
96+
static constexpr int allowedStatic6 = init_constexpr(2);
97+
static const int allowedStatic7 = init_constexpr(3);
98+
}
99+
100+
} // Anonymous namespace
101+
102+
HIDDEN inline void static_local_always_hidden() {
103+
static int disallowedStatic1 = 3; // hidden-warning {{'disallowedStatic1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
104+
// expected-warning@-1 {{'disallowedStatic1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
105+
{
106+
static int disallowedStatic2 = 3; // hidden-warning {{'disallowedStatic2' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
107+
// expected-warning@-1 {{'disallowedStatic2' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
108+
}
109+
110+
auto lmb = []() {
111+
static int disallowedStatic3 = 3; // hidden-warning {{'disallowedStatic3' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
112+
// expected-warning@-1 {{'disallowedStatic3' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
113+
};
114+
}
115+
116+
DEFAULT void static_local_never_hidden() {
117+
static int allowedStatic1 = 3;
118+
119+
{
120+
static int allowedStatic2 = 3;
121+
}
122+
123+
auto lmb = []() {
124+
static int allowedStatic3 = 3;
125+
};
126+
}
127+
128+
// Don't warn on this because it's not in a function
129+
const int setByLambda = ([]() { static int x = 3; return x++; })();
130+
131+
inline void has_extern_local() {
132+
extern int allowedAddressExtern; // Not a definition
133+
}
134+
135+
inline void has_regular_local() {
136+
int allowedAddressLocal = 0;
137+
}
138+
139+
inline void has_thread_local() {
140+
// thread_local variables are static by default
141+
thread_local int disallowedThreadLocal = 0; // hidden-warning {{'disallowedThreadLocal' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
142+
}
143+
144+
} // namespace StaticLocalTest
145+
146+
/******************************************************************************
147+
* Case two: Globals with external linkage
148+
******************************************************************************/
149+
namespace GlobalTest {
150+
// Mutable
151+
inline float disallowedGlobal1 = 3.14; // hidden-warning {{'disallowedGlobal1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
152+
// Same as above, but explicitly marked inline
153+
inline float disallowedGlobal4 = 3.14; // hidden-warning {{'disallowedGlobal4' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
154+
155+
// Initialization might run more than once
156+
inline const double disallowedGlobal5 = disallowedGlobal1++; // hidden-warning {{'disallowedGlobal5' has hidden visibility, and external linkage; its initialization may run more than once when built into a shared library}}
157+
158+
// OK because internal linkage, so duplication is intended
159+
static float allowedGlobal1 = 3.14;
160+
const double allowedGlobal2 = init_dynamic(2);
161+
static const char allowedGlobal3 = []() { return disallowedGlobal1++; }();
162+
static inline double allowedGlobal4 = init_dynamic(2);
163+
164+
// OK, because immutable and compile-time-initialized
165+
constexpr int allowedGlobal5 = 0;
166+
const float allowedGlobal6 = 1;
167+
constexpr int allowedGlobal7 = init_constexpr(2);
168+
const int allowedGlobal8 = init_constexpr(3);
169+
170+
// We don't warn on this because non-inline variables can't (legally) appear
171+
// in more than one TU.
172+
float allowedGlobal9 = 3.14;
173+
174+
template<typename T>
175+
T templatedGlobal = T(9); // hidden-warning {{'templatedGlobal' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
176+
177+
template<typename T>
178+
int templatedGlobal<T*> = T(3); // hidden-warning {{'templatedGlobal<T *>' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
179+
180+
// Don't warn on this because each explicit specialization can't (legally) appear
181+
// in more than one TU.
182+
template<>
183+
int templatedGlobal<int*> = 8;
184+
185+
// Pointers need to be double-const-qualified
186+
inline float& nonConstReference = disallowedGlobal1; // hidden-warning {{'nonConstReference' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
187+
const inline int& constReference = allowedGlobal5;
188+
189+
inline int* nonConstPointerToNonConst = nullptr; // hidden-warning {{'nonConstPointerToNonConst' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
190+
inline int const* nonConstPointerToConst = nullptr; // hidden-warning {{'nonConstPointerToConst' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
191+
inline int* const constPointerToNonConst = nullptr; // hidden-warning {{'constPointerToNonConst' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
192+
inline int const* const constPointerToConst = nullptr;
193+
// Don't warn on new because it tends to generate false positives
194+
inline int const* const constPointerToConstNew = new int(7);
195+
196+
inline int const * const * const * const nestedConstPointer = nullptr;
197+
inline int const * const ** const * const nestedNonConstPointer = nullptr; // hidden-warning {{'nestedNonConstPointer' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
198+
199+
struct Test {
200+
static inline float disallowedStaticMember1; // hidden-warning {{'disallowedStaticMember1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
201+
// Defined below, in the header file
202+
static float disallowedStaticMember2;
203+
// Defined in the cpp file, so won't get duplicated
204+
static float allowedStaticMember1;
205+
206+
// Tests here are sparse because the AddrTest case below will define plenty
207+
// more, which aren't problematic to define (because they're immutable), but
208+
// may still cause problems if their address is taken.
209+
};
210+
211+
inline float Test::disallowedStaticMember2 = 2.3; // hidden-warning {{'disallowedStaticMember2' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
212+
} // namespace GlobalTest

0 commit comments

Comments
 (0)