On Jun 20, 2012, at 12:51 AM, Chandler Carruth wrote:
>> On Mon, Jun 18, 2012 at 3:09 PM, Jordan Rose <[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
