<div dir="ltr">Hi Richard,<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">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).</blockquote><div><br></div><div>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.)</div><div><br></div><div>The reproducer for the bug is:</div><div><br></div><div><div><font face="monospace, monospace">class Foo {};</font></div><div><font face="monospace, monospace">class Bar : public Foo {};</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">class Base {</font></div><div><font face="monospace, monospace">public:</font></div><div><font face="monospace, monospace"> virtual Foo* foo() { return nullptr; }</font></div><div><font face="monospace, monospace">};</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">class Derived : public Base {</font></div><div><font face="monospace, monospace">public:</font></div><div><font face="monospace, monospace"> virtual Bar* foo() { return nullptr; }</font></div><div><font face="monospace, monospace">};</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">int main() {</font></div><div><font face="monospace, monospace"> Derived D;</font></div><div><font face="monospace, monospace"> D.foo(); // Evaluate 'D.foo()' here, crash LLDB.</font></div><div><font face="monospace, monospace">}</font></div></div><div><br></div><div>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? </div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 13, 2018 at 3:30 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, 9 Aug 2018 at 10:47, Lang Hames via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi clang-dev, lldb-dev,<div><br></div><div>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.</div><div><br></div><div>The bug itself is fairly straightforward. In r305850 I introduced the following method, which is called from ASTNodeImporter::VisitFunctionDecl:</div><div><br></div><div><font face="monospace, monospace">void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,</font></div><div><font face="monospace, monospace"> CXXMethodDecl *FromMethod) {</font></div><div><font face="monospace, monospace"> for (auto *FromOverriddenMethod : FromMethod->overridden_methods())</font></div><div><font face="monospace, monospace"> ToMethod->addOverriddenMethod(</font></div><div><font face="monospace, monospace"> cast<CXXMethodDecl>(Importer.Import(const_cast<CXXMethodDecl*>(</font></div><div><font face="monospace, monospace"> FromOverriddenMethod))));</font></div><div><font face="monospace, monospace">}</font></div><div><br></div><div>This will produce the correct override table, but can also cause methods in the Base class to be visited in the wrong order. Consider:</div><div><br></div><div><font face="monospace, monospace">class Base {</font></div><div><font face="monospace, monospace">public:</font></div><div><font face="monospace, monospace"> virtual void bar() {}</font></div><div><font face="monospace, monospace"> virtual void foo() {}</font></div><div><font face="monospace, monospace">};</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">class Derived : public Base {</font></div><div><font face="monospace, monospace">public:</font></div><div><font face="monospace, monospace"> void foo() override {}</font></div><div><font face="monospace, monospace">};</font></div><div><br></div><div>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.</div><div><br></div><div>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).</div><div><br></div><div>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.</div><div><br></div><div>Any insight from ASTImporter experts would be greatly appreciated. :)</div></div></blockquote><div><br></div><div>Hi Lang,</div><div><br></div><div>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).</div><div><br></div><div>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).</div></div></div>
</blockquote></div>