[cfe-dev] stable layout bug for imported record decls.

[Lang Hames via cfe-dev]

Hi clang-dev, lldb-dev,

It looks like my clang commit r305850, which modified ASTImporter to import
method override tables from an external context, introduced a new bug which
manifests as incorrect vtable layouts for LLDB expression code.

The bug itself is fairly straightforward. In r305850 I introduced the
following method, which is called from ASTNodeImporter::VisitFunctionDecl:

void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
                                      CXXMethodDecl *FromMethod) {
  for (auto *FromOverriddenMethod : FromMethod->overridden_methods())
    ToMethod->addOverriddenMethod(
      cast<CXXMethodDecl>(Importer.Import(const_cast<CXXMethodDecl*>(
                                            FromOverriddenMethod))));
}

This will produce the correct override table, but can also cause methods in
the Base class to be visited in the wrong order. Consider:

class Base {
public:
  virtual void bar() {}
  virtual void foo() {}
};

class Derived : public Base {
public:
  void foo() override {}
};

If Derived is imported into a new context before Base, then the importer
will visit Derived::foo, and (via ImportOverrides) immediately import
Base::foo, but this happens before Base::bar is imported. As a consequence,
the decl order on the imported Base class will end up being [ foo, bar ],
instead of [ bar, foo ]. In LLDB expression evaluation this manifests as an
incorrect vtable layout for Base, with foo occupying the first slot.

I am looking for suggestions on the right way to fix this. A brute force
solution might be to always have ASTNodeImporter::VisitRecordDecl visit all
base classes, then all virtual methods, which would ensure they are visited
in the original decl order. However I do not know whether this covers all
paths by which a CXXRecordDecl might be imported, nor whether the
performance of this solution would be acceptable (it seems like it would
preclude a lot of laziness).

Alternatively we might be able to associate an index with each imported
decl and sort on that when we complete the type, but that would leave
imported decls in the wrong order until the type was complete, and since I
do not know all the use cases for the importer I'm concerned people may
rely on the decl order before type is complete.

Any insight from ASTImporter experts would be greatly appreciated. :)

Cheers,
Lang.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180809/ffb751ea/attachment.html>