On Mon, 13 Aug 2018 at 17:08, Lang Hames via cfe-dev <cfe-...@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 <rich...@metafoo.co.uk> > wrote: > >> On Thu, 9 Aug 2018 at 10:47, Lang Hames via cfe-dev < >> cfe-...@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-...@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev