[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