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

[David Blaikie]

On Fri, Nov 9, 2012 at 5:38 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> 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.

Thanks for the explanation.

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

Seems not - thanks for the catch.

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

Test case added & then I had to restructure the code to address this.
Since we don't know which way a type will be mangled until after we've
parsed all the declarators in the group (eg: typedef struct{} *x, *y,
z; - only when we see 'z' do we know that we have a linkage name for
the struct, if we only have x and y, then we don't have such a name &
we need to use the UtX_ naming) I had to inject the logic into
FinalizeDeclaratorGroup (or somewhere near there - perhaps there's a
better spot). This means we miss the original test case failure of:
struct foo { enum { X }; }; since there is no declarator group in tht
instance, just a standalone decl. So I added this to the
ParsedFreeStandingDeclSpec as well.

If there's some more appropriate common point to put this logic, I'm
all ears - but it seems to me there's no common point in Sema for
these two cases so it's either duplicated or could be non-duplicated
if it were up in the Parser (or had a new Sema entry point to call
from there).

Open to ideas - thanks again for the help/review/pointers.

- David