[cfe-commits] r158683 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/inline.c test/SemaCXX/inline.cpp
David Blaikie
dblaikie at gmail.com
Wed Jun 20 08:21:58 PDT 2012
On Wed, Jun 20, 2012 at 12:51 AM, Chandler Carruth <chandlerc at google.com> wrote:
> On Mon, Jun 18, 2012 at 3:09 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>
>> Author: jrose
>> Date: Mon Jun 18 17:09:19 2012
>> New Revision: 158683
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=158683&view=rev
>> Log:
>> Support -Winternal-linkage-in-inline in C++ code.
>>
>> This includes treating anonymous namespaces like internal linkage, and
>> allowing
>> const variables to be used even if internal. The whole thing's been broken
>> out
>> into a separate function to avoid nested ifs.
>
>
> 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:
>
> In file included from ../lib/Transforms/Utils/SSAUpdater.cpp:29:
> ../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]
> for (typename Traits::PHI_iterator I = Traits::PHI_begin(PHI),
> ^
>
> 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:
>
> "warning: calling function 'PHI_begin' from an inline method with external
> linkage, but this method is defined in an anonymous namespace"
(as a very minor note on the wording (of the original & the revised):
I assume we don't refer to "methods" in other C++ diagnostics, but
instead use 'functions' or 'member functions')
>
> This actually starts with the code under the cursor, and takes the user to
> the nature of the problem.
>
> Now we get a pile of template instantiation notes... ok...
>
> ../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:382:11: note: in
> instantiation of member function
> 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::CheckIfPHIMatches' requested here
> if (CheckIfPHIMatches(SomePHI)) {
> ^
> ../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:329:7: note: in
> instantiation of member function
> 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::FindExistingPHI' requested here
> FindExistingPHI(Info->BB, BlockList);
> ^
> ../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:91:5: note: in
> instantiation of member function
> 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::FindAvailableVals' requested here
> FindAvailableVals(&BlockList);
> ^
> ../lib/Transforms/Utils/SSAUpdater.cpp:352:15: note: in instantiation of
> member function 'llvm::SSAUpdaterImpl<llvm::SSAUpdater>::GetValue' requested
> here
> return Impl.GetValue(BB);
> ^
>
> And then this note, which is *crazy* important:
>
> ../lib/Transforms/Utils/SSAUpdater.cpp:270:30: note: 'PHI_begin' declared
> here
> static inline PHI_iterator PHI_begin(PhiT *PHI) { return
> PHI_iterator(PHI); }
> ^
>
>
> 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.
>
> 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?
>
> 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!
>
> 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:
>
> 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.
>
> 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).
>
> 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.
>
>
> Does that seem reasonable? Is this do-able?
>
>>
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaExpr.cpp
>> cfe/trunk/test/Sema/inline.c
>> cfe/trunk/test/SemaCXX/inline.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=158683&r1=158682&r2=158683&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jun 18
>> 17:09:19 2012
>> @@ -2997,17 +2997,19 @@
>> def note_used_here : Note<"used here">;
>>
>> def warn_internal_in_extern_inline : ExtWarn<
>> - "%select{function|variable}0 %1 has internal linkage but is used in an
>> "
>> - "inline %select{function|method}2 with external linkage">,
>> + "%select{function|variable}0 %1 %select{has internal linkage|is in an
>> anonymous"
>> + " namespace}2 but is used in an inline %select{function|method}3 with"
>> + " external linkage">,
>> InGroup<DiagGroup<"internal-linkage-in-inline"> >;
>> def ext_internal_in_extern_inline : Extension<
>> - "%select{function|variable}0 %1 has internal linkage but is used in an
>> "
>> - "inline %select{function|method}2 with external linkage">,
>> + "%select{function|variable}0 %1 %select{has internal linkage|is in an
>> anonymous"
>> + " namespace}2 but is used in an inline %select{function|method}3 with"
>> + " external linkage">,
>> InGroup<DiagGroup<"internal-linkage-in-inline"> >;
>> -def note_internal_decl_declared_here : Note<
>> - "%0 declared here">;
>> def note_convert_inline_to_static : Note<
>> "use 'static' to give inline function %0 internal linkage">;
>> +def note_internal_decl_declared_here : Note<
>> + "%0 declared here">;
>>
>> def warn_redefinition_of_typedef : ExtWarn<
>> "redefinition of typedef %0 is a C11 feature">,
>>
>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=158683&r1=158682&r2=158683&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Mon Jun 18 17:09:19 2012
>> @@ -130,6 +130,103 @@
>> << 1 << Decl->isDeleted();
>> }
>>
>> +/// \brief Determine whether a FunctionDecl was ever declared with an
>> +/// explicit storage class.
>> +static bool hasAnyExplicitStorageClass(const FunctionDecl *D) {
>> + for (FunctionDecl::redecl_iterator I = D->redecls_begin(),
>> + E = D->redecls_end();
>> + I != E; ++I) {
>> + if (I->getStorageClassAsWritten() != SC_None)
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +/// \brief Check whether we're in an extern inline function and referring
>> to a
>> +/// variable or function with internal linkage.
>> +///
>> +/// This also applies to anonymous-namespaced objects, which are
>> effectively
>> +/// internal.
>> +/// This is only a warning because we used to silently accept this code,
>> but
>> +/// most likely it will not do what the user intends.
>> +static void diagnoseUseOfInternalDeclInInlineFunction(Sema &S,
>> + const NamedDecl *D,
>> + SourceLocation Loc)
>> {
>> + // C11 6.7.4p3: An inline definition of a function with external
>> linkage...
>> + // shall not contain a reference to an identifier with internal
>> linkage.
>> + // C++11 [basic.def.odr]p6: ...in each definition of D, corresponding
>> names,
>> + // looked up according to 3.4, shall refer to an entity defined
>> within the
>> + // definition of D, or shall refer to the same entity, after overload
>> + // resolution (13.3) and after matching of partial template
>> specialization
>> + // (14.8.3), except that a name can refer to a const object with
>> internal or
>> + // no linkage if the object has the same literal type in all
>> definitions of
>> + // D, and the object is initialized with a constant expression
>> (5.19), and
>> + // the value (but not the address) of the object is used, and the
>> object has
>> + // the same value in all definitions of D; ...
>> +
>> + // Check if this is an inlined function or method.
>> + FunctionDecl *Current = S.getCurFunctionDecl();
>> + if (!Current)
>> + return;
>> + if (!Current->isInlined())
>> + return;
>> + if (Current->getLinkage() != ExternalLinkage)
>> + return;
>> +
>> + // Check if the decl has internal linkage.
>> + Linkage UsedLinkage = D->getLinkage();
>> + switch (UsedLinkage) {
>> + case NoLinkage:
>> + return;
>> + case InternalLinkage:
>> + case UniqueExternalLinkage:
>> + break;
>> + case ExternalLinkage:
>> + return;
>> + }
>> +
>> + // Check C++'s exception for const variables. This is in the standard
>> + // because in C++ const variables have internal linkage unless
>> + // explicitly declared extern.
>> + // Note that we don't do any of the cross-TU checks, and this code
>> isn't
>> + // even particularly careful about checking if the variable "has the
>> + // same value in all definitions" of the inline function. It just does
>> a
>> + // sanity check to make sure there is an initializer at all.
>> + // FIXME: We should still be checking to see if we're using a constant
>> + // as a glvalue anywhere, but we don't have the necessary information
>> to
>> + // do that here, and as long as this is a warning and not a hard error
>> + // it's not appropriate to change the semantics of the program (i.e.
>> + // by having BuildDeclRefExpr use VK_RValue for constants like these).
>> + const VarDecl *VD = dyn_cast<VarDecl>(D);
>> + if (VD && S.getLangOpts().CPlusPlus)
>> + if (VD->getType().isConstant(S.getASTContext()) &&
>> VD->getAnyInitializer())
>> + return;
>> +
>> + // Don't warn unless -pedantic is on if the inline function is in the
>> main
>> + // source file. These functions will most likely not be inlined into
>> another
>> + // translation unit, so they're effectively internal.
>> + bool IsInMainFile = S.getSourceManager().isFromMainFile(Loc);
>> + S.Diag(Loc, IsInMainFile ? diag::ext_internal_in_extern_inline
>> + : diag::warn_internal_in_extern_inline)
>> + << (bool)VD
>> + << D
>> + << (UsedLinkage == UniqueExternalLinkage)
>> + << isa<CXXMethodDecl>(Current);
>> +
>> + // Suggest "static" on the inline function, if possible.
>> + if (!isa<CXXMethodDecl>(Current) &&
>> + !hasAnyExplicitStorageClass(Current)) {
>> + const FunctionDecl *FirstDecl = Current->getCanonicalDecl();
>> + SourceLocation DeclBegin = FirstDecl->getSourceRange().getBegin();
>> + S.Diag(DeclBegin, diag::note_convert_inline_to_static)
>> + << Current << FixItHint::CreateInsertion(DeclBegin, "static ");
>> + }
>> +
>> + S.Diag(D->getCanonicalDecl()->getLocation(),
>> + diag::note_internal_decl_declared_here)
>> + << D;
>> +}
>> +
>> /// \brief Determine whether the use of this declaration is valid, and
>> /// emit any corresponding diagnostics.
>> ///
>> @@ -183,42 +280,7 @@
>> if (D->hasAttr<UnusedAttr>())
>> Diag(Loc, diag::warn_used_but_marked_unused) << D->getDeclName();
>>
>> - // Warn if we're in an extern inline function referring to a decl
>> - // with internal linkage. (C99 6.7.4p3)
>> - // FIXME: This is not explicitly forbidden in C++, but it's not clear
>> - // what the correct behavior is. We should probably still have a
>> warning.
>> - // (However, in C++ const variables have internal linkage by default,
>> while
>> - // functions still have external linkage by default, so this warning
>> becomes
>> - // very noisy.)
>> - if (!getLangOpts().CPlusPlus) {
>> - if (FunctionDecl *Current = getCurFunctionDecl()) {
>> - if (Current->isInlined() && Current->getLinkage() >
>> InternalLinkage) {
>> - if (D->getLinkage() == InternalLinkage) {
>> - // We won't warn by default if the inline function is in the
>> main
>> - // source file; in these cases it is almost certain that the
>> inlining
>> - // will only occur in this file, even if there is an external
>> - // declaration as well.
>> - bool IsFromMainFile = getSourceManager().isFromMainFile(Loc);
>> - Diag(Loc, IsFromMainFile ? diag::ext_internal_in_extern_inline
>> - :
>> diag::warn_internal_in_extern_inline)
>> - << !isa<FunctionDecl>(D) << D << isa<CXXMethodDecl>(Current);
>> -
>> - // If the user didn't explicitly specify a storage class,
>> - // suggest adding "static" to fix the problem.
>> - const FunctionDecl *FirstDecl = Current->getCanonicalDecl();
>> - if (FirstDecl->getStorageClassAsWritten() == SC_None) {
>> - SourceLocation DeclBegin =
>> FirstDecl->getSourceRange().getBegin();
>> - Diag(DeclBegin, diag::note_convert_inline_to_static)
>> - << Current << FixItHint::CreateInsertion(DeclBegin, "static
>> ");
>> - }
>> -
>> - Diag(D->getCanonicalDecl()->getLocation(),
>> - diag::note_internal_decl_declared_here)
>> - << D;
>> - }
>> - }
>> - }
>> - }
>> + diagnoseUseOfInternalDeclInInlineFunction(*this, D, Loc);
>>
>> return false;
>> }
>>
>> Modified: cfe/trunk/test/Sema/inline.c
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/inline.c?rev=158683&r1=158682&r2=158683&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/Sema/inline.c (original)
>> +++ cfe/trunk/test/Sema/inline.c Mon Jun 18 17:09:19 2012
>> @@ -1,4 +1,5 @@
>> // RUN: %clang_cc1 -fsyntax-only -verify %s
>> +// RUN: %clang_cc1 -fsyntax-only -verify -x c++ %s
>>
>> #if defined(INCLUDE)
>> // -------
>> @@ -40,7 +41,7 @@
>> // Check that the warnings from the "header file" aren't on by default in
>> // the main source file.
>>
>> -inline int useStaticMain () {
>> +inline int useStaticMainFile () {
>> staticFunction(); // no-warning
>> return staticVar; // no-warning
>> }
>>
>> Modified: cfe/trunk/test/SemaCXX/inline.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/inline.cpp?rev=158683&r1=158682&r2=158683&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/inline.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/inline.cpp Mon Jun 18 17:09:19 2012
>> @@ -1,5 +1,110 @@
>> // RUN: %clang_cc1 -fsyntax-only -verify %s
>>
>> +#if defined(INCLUDE)
>> +// -------
>> +// This section acts like a header file.
>> +// -------
>> +
>> +// Check the use of static variables in non-static inline functions.
>> +static int staticVar; // expected-note + {{'staticVar' declared here}}
>> +static int staticFunction(); // expected-note + {{'staticFunction'
>> declared here}}
>> +const int constVar = 0; // no-warning
>> +
>> +namespace {
>> + int anonVar; // expected-note + {{'anonVar' declared here}}
>> + int anonFunction(); // expected-note + {{'anonFunction' declared here}}
>> + const int anonConstVar = 0; // no-warning
>> +
>> + class Anon {
>> + public:
>> + static int var; // expected-note + {{'var' declared here}}
>> + static const int constVar = 0; // no-warning
>> + };
>> +}
>> +
>> +inline void useStatic() { // expected-note + {{use 'static' to give
>> inline function 'useStatic' internal linkage}}
>> + staticFunction(); // expected-warning{{function 'staticFunction' has
>> internal linkage but is used in an inline function with external linkage}}
>> + (void)staticVar; // expected-warning{{variable 'staticVar' has internal
>> linkage but is used in an inline function with external linkage}}
>> + anonFunction(); // expected-warning{{function 'anonFunction' is in an
>> anonymous namespace but is used in an inline function with external
>> linkage}}
>> + (void)anonVar; // expected-warning{{variable 'anonVar' is in an
>> anonymous namespace but is used in an inline function with external
>> linkage}}
>> + (void)Anon::var; // expected-warning{{variable 'var' is in an anonymous
>> namespace but is used in an inline function with external linkage}}
>> +
>> + (void)constVar; // no-warning
>> + (void)anonConstVar; // no-warning
>> + (void)Anon::constVar; // no-warning
>> +}
>> +
>> +extern inline int useStaticFromExtern() { // no suggestions
>> + staticFunction(); // expected-warning{{function 'staticFunction' has
>> internal linkage but is used in an inline function with external linkage}}
>> + return staticVar; // expected-warning{{variable 'staticVar' has
>> internal linkage but is used in an inline function with external linkage}}
>> +}
>> +
>> +class A {
>> +public:
>> + static inline int useInClass() { // no suggestions
>> + return staticFunction(); // expected-warning{{function
>> 'staticFunction' has internal linkage but is used in an inline method with
>> external linkage}}
>> + }
>> + inline int useInInstance() { // no suggestions
>> + return staticFunction(); // expected-warning{{function
>> 'staticFunction' has internal linkage but is used in an inline method with
>> external linkage}}
>> + }
>> +};
>> +
>> +static inline void useStaticFromStatic () {
>> + // No warnings.
>> + staticFunction();
>> + (void)staticVar;
>> + (void)constVar;
>> + anonFunction();
>> + (void)anonVar;
>> + (void)anonConstVar;
>> + (void)Anon::var;
>> + (void)Anon::constVar;
>> +}
>> +
>> +namespace {
>> + inline void useStaticFromAnon() {
>> + // No warnings.
>> + staticFunction();
>> + (void)staticVar;
>> + (void)constVar;
>> + anonFunction();
>> + (void)anonVar;
>> + (void)anonConstVar;
>> + (void)Anon::var;
>> + (void)Anon::constVar;
>> + }
>> +}
>> +
>> +#else
>> +// -------
>> +// This is the main source file.
>> +// -------
>> +
>> +#define INCLUDE
>> +#include "inline.cpp"
>> +
>> // Check that we don't allow illegal uses of inline
>> // (checking C++-only constructs here)
>> struct c {inline int a;}; // expected-error{{'inline' can only appear on
>> functions}}
>> +
>> +// Check that the warnings from the "header file" aren't on by default in
>> +// the main source file.
>> +
>> +inline int useStaticMainFile () {
>> + anonFunction(); // no-warning
>> + return staticVar; // no-warning
>> +}
>> +
>> +// Check that the warnings show up when explicitly requested.
>> +
>> +#pragma clang diagnostic push
>> +#pragma clang diagnostic warning "-Winternal-linkage-in-inline"
>> +
>> +inline int useStaticAgain () { // expected-note 2 {{use 'static' to give
>> inline function 'useStaticAgain' internal linkage}}
>> + anonFunction(); // expected-warning{{function 'anonFunction' is in an
>> anonymous namespace but is used in an inline function with external
>> linkage}}
>> + return staticVar; // expected-warning{{variable 'staticVar' has
>> internal linkage but is used in an inline function with external linkage}}
>> +}
>> +
>> +#pragma clang diagnostic pop
>> +
>> +#endif
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list