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
