[cfe-dev] clang::Modules and duplicate definitions of base CXXRecordDecls

[Douglas Gregor]

On Oct 2, 2012, at 8:38 AM, Axel Naumann <Axel.Naumann at cern.ch> 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