clang::Modules and duplicate definitions of base CXXRecordDecls

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

clang::Modules and duplicate definitions of base CXXRecordDecls

Axel Naumann
Hi,

clang::Modules attach definitions of CXXRecordDecls to the redecl chain,
updating the Definition* of the CXXRecordDecl::DefinitionData. If I read
a derived class's decl, its CXXMethodDecls will read the overridden
methods - those of the base CXXRecordDecl that existed at the time of
reading the derived class. If the ASTReader then reads a new instance of
the base class, the check:

AST/VTableBuilder.cpp:1419 FindNearestOverriddenMethod():
      // We found our overridden method.
      if (OverriddenMD->getParent() == PrimaryBase)
        return OverriddenMD;

fails, because the OverriddenMD is from the old base, but PrimaryBase is
the one read later. Thus the vtable index gets miscalculated. Changing
this to
      // We found our overridden method.
      if (OverriddenMD->getParent()->getCanonicalDecl()
          == PrimaryBase->getCanonicalDecl())
        return OverriddenMD;
works around it.

I'd of course like to fix this in ASTReader, but I'd have to fix the
*derived* classes' overridden methods when reading the *base*. I have a
similar issue with the derived class's
ASTRecordLayout::getBaseClassOffset() not finding the base because the
map uses the old base decl as the key.

Alternatively we could refuse to add dupe definitions to the AST in the
ASTReader, but that's a revolution that I'm afraid of :-)

What are your recommendations? How could we fix that?

Cheers, Axel.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang::Modules and duplicate definitions of base CXXRecordDecls

Douglas Gregor

On Oct 2, 2012, at 8:38 AM, Axel Naumann <[hidden email]> wrote:

> Hi,
>
> clang::Modules attach definitions of CXXRecordDecls to the redecl chain,
> updating the Definition* of the CXXRecordDecl::DefinitionData. If I read
> a derived class's decl, its CXXMethodDecls will read the overridden
> methods - those of the base CXXRecordDecl that existed at the time of
> reading the derived class. If the ASTReader then reads a new instance of
> the base class, the check:
>
> AST/VTableBuilder.cpp:1419 FindNearestOverriddenMethod():
>      // We found our overridden method.
>      if (OverriddenMD->getParent() == PrimaryBase)
>        return OverriddenMD;
>
> fails, because the OverriddenMD is from the old base, but PrimaryBase is
> the one read later. Thus the vtable index gets miscalculated. Changing
> this to
>      // We found our overridden method.
>      if (OverriddenMD->getParent()->getCanonicalDecl()
>          == PrimaryBase->getCanonicalDecl())
>        return OverriddenMD;
> works around it.
>
> I'd of course like to fix this in ASTReader, but I'd have to fix the
> *derived* classes' overridden methods when reading the *base*. I have a
> similar issue with the derived class's
> ASTRecordLayout::getBaseClassOffset() not finding the base because the
> map uses the old base decl as the key.

This is just the canary for a whole host of issues that will crop up due to duplicated definitions. For example, we have to deal with instantiation of enumerators, which (as a declaration kind) don't support redeclaration the way functions/variables/classes do:

        template<typename T>
        struct X {
                enum { value = sizeof(T) };
        };

        // reference X<int>::value in two different modules, and end up with two distinct declarations.

> Alternatively we could refuse to add dupe definitions to the AST in the
> ASTReader, but that's a revolution that I'm afraid of :-)


I've been fretting over this particular issue for quite a while, and I think that teaching the ASTReader to merge the declarations as it reads them is probably the only way to really solve this problem. To do so, we'd need to be able to detect when the same implicit instantiation occurs in two modules (which isn't hard), then consider one of them (say, the first one read) to be the in-memory representative. The others would never be deserialized to a Decl; rather, we would just redirect their DeclIDs over to the representation DeclID.  Again, that's not terribly hard… except that we need to do so recursively, mapping the children of one to the children of the other.

        - Doug
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev