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

[Richard Smith via lldb-dev]

On Mon, 13 Aug 2018 at 17:08, Lang Hames via cfe-dev <cfe-dev at lists.llvm.org>
wrote:

> 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?
>

Yes, such an approach should be able to solve that problem too: when
CodeGen (or indeed anything) queries any property of the definition of Foo
/ Bar (including whether a definition exists), you'll get a callback and a
chance to provide a complete type.


> 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).
>>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20180813/df458701/attachment-0001.html>