Hi Alexey, Gábor, Thank you so much for this information. This is very helpful!
One trivial solution would be to change `ImporDeclContext` to behave > the same way in both modes. This is somewhat similar to the "brute > force" method you mentioned. > I am not an LLDB expert, so I am not sure if this is acceptable, and > really don't know how many LLDB tests would it break, but this seems > the simplest solution (and preferred) to me. Sounds good to me. I will try this, see whether anything fails, and try to get a sense of the performance impact. Do you know of anyone using minimal import mode other than LLDB? Cheers, Lang On Sat, Aug 11, 2018 at 12:20 AM Gábor Márton <martongab...@gmail.com> wrote: > I have forgot to include the matcher I used for the test: > ``` > AST_MATCHER_P(CXXRecordDecl, hasMethodOrder, std::vector<StringRef>, > Order) { > size_t Index = 0; > for (CXXMethodDecl *Method : Node.methods()) { > if (!Method->isImplicit()) { > if (Method->getName() != Order[Index]) > return false; > ++Index; > } > } > return Index == Order.size(); > } > ``` > > Gabor > On Fri, Aug 10, 2018 at 6:42 PM Gábor Márton <martongab...@gmail.com> > wrote: > > > > Hi Lang, Alexey, > > > > I dug deeper into this and it seems like the issue happens only when a > > **minimal** import is used. LLDB uses the minimal import. CrossTU > > static analyzer uses the normal mode. > > In normal import mode, in `ImportDeclContext` we do import all the > > methods in the correct order. However, in minimal mode we early return > > before importing the methods. > > So by merging D44100 this particular problem will not be solved. > > Still, that patch sounds very reasonable and I support that we should > > reorder all imported Decls, not just fields. > > > > One trivial solution would be to change `ImporDeclContext` to behave > > the same way in both modes. This is somewhat similar to the "brute > > force" method you mentioned. > > I am not an LLDB expert, so I am not sure if this is acceptable, and > > really don't know how many LLDB tests would it break, but this seems > > the simplest solution (and preferred) to me. > > > > The other solution if you'd like to keep the minimal behavior is the > > index based solution (as you mentioned). > > You should compare the order of all the imported methods (and fields) > > to the order in the original class in the "From" context. And I > > believe you have to do that at the end of VisitFunctionDecl. It would > > not work if you did that check when the type become complete, since in > > minimal mode we never set the class to be incomplete. > > > > I have created a unit test case, which fails in minimal mode and > > succeeds in normal mode. You can change the mode in > > `ASTImporterTestBase::TU::lazyInitImporter`. > > If you provide a patch then could you please also add this test (or > > similar) for both normal and minimal mode? > > ``` > > TEST_P(ASTImporterTestBase, OrderOfVirtualMethods) { > > auto Code = > > R"( > > class Base { > > public: > > virtual void bar() {} > > virtual void foo() {} > > }; > > > > class Derived : public Base { > > public: > > void foo() override {} > > }; > > )"; > > Decl *FromTU = getTuDecl(Code, Lang_CXX11, "input0.cc"); > > auto *DerivedFoo = FirstDeclMatcher<FunctionDecl>().match( > > FromTU, functionDecl(hasName("foo"), > > > hasParent(cxxRecordDecl(hasName("Derived"))))); > > auto *BaseBar = FirstDeclMatcher<FunctionDecl>().match( > > FromTU, functionDecl(hasName("bar"), > > hasParent(cxxRecordDecl(hasName("Base"))))); > > > > Import(DerivedFoo, Lang_CXX); > > // Importing Base::bar explicitly is needed only in minimal mode, > > // in normal mode we already imported all methods of Base. > > Import(BaseBar, Lang_CXX); > > > > Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl(); > > auto *ImportedBase = FirstDeclMatcher<CXXRecordDecl>().match( > > ToTU, cxxRecordDecl(hasName("Base"))); > > MatchVerifier<Decl> Verifier; > > EXPECT_TRUE(Verifier.match(ImportedBase, > > cxxRecordDecl(hasMethodOrder({"bar", > "foo"})))); > > } > > ``` > > > > Thanks, > > Gabor > > On Fri, Aug 10, 2018 at 3:47 PM Alexey Sidorin <alexey.v.sido...@ya.ru> > wrote: > > > > > > (+ Gabor and Gabor) > > > > > > Hi Lang, > > > > > > We faced a very similar issue with record fields where import order > can change the order of imported FieldDecls resulting in broken > ASTRecordLayout. The patch for this issue is on review: > https://reviews.llvm.org/D44100. It just reorders the fields after > structure import is finished. CSA guys also reported the same problem with > FriendDecls in the same review.The order of methods was not a problem for > us but your report adds a new item to support. It looks like _all_ decls > inside RecordDecl have to be reordered. I'll try to resurrect the patch > this weekend (it is a bit outdated due to my workload, sorry) and add you > as a reviewer so you can check if it solves the problem or not. > > > > > > 09.08.2018 20:46, Lang Hames via lldb-dev пишет: > > > > > > 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. :) > > > > > > Cheers, > > > Lang. > > > > > > > > > _______________________________________________ > > > lldb-dev mailing list > > > lldb-dev@lists.llvm.org > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev > > > > > > >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev