From 405e2dbf3767586ab06a8506a6f5754aeb5a613d Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 20 Sep 2017 07:22:00 +0000 Subject: [PATCH] Implement C++ [basic.link]p8. If a function or variable has a type with no linkage (and is not extern "C"), any use of it requires a definition within the same translation unit; the idea is that it is not possible to define the entity elsewhere, so any such use is necessarily an error. There is an exception, though: some types formally have no linkage but nonetheless can be referenced from other translation units (for example, this happens to anonymous structures defined within inline functions). For entities with those types, we suppress the diagnostic except under -pedantic. llvm-svn: 313729 --- clang/include/clang/Basic/DiagnosticSemaKinds.td | 8 +++ clang/include/clang/Sema/Sema.h | 5 ++ clang/include/clang/Sema/SemaInternal.h | 3 +- clang/lib/AST/Decl.cpp | 12 ++-- clang/lib/Sema/Sema.cpp | 79 +++++++++++++++++------- clang/lib/Sema/SemaDecl.cpp | 14 +---- clang/lib/Sema/SemaExpr.cpp | 2 + clang/test/Analysis/malloc.mm | 2 +- clang/test/Analysis/reference.cpp | 11 ++-- clang/test/CXX/basic/basic.link/p8.cpp | 69 +++++++++++++++++++++ clang/test/CodeGenCXX/debug-info-method.cpp | 3 + clang/test/CodeGenCXX/mangle-ms-cxx11.cpp | 7 ++- clang/test/CodeGenCXX/mangle.cpp | 3 +- clang/test/PCH/cxx11-lambdas.mm | 2 +- clang/test/SemaCXX/warn-unreachable.cpp | 16 +++-- 15 files changed, 180 insertions(+), 56 deletions(-) create mode 100644 clang/test/CXX/basic/basic.link/p8.cpp diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 3974fe0..16fee891 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4707,6 +4707,14 @@ def note_deleted_assign_field : Note< def warn_undefined_internal : Warning< "%select{function|variable}0 %q1 has internal linkage but is not defined">, InGroup>; +def err_undefined_internal_type : Error< + "%select{function|variable}0 %q1 is used but not defined in this " + "translation unit, and cannot be defined in any other translation unit " + "because its type does not have linkage">; +def ext_undefined_internal_type : Extension< + "ISO C++ requires a definition in this translation unit for " + "%select{function|variable}0 %q1 because its type does not have linkage">, + InGroup>; def warn_undefined_inline : Warning<"inline function %q0 is not defined">, InGroup>; def err_undefined_inline_var : Error<"inline variable %q0 is not defined">; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index fc34f68..9420407 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1086,6 +1086,11 @@ public: /// definition in this translation unit. llvm::MapVector UndefinedButUsed; + /// Determine if VD, which must be a variable or function, is an external + /// symbol that nonetheless can't be referenced from outside this translation + /// unit because its type has no linkage and it's not extern "C". + bool isExternalWithNoLinkageType(ValueDecl *VD); + /// Obtain a sorted list of functions that are undefined but ODR-used. void getUndefinedButUsed( SmallVectorImpl > &Undefined); diff --git a/clang/include/clang/Sema/SemaInternal.h b/clang/include/clang/Sema/SemaInternal.h index a01e8d6..4dc215b 100644 --- a/clang/include/clang/Sema/SemaInternal.h +++ b/clang/include/clang/Sema/SemaInternal.h @@ -73,7 +73,8 @@ inline void MarkVarDeclODRUsed(VarDecl *Var, // Keep track of used but undefined variables. // FIXME: We shouldn't suppress this warning for static data members. if (Var->hasDefinition(SemaRef.Context) == VarDecl::DeclarationOnly && - (!Var->isExternallyVisible() || Var->isInline()) && + (!Var->isExternallyVisible() || Var->isInline() || + SemaRef.isExternalWithNoLinkageType(Var)) && !(Var->isStaticDataMember() && Var->hasInit())) { SourceLocation &old = SemaRef.UndefinedButUsed[Var->getCanonicalDecl()]; if (old.isInvalid()) diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index 0a4c4a4..c3e3de1 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -721,8 +721,7 @@ LinkageComputer::getLVForNamespaceScopeDecl(const NamedDecl *D, // because of this, but unique-external linkage suits us. if (Context.getLangOpts().CPlusPlus && !isFirstInExternCContext(Var)) { LinkageInfo TypeLV = getLVForType(*Var->getType(), computation); - if (TypeLV.getLinkage() != ExternalLinkage && - TypeLV.getLinkage() != ModuleLinkage) + if (!isExternallyVisible(TypeLV.getLinkage())) return LinkageInfo::uniqueExternal(); if (!LV.isVisibilityExplicit()) LV.mergeVisibility(TypeLV); @@ -772,7 +771,7 @@ LinkageComputer::getLVForNamespaceScopeDecl(const NamedDecl *D, QualType TypeAsWritten = Function->getType(); if (TypeSourceInfo *TSI = Function->getTypeSourceInfo()) TypeAsWritten = TSI->getType(); - if (TypeAsWritten->getLinkage() == UniqueExternalLinkage) + if (!isExternallyVisible(TypeAsWritten->getLinkage())) return LinkageInfo::uniqueExternal(); } @@ -909,7 +908,7 @@ LinkageComputer::getLVForClassMember(const NamedDecl *D, const NamedDecl *explicitSpecSuppressor = nullptr; if (const auto *MD = dyn_cast(D)) { - // If the type of the function uses a type with unique-external + // If the type of the function uses a type that has non-externally-visible // linkage, it's not legally usable from outside this translation unit. // But only look at the type-as-written. If this function has an // auto-deduced return type, we can't compute the linkage of that type @@ -922,7 +921,7 @@ LinkageComputer::getLVForClassMember(const NamedDecl *D, QualType TypeAsWritten = MD->getType(); if (TypeSourceInfo *TSI = MD->getTypeSourceInfo()) TypeAsWritten = TSI->getType(); - if (TypeAsWritten->getLinkage() == UniqueExternalLinkage) + if (!isExternallyVisible(TypeAsWritten->getLinkage())) return LinkageInfo::uniqueExternal(); } // If this is a method template specialization, use the linkage for @@ -1119,6 +1118,9 @@ LinkageInfo LinkageComputer::getLVForClosure(const DeclContext *DC, Decl *ContextDecl, LVComputationKind computation) { // This lambda has its linkage/visibility determined by its owner. + // FIXME: This is wrong. A lambda never formally has linkage; if this + // calculation determines the lambda has external linkage, it should be + // downgraded to VisibleNoLinkage. if (ContextDecl) { if (isa(ContextDecl)) DC = ContextDecl->getDeclContext()->getRedeclContext(); diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index 66cb497..6269251 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -582,6 +582,23 @@ static bool ShouldRemoveFromUnused(Sema *SemaRef, const DeclaratorDecl *D) { return false; } +static bool isFunctionOrVarDeclExternC(NamedDecl *ND) { + if (auto *FD = dyn_cast(ND)) + return FD->isExternC(); + return cast(ND)->isExternC(); +} + +/// Determine whether ND is an external-linkage function or variable whose +/// type has no linkage. +bool Sema::isExternalWithNoLinkageType(ValueDecl *VD) { + // Note: it's not quite enough to check whether VD has UniqueExternalLinkage, + // because we also want to catch the case where its type has VisibleNoLinkage, + // which does not affect the linkage of VD. + return getLangOpts().CPlusPlus && VD->hasExternalFormalLinkage() && + !isExternalFormalLinkage(VD->getType()->getLinkage()) && + !isFunctionOrVarDeclExternC(VD); +} + /// Obtains a sorted list of functions and variables that are undefined but /// ODR-used. void Sema::getUndefinedButUsed( @@ -598,17 +615,27 @@ void Sema::getUndefinedButUsed( if (isa(ND)) continue; + if (ND->hasAttr() || ND->hasAttr()) { + // An exported function will always be emitted when defined, so even if + // the function is inline, it doesn't have to be emitted in this TU. An + // imported function implies that it has been exported somewhere else. + continue; + } + if (FunctionDecl *FD = dyn_cast(ND)) { if (FD->isDefined()) continue; if (FD->isExternallyVisible() && + !isExternalWithNoLinkageType(FD) && !FD->getMostRecentDecl()->isInlined()) continue; } else { auto *VD = cast(ND); if (VD->hasDefinition() != VarDecl::DeclarationOnly) continue; - if (VD->isExternallyVisible() && !VD->getMostRecentDecl()->isInline()) + if (VD->isExternallyVisible() && + !isExternalWithNoLinkageType(VD) && + !VD->getMostRecentDecl()->isInline()) continue; } @@ -626,33 +653,43 @@ static void checkUndefinedButUsed(Sema &S) { S.getUndefinedButUsed(Undefined); if (Undefined.empty()) return; - for (SmallVectorImpl >::iterator - I = Undefined.begin(), E = Undefined.end(); I != E; ++I) { - NamedDecl *ND = I->first; - - if (ND->hasAttr() || ND->hasAttr()) { - // An exported function will always be emitted when defined, so even if - // the function is inline, it doesn't have to be emitted in this TU. An - // imported function implies that it has been exported somewhere else. - continue; - } - - if (!ND->isExternallyVisible()) { - S.Diag(ND->getLocation(), diag::warn_undefined_internal) - << isa(ND) << ND; - } else if (auto *FD = dyn_cast(ND)) { + for (auto Undef : Undefined) { + ValueDecl *VD = cast(Undef.first); + SourceLocation UseLoc = Undef.second; + + if (S.isExternalWithNoLinkageType(VD)) { + // C++ [basic.link]p8: + // A type without linkage shall not be used as the type of a variable + // or function with external linkage unless + // -- the entity has C language linkage + // -- the entity is not odr-used or is defined in the same TU + // + // As an extension, accept this in cases where the type is externally + // visible, since the function or variable actually can be defined in + // another translation unit in that case. + S.Diag(VD->getLocation(), isExternallyVisible(VD->getType()->getLinkage()) + ? diag::ext_undefined_internal_type + : diag::err_undefined_internal_type) + << isa(VD) << VD; + } else if (!VD->isExternallyVisible()) { + // FIXME: We can promote this to an error. The function or variable can't + // be defined anywhere else, so the program must necessarily violate the + // one definition rule. + S.Diag(VD->getLocation(), diag::warn_undefined_internal) + << isa(VD) << VD; + } else if (auto *FD = dyn_cast(VD)) { (void)FD; assert(FD->getMostRecentDecl()->isInlined() && "used object requires definition but isn't inline or internal?"); // FIXME: This is ill-formed; we should reject. - S.Diag(ND->getLocation(), diag::warn_undefined_inline) << ND; + S.Diag(VD->getLocation(), diag::warn_undefined_inline) << VD; } else { - assert(cast(ND)->getMostRecentDecl()->isInline() && + assert(cast(VD)->getMostRecentDecl()->isInline() && "used var requires definition but isn't inline or internal?"); - S.Diag(ND->getLocation(), diag::err_undefined_inline_var) << ND; + S.Diag(VD->getLocation(), diag::err_undefined_inline_var) << VD; } - if (I->second.isValid()) - S.Diag(I->second, diag::note_used_here); + if (UseLoc.isValid()) + S.Diag(UseLoc, diag::note_used_here); } S.UndefinedButUsed.clear(); diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 2cd9336..389cd7a 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -3827,7 +3827,7 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) { } } - // If this redeclaration makes the function inline, we may need to add it to + // If this redeclaration makes the variable inline, we may need to add it to // UndefinedButUsed. if (!Old->isInline() && New->isInline() && Old->isUsed(false) && !Old->getDefinition() && !New->isThisDeclarationADefinition()) @@ -12329,18 +12329,6 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, } } - // The only way to be included in UndefinedButUsed is if there is an - // ODR use before the definition. Avoid the expensive map lookup if this - // is the first declaration. - if (!FD->isFirstDecl() && FD->getPreviousDecl()->isUsed()) { - if (!FD->isExternallyVisible()) - UndefinedButUsed.erase(FD); - else if (FD->isInlined() && - !LangOpts.GNUInline && - (!FD->getPreviousDecl()->hasAttr())) - UndefinedButUsed.erase(FD); - } - // If the function implicitly returns zero (like 'main') or is naked, // don't complain about missing return statements. if (FD->hasImplicitReturnZero() || FD->hasAttr()) diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index c1b2ad0..46853dd 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -13897,6 +13897,8 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func, !LangOpts.GNUInline && !Func->getMostRecentDecl()->hasAttr()) UndefinedButUsed.insert(std::make_pair(Func->getCanonicalDecl(), Loc)); + else if (isExternalWithNoLinkageType(Func)) + UndefinedButUsed.insert(std::make_pair(Func->getCanonicalDecl(), Loc)); } Func->markUsed(Context); diff --git a/clang/test/Analysis/malloc.mm b/clang/test/Analysis/malloc.mm index f8a43b3..e3daa85 100644 --- a/clang/test/Analysis/malloc.mm +++ b/clang/test/Analysis/malloc.mm @@ -187,7 +187,7 @@ typedef volatile struct { void *opaque1; long opaque2; } OSQueueHead; -void OSAtomicEnqueue( OSQueueHead *__list, void *__new, size_t __offset) __attribute__((weak_import)); +extern "C" void OSAtomicEnqueue( OSQueueHead *__list, void *__new, size_t __offset) __attribute__((weak_import)); static inline void radar11111210(OSQueueHead *pool) { void *newItem = malloc(4); OSAtomicEnqueue(pool, newItem, 4); diff --git a/clang/test/Analysis/reference.cpp b/clang/test/Analysis/reference.cpp index b323b96..c269dd7 100644 --- a/clang/test/Analysis/reference.cpp +++ b/clang/test/Analysis/reference.cpp @@ -117,6 +117,11 @@ void testRetroactiveNullReference(int *x) { y = 5; // expected-warning{{Dereference of null pointer}} } +namespace TestReferenceAddress { +struct S { int &x; }; +S getS(); +S *getSP(); + void testReferenceAddress(int &x) { // FIXME: Move non-zero reference assumption out of RangeConstraintManager.cpp:422 #ifdef ANALYZER_CM_Z3 @@ -127,23 +132,19 @@ void testReferenceAddress(int &x) { clang_analyzer_eval(&ref() != 0); // expected-warning{{TRUE}} #endif - struct S { int &x; }; - - extern S getS(); #ifdef ANALYZER_CM_Z3 clang_analyzer_eval(&getS().x != 0); // expected-warning{{UNKNOWN}} #else clang_analyzer_eval(&getS().x != 0); // expected-warning{{TRUE}} #endif - extern S *getSP(); #ifdef ANALYZER_CM_Z3 clang_analyzer_eval(&getSP()->x != 0); // expected-warning{{UNKNOWN}} #else clang_analyzer_eval(&getSP()->x != 0); // expected-warning{{TRUE}} #endif } - +} void testFunctionPointerReturn(void *opaque) { typedef int &(*RefFn)(); diff --git a/clang/test/CXX/basic/basic.link/p8.cpp b/clang/test/CXX/basic/basic.link/p8.cpp new file mode 100644 index 0000000..842a241 --- /dev/null +++ b/clang/test/CXX/basic/basic.link/p8.cpp @@ -0,0 +1,69 @@ +// RUN: %clang_cc1 -std=c++2a -verify %s -pedantic + +template struct Template {}; + +struct Linkage1 { struct Inner {}; }; +typedef struct { struct Inner {}; } Linkage2; + +typedef struct {} const NoLinkage1; +auto x = [] {}; +typedef decltype(x) NoLinkage2; +auto f() { return [] {}; } +typedef decltype(f()) NoLinkage3; + +inline auto g() { return [] {}; } +typedef decltype(g()) VisibleNoLinkage1; +inline auto y = [] {}; +typedef decltype(x) VisibleNoLinkage2; +inline auto h() { struct {} x; return x; } +typedef decltype(h()) VisibleNoLinkage3; + +extern Linkage1 linkage1v; +extern Linkage1::Inner linkage1iv; +extern Linkage2 linkage2v; +extern Linkage2::Inner linkage2iv; +extern Template linkaget1v; +extern Linkage1 linkage1f(); +void linkage2f(Linkage2); + +void use_linkage() { + &linkage1v, &linkage1iv, &linkage2v, &linkage2iv, &linkaget1v; // expected-warning 5{{unused}} + linkage1f(); + linkage2f({}); +} + +extern NoLinkage1 no_linkage1(); // expected-error {{function 'no_linkage1' is used but not defined in this translation unit}} +extern NoLinkage2 no_linkage2(); // expected-error {{function 'no_linkage2' is used but not defined in this translation unit}} +extern NoLinkage3 no_linkage3(); // expected-error {{function 'no_linkage3' is used but not defined in this translation unit}} + +void use_no_linkage() { + no_linkage1(); // expected-note {{used here}} + no_linkage2(); // expected-note {{used here}} + no_linkage3(); // expected-note {{used here}} +} + +// FIXME: This should emit an extension warning. It does not because we +// incorrectly give the lambda external linkage. +extern VisibleNoLinkage1 visible_no_linkage1(); + +// FIXME: We should accept this as an extension. We don't because we +// incorrectly give the lambda no linkage instead of "VisibleNoLinkage". +extern VisibleNoLinkage2 visible_no_linkage2(); // expected-error {{used but not defined}} + +// This case is correctly accepted as an extension. +extern VisibleNoLinkage3 visible_no_linkage3(); // expected-warning {{ISO C++ requires a definition}} + +void use_visible_no_linkage() { + visible_no_linkage1(); + visible_no_linkage2(); // expected-note {{used here}} + visible_no_linkage3(); // expected-note {{used here}} +} + + +extern inline int not_defined; // expected-error {{not defined}} +extern inline int defined_after_use; +void use_inline_vars() { + not_defined = 1; // expected-note {{used here}} + defined_after_use = 2; +} +inline int defined_after_use; diff --git a/clang/test/CodeGenCXX/debug-info-method.cpp b/clang/test/CodeGenCXX/debug-info-method.cpp index e0f9a28..af7103e 100644 --- a/clang/test/CodeGenCXX/debug-info-method.cpp +++ b/clang/test/CodeGenCXX/debug-info-method.cpp @@ -20,6 +20,7 @@ union { class A { protected: void foo(int, A, decltype(u)); + void bar(); }; void A::foo(int, A, decltype(u)) { @@ -29,3 +30,5 @@ A a; int A::*x = 0; int (A::*y)(int) = 0; + +void A::bar() { foo(0, *this, u); } diff --git a/clang/test/CodeGenCXX/mangle-ms-cxx11.cpp b/clang/test/CodeGenCXX/mangle-ms-cxx11.cpp index 819b0d9..b22c046 100644 --- a/clang/test/CodeGenCXX/mangle-ms-cxx11.cpp +++ b/clang/test/CodeGenCXX/mangle-ms-cxx11.cpp @@ -160,6 +160,8 @@ struct { } a; decltype(a) fun(decltype(a) x, decltype(a)) { return x; } // CHECK-DAG: @"\01?fun@PR18022@@YA?AU@1@U21@0@Z" +void use_fun() { fun(a, a); } + } inline int define_lambda() { @@ -280,7 +282,7 @@ void g() { namespace PR18204 { template -int f(T *); +int f(T *) { return 0; } static union { int n = f(this); }; @@ -346,4 +348,5 @@ int call_it = (A::default_args(), 1); enum { enumerator }; void f(decltype(enumerator)) {} -// CHECK-DAG: define void @"\01?f@@YAXW4@@@Z"( +// CHECK-DAG: define internal void @"\01?f@@YAXW4@@@Z"( +void use_f() { f(enumerator); } diff --git a/clang/test/CodeGenCXX/mangle.cpp b/clang/test/CodeGenCXX/mangle.cpp index 91fe6ae..919f8af 100644 --- a/clang/test/CodeGenCXX/mangle.cpp +++ b/clang/test/CodeGenCXX/mangle.cpp @@ -893,7 +893,8 @@ namespace test39 { struct {} a; } *foo; template void func(T) {} - void test(foo x) { + void test() { + foo x; func(x->a); } } diff --git a/clang/test/PCH/cxx11-lambdas.mm b/clang/test/PCH/cxx11-lambdas.mm index 1f568f0..9628d12 100644 --- a/clang/test/PCH/cxx11-lambdas.mm +++ b/clang/test/PCH/cxx11-lambdas.mm @@ -39,7 +39,7 @@ int init_capture(T t) { } struct X { - template X(T); + template X(T) {} }; struct Y { Y(const X &x = [] {}); }; diff --git a/clang/test/SemaCXX/warn-unreachable.cpp b/clang/test/SemaCXX/warn-unreachable.cpp index b08467a..0811814 100644 --- a/clang/test/SemaCXX/warn-unreachable.cpp +++ b/clang/test/SemaCXX/warn-unreachable.cpp @@ -49,22 +49,26 @@ void test3() { (halt()); // expected-warning {{will never be executed}} } -void test4() { +namespace Test4 { struct S { int mem; } s; S &foor(); - halt(), foor()// expected-warning {{will never be executed}} - .mem; + void test4() { + halt(), foor()// expected-warning {{will never be executed}} + .mem; + } } -void test5() { +namespace Test5 { struct S { int mem; } s; S &foonr() __attribute__((noreturn)); - foonr() - .mem; // expected-warning {{will never be executed}} + void test5() { + foonr() + .mem; // expected-warning {{will never be executed}} + } } void test6() { -- 2.7.4