[cfe-dev] Mangling & linkage of unnamed but instantiated nested classes

[Eli Friedman]

Comments below.

On Fri, Nov 9, 2012 at 5:25 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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.

It might not matter here, because all decls contained in a RecordDecl
generally have the same parent... this has a bigger impact for
NamespaceDecl, for example, because there are two different Decls
which are semantically the same context.

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

@@ -2582,8 +2581,9 @@ void
TagDecl::setTypedefNameForAnonDecl(TypedefNameDecl *TDD) {
 void TagDecl::startDefinition() {
   IsBeingDefined = true;

-  if (isa<CXXRecordDecl>(this)) {
-    CXXRecordDecl *D = cast<CXXRecordDecl>(this);
+  getASTContext().addUnnamedTag(this);

Will getTypedefNameForAnonDecl actually return the correct result
here? (I'd like to see a test to make sure that "typedef struct {} x"
doesn't cause us to increment the mangling number, assuming that's
consistent with gcc.)

-Eli