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

[Lang Hames via cfe-dev]

Hi Richard,

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


This seems worth investigating. I wonder if it might also fix another bug
that I know of involving virtual methods with covariant return types. (You
and I actually discussed it at one of the socials a while back, but I have
not had time to dig into it further.)

The reproducer for the bug is:

class Foo {};
class Bar : public Foo {};

class Base {
public:
  virtual Foo* foo() { return nullptr; }
};

class Derived : public Base {
public:
  virtual Bar* foo() { return nullptr; }
};

int main() {
  Derived D;
  D.foo(); // Evaluate 'D.foo()' here, crash LLDB.
}

The issue is that since Bar's definition is not used its bases are never
imported, and so the call to Bar::bases() crashes in CodeGen. If we
provided an ExternalASTSource, would that be able to lazily complete the
bases?

Cheers,
Lang.


On Mon, Aug 13, 2018 at 3:30 PM Richard Smith <richard at metafoo.co.uk> wrote:

> 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/cfe-dev/attachments/20180813/18bc2a44/attachment.html>