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

[Richard Smith via lldb-dev]

On Thu, 9 Aug 2018 at 10:47, Lang Hames via cfe-dev <cfe-dev at lists.llvm.org>
wrote:

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

Hi Lang,

It might be interesting to consider how this is addressed by the
ExternalASTSource interface. There, we have separate "import the lexical
contents of this AST node (including populating the lexical declarations
list with all members, in the right order)", "import the lookup results for
this name in this context (and cache them but don't add them to the list of
lexical members)" and "import this specific declaration member (but don't
add it to the list of lexical members)" queries. One could imagine doing
something similar for the AST importer: when you're performing a minimal
import but you want to import a method declaration, don't insert it into
the list of lexical members of the enclosing record. Instead, defer doing
that until a complete type is required (at that point, you'd need to import
all the relevant members anyway, including the base classes and virtual
functions in the right order).

The above approach would violate AST invariants (you'd have a declaration
whose lexical parent doesn't believe it lexically contains the child), but
I don't know off-hand whether that would be problematic. Perhaps a better
approach would be to make the "minimal" mode for the ASTImporter provide an
ExternalASTSource to lazily complete the AST as needed (thereby avoiding
violating the invariant, because you would populate the lexical
declarations list whenever anyone actually asks for it).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20180813/53ea65cc/attachment-0001.html>