[cfe-dev] Mangling & linkage of unnamed but instantiated nested classes
David Blaikie
dblaikie at gmail.com
Fri Nov 9 17:25:45 PST 2012
On Fri, Nov 9, 2012 at 5:25 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Mon, Nov 5, 2012 at 12:29 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> On Sun, Nov 4, 2012 at 9:20 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Wed, Oct 31, 2012 at 2:19 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>>> On Wed, Oct 31, 2012 at 12:37 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>> Hi John, (& cfe-dev)
>>>>>
>>>>> I mentioned this in person last week & wanted to provide you with some
>>>>> more details & ask for your opinion.
>>>>>
>>>>> Backstory:
>>>>>
>>>>> I originally came across this while trying to use
>>>>> -Wunused-member-function & found it was flagging code like this:
>>>>>
>>>>> struct foo {
>>>>> struct {
>>>>> void func() { ... } // warning that this is 'unused'
>>>>> } x;
>>>>> };
>>>>>
>>>>> This surprised me, so I looked around & found that this function has
>>>>> "no linkage". This seemed strange (because I would expect to be able
>>>>> to call the function from multiple TUs that included the header
>>>>> defining 'foo', compare its address for equality, & such things) & it
>>>>> looks like the Right Thing is for func() to have the same linkage as,
>>>>> say, an inline member function of 'foo' would have.
>>>>>
>>>>> The standard (3.5[basic.link]) seems to "miss" this case depending on
>>>>> how you read it:
>>>>>
>>>>> 1) arguably p2, which says that "A name is said to have linkage when
>>>>> it might denote the same ... function ... as a name introduced by a
>>>>> declaration in another scope: - When a name has external linkage, the
>>>>> entity it denotes can be referred to by names from scopes of other
>>>>> translation units or from other scopes of the same translation unit"
>>>>>
>>>>> 2) p5: "... a member function ... has external linkage if the name of
>>>>> the class has external linkage" (with an exception only for unnamed
>>>>> classes (& enumerations) defined in class-scope typedef declarations
>>>>> such that the class or enumeration has the dypedef name for linkage
>>>>> purposes (7.1.3)) & there are no rules that seem to govern the linkage
>>>>> of this unnamed class.
>>>>> p8 "Names not covered by these rules have no linkage"
>>>>
>>>> [basic.link]p8: "A type is said to have linkage if and only if [...]
>>>> it is an unnamed class or enumeration member of a class with linkage".
>>>>
>>>> I'm going to assume it's an oversight in the standard that the members
>>>> of such an unnamed class don't have linkage.
>>>>
>>>>> The next step was also to look at the mangling compared to GCC. After
>>>>> modifying the linkage of functions like this (with this change):
>>>>>
>>>>>
>>>>> --- lib/AST/Decl.cpp
>>>>> +++ lib/AST/Decl.cpp
>>>>> @@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
>>>>> NamedDecl *D, bool OnlyTemplate) {
>>>>> if (!(isa<CXXMethodDecl>(D) ||
>>>>> isa<VarDecl>(D) ||
>>>>> isa<FieldDecl>(D) ||
>>>>> - (isa<TagDecl>(D) &&
>>>>> - (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
>>>>> + isa<TagDecl>(D)))
>>>>> return LinkageInfo::none();
>>>>>
>>>>> LinkageInfo LV;
>>>>>
>>>>> (& Richard reckons we might be able to simplify that check - just to
>>>>> eliminate some template cases that still seem to get filtered out by
>>>>> it)
>>>>
>>>> This looks like a step in the right direction.
>>>>
>>>>> I caused one test to fail:
>>>>> test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
>>>>> better investigate GCC 4.7's mangling) looks like this:
>>>>>
>>>>> struct S {
>>>>> enum { FOO = 42 };
>>>>> enum { BAR = 42 };
>>>>> };
>>>>>
>>>>> struct T {
>>>>> enum { FOO = 42 };
>>>>> enum { BAR = 42 };
>>>>> };
>>>>>
>>>>> template <typename T> struct X {
>>>>> T value;
>>>>> X(T t) : value(t) {}
>>>>> int f() { return value; }
>>>>> };
>>>>>
>>>>> template <typename T> int f(T t) {
>>>>> X<T> x(t);
>>>>> return x.f();
>>>>> }
>>>>>
>>>>> void test() {
>>>>> (void)f(S::FOO);
>>>>> (void)f(S::BAR);
>>>>> (void)f(T::FOO);
>>>>> (void)f(T::BAR);
>>>>> }
>>>>>
>>>>> & with my change we get the right linkage for the instantiations of
>>>>> 'f' (linkonce_odr instead of internal) but the mangling is still
>>>>> inconsistent with GCC at least. Clang mangles these 4 'f's as:
>>>>>
>>>>> _Z1fIN1S3$_0EEiT_
>>>>> _Z1fIN1S3$_1EEiT_
>>>>> _Z1fIN1T3$_2EEiT_
>>>>> _Z1fIN1T3$_3EEiT_
>>>>>
>>>>> GCC 4.7 mangles them as:
>>>>>
>>>>> _Z1fIN1SUt_EEiT_
>>>>> _Z1fIN1SUt0_EEiT_
>>>>> _Z1fIN1TUt_EEiT_
>>>>> _Z1fIN1TUt0_EEiT_
>>>>>
>>>>> I don't think we have any class-specific unnamed nested type counter
>>>>> that would implement that Ut_, Ut0_, ... mangling scheme, though I can
>>>>> imagine where one might be added (I'm not very familiar with IRGen
>>>>> though, so I'll certainly be happy to have any pointers about how that
>>>>> could/should be implemented).
>>>>
>>>> We have a similar scheme already implemented for lambdas; look at
>>>> getLambdaManglingNumber() etc.
>>>
>>> Thanks for the pointer. Following the lines there I've had a first
>>> blush at this (see attached).
>>>
>>> I've not addressed some of the issues Richard brought up but I'd be
>>> happy to take the time to generalize this to handle.
>
> Spent a little time trying to generalize this to handle the function
> local case but it was taking a bit too much of my time so I figured
> I'd at least get this case handled (which addresses my issue of
> -Wunused-function & generally improves the world a little bit at
> least).
>
>> + if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
>> + !isa<CXXRecordDecl>(Tag->getParent()))
>> + return -2;
>> +
>> + std::pair<llvm::DenseMap<const DeclContext *, int>::iterator, bool>
>> P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),
>> -1));
>>
>> getParent() doesn't return a canonical DeclContext.
>
> I'm not entirely sure what bugs this might cause (test cases welcome)
> but I believe I've addressed this in the newly attached patch.
>
>> Also, 80 columns.
>
> Right, sorry, I was mostly looking to see if I was generally on the
> right track. Fixed, though.
>
>> Also, we probably don't want to generate numbers for a decl unless
>> the mangling is actually externally visible.
>
> Done.
>
>>
>> --- include/clang/AST/Decl.h
>> +++ include/clang/AST/Decl.h
>> @@ -2486,6 +2486,8 @@ private:
>> /// otherwise, it is a null (TypedefNameDecl) pointer.
>> llvm::PointerUnion<TypedefNameDecl*, ExtInfo*> TypedefNameDeclOrQualifier;
>>
>> + int UnnamedManglingNumber;
>> +
>> bool hasExtInfo() const { return TypedefNameDeclOrQualifier.is<ExtInfo*>(); }
>> ExtInfo *getExtInfo() { return TypedefNameDeclOrQualifier.get<ExtInfo*>(); }
>> const ExtInfo *getExtInfo() const {
>>
>> We don't really want to add an extra member to DeclContext just to
>> handle an obscure edge case.
>
> Used a separate side table in the ASTContext to keep the numbers. (& I
> changed the numbers to be simpler - counting from zero instead of -1,
> then doing an offset to compute the actual mangle number. Let me know
> if you'd prefer it to work the other way)
(now with the actually up-to-date patch)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mangle.diff
Type: application/octet-stream
Size: 7018 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121109/a2a4cb48/attachment.obj>
More information about the cfe-dev
mailing list