[cfe-commits] r158683 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/inline.c test/SemaCXX/inline.cpp

[Jordan Rose]

On Jun 20, 2012, at 12:51 AM, Chandler Carruth 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"
> 
> This actually starts with the code under the cursor, and takes the user to the nature of the problem.

I see the ordering problem; there's a bit of a dangling reference here (does "this method" refer to PHI_begin or the current method?). I'll try to come up with something else.


> 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);
>               ^

Yuck.

> 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!

Well. /That/ was a case I completely missed. Thank you for that!

Also, is there a more concise name for "declared in an anonymous namespace"? Because the function type here has UniqueExternalLinkage, but that's not something we can say to a user.


> 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.

Yes.


> 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).

*sigh* This seems like it's going to replicate the work in getLVForDecl...


> 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.

…and this could be recursive. I'm worried about another pile of notes here.

Any suggestions on how to make this better? I'd be okay with adding a flag for getLVForDecl that asks us to find the type/object that brings a linkage down from what it would otherwise be, but the recursiveness is fairly nasty. ("But why does /that/ class have internal linkage?")

Thanks for debugging the warning,
Jordan