<div style="font-family: arial, helvetica, sans-serif"><font size="2"><div class="gmail_quote">On Mon, Jun 18, 2012 at 3:09 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: jrose<br>
Date: Mon Jun 18 17:09:19 2012<br>
New Revision: 158683<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=158683&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=158683&view=rev</a><br>
Log:<br>
Support -Winternal-linkage-in-inline in C++ code.<br>
<br>
This includes treating anonymous namespaces like internal linkage, and allowing<br>
const variables to be used even if internal. The whole thing's been broken out<br>
into a separate function to avoid nested ifs.<br></blockquote><div><br></div><div>This warning seems interesting, and I generally like the correctness insistence, but the current output is really unhelpful. Let's look at one of the *many* cases I'm trying to fix in LLVM's own code:</div>
<div><br></div><div><div>In file included from ../lib/Transforms/Utils/SSAUpdater.cpp:29:</div><div>../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:406:54: warning: function 'PHI_begin' is in an anonymous namespace but is used in an inline method with external linkage [-Winternal-linkage-in-inline]</div>
<div> for (typename Traits::PHI_iterator I = Traits::PHI_begin(PHI),</div><div> ^</div><div><br></div><div>So, for starters this message doesn't read naturally with the code snippet its pointing at because the message is a bit inverted. Imagine it instead read as:</div>
<div><br></div><div>"warning: calling function 'PHI_begin' from an inline method with external linkage, but this method is defined in an anonymous namespace"</div><div><br></div><div>This actually starts with the code under the cursor, and takes the user to the nature of the problem.</div>
<div><br></div><div>Now we get a pile of template instantiation notes... ok...</div><div><br></div><div>../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:382:11: note: in instantiation of member function 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::CheckIfPHIMatches' requested here</div>
<div> if (CheckIfPHIMatches(SomePHI)) {</div><div> ^</div><div>../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:329:7: note: in instantiation of member function 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::FindExistingPHI' requested here</div>
<div> FindExistingPHI(Info->BB, BlockList);</div><div> ^</div><div>../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:91:5: note: in instantiation of member function 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::FindAvailableVals' requested here</div>
<div> FindAvailableVals(&BlockList);</div><div> ^</div><div>../lib/Transforms/Utils/SSAUpdater.cpp:352:15: note: in instantiation of member function 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::GetValue' requested here</div>
<div> return Impl.GetValue(BB);</div><div> ^</div><div><br></div><div>And then this note, which is *crazy* important:</div><div><br></div><div>../lib/Transforms/Utils/SSAUpdater.cpp:270:30: note: 'PHI_begin' declared here</div>
<div> static inline PHI_iterator PHI_begin(PhiT *PHI) { return PHI_iterator(PHI); }</div><div> ^</div></div><div><br></div><div><br></div><div>Now, at this point, I know where the bad use is, why it is bad, and where the used entity is declared. But this doesn't really tell me how to fix anything.</div>
<div><br></div><div>Even worse, if you dig into it, you'll see that PHI_begin is *not* in fact declared in an anonymous namespace at all. It is a static member of a class template specialization declared in the 'llvm' namespace! So why did clang lie to us and complain?</div>
<div><br></div><div>Well, the return type is a typedef, and that typedef is of a type defined in an anonymous namespace. Because of that, the return type has internal linkage, which means the function has internal linkage, which means we can't call it from an inline function with external linkage. OW!</div>
<div><br></div><div>I don't think most folks are going to figure that out easily. I think we'll need to do several things to help users get benefit from the warning:</div><div><br></div><div>1) We need to fix the wording to be more precise about declarations within an anonymous namespace, versus file-static declarations, versus declarations with internal linkage (for some other reason). There is already some of that here, but not enough I think.</div>
<div><br></div><div>2) We need to actually dig out *why* the entity has internal linkage if at all possible, and tell the user that ("due to its return type" or something).</div><div><br></div><div>3) Ideally, when we dig out the reason as having to do with some aspect of the function's type, we should emit a second note at the location of the internal linkage type that cascaded to the function.</div>
<div><br></div><div><br></div><div>Does that seem reasonable? Is this do-able?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Sema/SemaExpr.cpp<br>
cfe/trunk/test/Sema/inline.c<br>
cfe/trunk/test/SemaCXX/inline.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=158683&r1=158682&r2=158683&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=158683&r1=158682&r2=158683&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jun 18 17:09:19 2012<br>
@@ -2997,17 +2997,19 @@<br>
def note_used_here : Note<"used here">;<br>
<br>
def warn_internal_in_extern_inline : ExtWarn<<br>
- "%select{function|variable}0 %1 has internal linkage but is used in an "<br>
- "inline %select{function|method}2 with external linkage">,<br>
+ "%select{function|variable}0 %1 %select{has internal linkage|is in an anonymous"<br>
+ " namespace}2 but is used in an inline %select{function|method}3 with"<br>
+ " external linkage">,<br>
InGroup<DiagGroup<"internal-linkage-in-inline"> >;<br>
def ext_internal_in_extern_inline : Extension<<br>
- "%select{function|variable}0 %1 has internal linkage but is used in an "<br>
- "inline %select{function|method}2 with external linkage">,<br>
+ "%select{function|variable}0 %1 %select{has internal linkage|is in an anonymous"<br>
+ " namespace}2 but is used in an inline %select{function|method}3 with"<br>
+ " external linkage">,<br>
InGroup<DiagGroup<"internal-linkage-in-inline"> >;<br>
-def note_internal_decl_declared_here : Note<<br>
- "%0 declared here">;<br>
def note_convert_inline_to_static : Note<<br>
"use 'static' to give inline function %0 internal linkage">;<br>
+def note_internal_decl_declared_here : Note<<br>
+ "%0 declared here">;<br>
<br>
def warn_redefinition_of_typedef : ExtWarn<<br>
"redefinition of typedef %0 is a C11 feature">,<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=158683&r1=158682&r2=158683&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=158683&r1=158682&r2=158683&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Mon Jun 18 17:09:19 2012<br>
@@ -130,6 +130,103 @@<br>
<< 1 << Decl->isDeleted();<br>
}<br>
<br>
+/// \brief Determine whether a FunctionDecl was ever declared with an<br>
+/// explicit storage class.<br>
+static bool hasAnyExplicitStorageClass(const FunctionDecl *D) {<br>
+ for (FunctionDecl::redecl_iterator I = D->redecls_begin(),<br>
+ E = D->redecls_end();<br>
+ I != E; ++I) {<br>
+ if (I->getStorageClassAsWritten() != SC_None)<br>
+ return true;<br>
+ }<br>
+ return false;<br>
+}<br>
+<br>
+/// \brief Check whether we're in an extern inline function and referring to a<br>
+/// variable or function with internal linkage.<br>
+///<br>
+/// This also applies to anonymous-namespaced objects, which are effectively<br>
+/// internal.<br>
+/// This is only a warning because we used to silently accept this code, but<br>
+/// most likely it will not do what the user intends.<br>
+static void diagnoseUseOfInternalDeclInInlineFunction(Sema &S,<br>
+ const NamedDecl *D,<br>
+ SourceLocation Loc) {<br>
+ // C11 6.7.4p3: An inline definition of a function with external linkage...<br>
+ // shall not contain a reference to an identifier with internal linkage.<br>
+ // C++11 [basic.def.odr]p6: ...in each definition of D, corresponding names,<br>
+ // looked up according to 3.4, shall refer to an entity defined within the<br>
+ // definition of D, or shall refer to the same entity, after overload<br>
+ // resolution (13.3) and after matching of partial template specialization<br>
+ // (14.8.3), except that a name can refer to a const object with internal or<br>
+ // no linkage if the object has the same literal type in all definitions of<br>
+ // D, and the object is initialized with a constant expression (5.19), and<br>
+ // the value (but not the address) of the object is used, and the object has<br>
+ // the same value in all definitions of D; ...<br>
+<br>
+ // Check if this is an inlined function or method.<br>
+ FunctionDecl *Current = S.getCurFunctionDecl();<br>
+ if (!Current)<br>
+ return;<br>
+ if (!Current->isInlined())<br>
+ return;<br>
+ if (Current->getLinkage() != ExternalLinkage)<br>
+ return;<br>
+<br>
+ // Check if the decl has internal linkage.<br>
+ Linkage UsedLinkage = D->getLinkage();<br>
+ switch (UsedLinkage) {<br>
+ case NoLinkage:<br>
+ return;<br>
+ case InternalLinkage:<br>
+ case UniqueExternalLinkage:<br>
+ break;<br>
+ case ExternalLinkage:<br>
+ return;<br>
+ }<br>
+<br>
+ // Check C++'s exception for const variables. This is in the standard<br>
+ // because in C++ const variables have internal linkage unless<br>
+ // explicitly declared extern.<br>
+ // Note that we don't do any of the cross-TU checks, and this code isn't<br>
+ // even particularly careful about checking if the variable "has the<br>
+ // same value in all definitions" of the inline function. It just does a<br>
+ // sanity check to make sure there is an initializer at all.<br>
+ // FIXME: We should still be checking to see if we're using a constant<br>
+ // as a glvalue anywhere, but we don't have the necessary information to<br>
+ // do that here, and as long as this is a warning and not a hard error<br>
+ // it's not appropriate to change the semantics of the program (i.e.<br>
+ // by having BuildDeclRefExpr use VK_RValue for constants like these).<br>
+ const VarDecl *VD = dyn_cast<VarDecl>(D);<br>
+ if (VD && S.getLangOpts().CPlusPlus)<br>
+ if (VD->getType().isConstant(S.getASTContext()) && VD->getAnyInitializer())<br>
+ return;<br>
+<br>
+ // Don't warn unless -pedantic is on if the inline function is in the main<br>
+ // source file. These functions will most likely not be inlined into another<br>
+ // translation unit, so they're effectively internal.<br>
+ bool IsInMainFile = S.getSourceManager().isFromMainFile(Loc);<br>
+ S.Diag(Loc, IsInMainFile ? diag::ext_internal_in_extern_inline<br>
+ : diag::warn_internal_in_extern_inline)<br>
+ << (bool)VD<br>
+ << D<br>
+ << (UsedLinkage == UniqueExternalLinkage)<br>
+ << isa<CXXMethodDecl>(Current);<br>
+<br>
+ // Suggest "static" on the inline function, if possible.<br>
+ if (!isa<CXXMethodDecl>(Current) &&<br>
+ !hasAnyExplicitStorageClass(Current)) {<br>
+ const FunctionDecl *FirstDecl = Current->getCanonicalDecl();<br>
+ SourceLocation DeclBegin = FirstDecl->getSourceRange().getBegin();<br>
+ S.Diag(DeclBegin, diag::note_convert_inline_to_static)<br>
+ << Current << FixItHint::CreateInsertion(DeclBegin, "static ");<br>
+ }<br>
+<br>
+ S.Diag(D->getCanonicalDecl()->getLocation(),<br>
+ diag::note_internal_decl_declared_here)<br>
+ << D;<br>
+}<br>
+<br>
/// \brief Determine whether the use of this declaration is valid, and<br>
/// emit any corresponding diagnostics.<br>
///<br>
@@ -183,42 +280,7 @@<br>
if (D->hasAttr<UnusedAttr>())<br>
Diag(Loc, diag::warn_used_but_marked_unused) << D->getDeclName();<br>
<br>
- // Warn if we're in an extern inline function referring to a decl<br>
- // with internal linkage. (C99 6.7.4p3)<br>
- // FIXME: This is not explicitly forbidden in C++, but it's not clear<br>
- // what the correct behavior is. We should probably still have a warning.<br>
- // (However, in C++ const variables have internal linkage by default, while<br>
- // functions still have external linkage by default, so this warning becomes<br>
- // very noisy.)<br>
- if (!getLangOpts().CPlusPlus) {<br>
- if (FunctionDecl *Current = getCurFunctionDecl()) {<br>
- if (Current->isInlined() && Current->getLinkage() > InternalLinkage) {<br>
- if (D->getLinkage() == InternalLinkage) {<br>
- // We won't warn by default if the inline function is in the main<br>
- // source file; in these cases it is almost certain that the inlining<br>
- // will only occur in this file, even if there is an external<br>
- // declaration as well.<br>
- bool IsFromMainFile = getSourceManager().isFromMainFile(Loc);<br>
- Diag(Loc, IsFromMainFile ? diag::ext_internal_in_extern_inline<br>
- : diag::warn_internal_in_extern_inline)<br>
- << !isa<FunctionDecl>(D) << D << isa<CXXMethodDecl>(Current);<br>
-<br>
- // If the user didn't explicitly specify a storage class,<br>
- // suggest adding "static" to fix the problem.<br>
- const FunctionDecl *FirstDecl = Current->getCanonicalDecl();<br>
- if (FirstDecl->getStorageClassAsWritten() == SC_None) {<br>
- SourceLocation DeclBegin = FirstDecl->getSourceRange().getBegin();<br>
- Diag(DeclBegin, diag::note_convert_inline_to_static)<br>
- << Current << FixItHint::CreateInsertion(DeclBegin, "static ");<br>
- }<br>
-<br>
- Diag(D->getCanonicalDecl()->getLocation(),<br>
- diag::note_internal_decl_declared_here)<br>
- << D;<br>
- }<br>
- }<br>
- }<br>
- }<br>
+ diagnoseUseOfInternalDeclInInlineFunction(*this, D, Loc);<br>
<br>
return false;<br>
}<br>
<br>
Modified: cfe/trunk/test/Sema/inline.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/inline.c?rev=158683&r1=158682&r2=158683&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/inline.c?rev=158683&r1=158682&r2=158683&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Sema/inline.c (original)<br>
+++ cfe/trunk/test/Sema/inline.c Mon Jun 18 17:09:19 2012<br>
@@ -1,4 +1,5 @@<br>
// RUN: %clang_cc1 -fsyntax-only -verify %s<br>
+// RUN: %clang_cc1 -fsyntax-only -verify -x c++ %s<br>
<br>
#if defined(INCLUDE)<br>
// -------<br>
@@ -40,7 +41,7 @@<br>
// Check that the warnings from the "header file" aren't on by default in<br>
// the main source file.<br>
<br>
-inline int useStaticMain () {<br>
+inline int useStaticMainFile () {<br>
staticFunction(); // no-warning<br>
return staticVar; // no-warning<br>
}<br>
<br>
Modified: cfe/trunk/test/SemaCXX/inline.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/inline.cpp?rev=158683&r1=158682&r2=158683&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/inline.cpp?rev=158683&r1=158682&r2=158683&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/inline.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/inline.cpp Mon Jun 18 17:09:19 2012<br>
@@ -1,5 +1,110 @@<br>
// RUN: %clang_cc1 -fsyntax-only -verify %s<br>
<br>
+#if defined(INCLUDE)<br>
+// -------<br>
+// This section acts like a header file.<br>
+// -------<br>
+<br>
+// Check the use of static variables in non-static inline functions.<br>
+static int staticVar; // expected-note + {{'staticVar' declared here}}<br>
+static int staticFunction(); // expected-note + {{'staticFunction' declared here}}<br>
+const int constVar = 0; // no-warning<br>
+<br>
+namespace {<br>
+ int anonVar; // expected-note + {{'anonVar' declared here}}<br>
+ int anonFunction(); // expected-note + {{'anonFunction' declared here}}<br>
+ const int anonConstVar = 0; // no-warning<br>
+<br>
+ class Anon {<br>
+ public:<br>
+ static int var; // expected-note + {{'var' declared here}}<br>
+ static const int constVar = 0; // no-warning<br>
+ };<br>
+}<br>
+<br>
+inline void useStatic() { // expected-note + {{use 'static' to give inline function 'useStatic' internal linkage}}<br>
+ staticFunction(); // expected-warning{{function 'staticFunction' has internal linkage but is used in an inline function with external linkage}}<br>
+ (void)staticVar; // expected-warning{{variable 'staticVar' has internal linkage but is used in an inline function with external linkage}}<br>
+ anonFunction(); // expected-warning{{function 'anonFunction' is in an anonymous namespace but is used in an inline function with external linkage}}<br>
+ (void)anonVar; // expected-warning{{variable 'anonVar' is in an anonymous namespace but is used in an inline function with external linkage}}<br>
+ (void)Anon::var; // expected-warning{{variable 'var' is in an anonymous namespace but is used in an inline function with external linkage}}<br>
+<br>
+ (void)constVar; // no-warning<br>
+ (void)anonConstVar; // no-warning<br>
+ (void)Anon::constVar; // no-warning<br>
+}<br>
+<br>
+extern inline int useStaticFromExtern() { // no suggestions<br>
+ staticFunction(); // expected-warning{{function 'staticFunction' has internal linkage but is used in an inline function with external linkage}}<br>
+ return staticVar; // expected-warning{{variable 'staticVar' has internal linkage but is used in an inline function with external linkage}}<br>
+}<br>
+<br>
+class A {<br>
+public:<br>
+ static inline int useInClass() { // no suggestions<br>
+ return staticFunction(); // expected-warning{{function 'staticFunction' has internal linkage but is used in an inline method with external linkage}}<br>
+ }<br>
+ inline int useInInstance() { // no suggestions<br>
+ return staticFunction(); // expected-warning{{function 'staticFunction' has internal linkage but is used in an inline method with external linkage}}<br>
+ }<br>
+};<br>
+<br>
+static inline void useStaticFromStatic () {<br>
+ // No warnings.<br>
+ staticFunction();<br>
+ (void)staticVar;<br>
+ (void)constVar;<br>
+ anonFunction();<br>
+ (void)anonVar;<br>
+ (void)anonConstVar;<br>
+ (void)Anon::var;<br>
+ (void)Anon::constVar;<br>
+}<br>
+<br>
+namespace {<br>
+ inline void useStaticFromAnon() {<br>
+ // No warnings.<br>
+ staticFunction();<br>
+ (void)staticVar;<br>
+ (void)constVar;<br>
+ anonFunction();<br>
+ (void)anonVar;<br>
+ (void)anonConstVar;<br>
+ (void)Anon::var;<br>
+ (void)Anon::constVar;<br>
+ }<br>
+}<br>
+<br>
+#else<br>
+// -------<br>
+// This is the main source file.<br>
+// -------<br>
+<br>
+#define INCLUDE<br>
+#include "inline.cpp"<br>
+<br>
// Check that we don't allow illegal uses of inline<br>
// (checking C++-only constructs here)<br>
struct c {inline int a;}; // expected-error{{'inline' can only appear on functions}}<br>
+<br>
+// Check that the warnings from the "header file" aren't on by default in<br>
+// the main source file.<br>
+<br>
+inline int useStaticMainFile () {<br>
+ anonFunction(); // no-warning<br>
+ return staticVar; // no-warning<br>
+}<br>
+<br>
+// Check that the warnings show up when explicitly requested.<br>
+<br>
+#pragma clang diagnostic push<br>
+#pragma clang diagnostic warning "-Winternal-linkage-in-inline"<br>
+<br>
+inline int useStaticAgain () { // expected-note 2 {{use 'static' to give inline function 'useStaticAgain' internal linkage}}<br>
+ anonFunction(); // expected-warning{{function 'anonFunction' is in an anonymous namespace but is used in an inline function with external linkage}}<br>
+ return staticVar; // expected-warning{{variable 'staticVar' has internal linkage but is used in an inline function with external linkage}}<br>
+}<br>
+<br>
+#pragma clang diagnostic pop<br>
+<br>
+#endif<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></font></div>