[cfe-dev] [lldb-dev] stable layout bug for imported record decls.
Alexey Sidorin via cfe-dev
cfe-dev at lists.llvm.org
Mon Aug 13 14:37:00 PDT 2018
Hi Lang,
There are only two users of ASTImporter known to me: CSA and LLDB. And
CSA doesn't use minimal import.
13.08.2018 23:28, Lang Hames via lldb-dev пишет:
> 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 <martongabesz at gmail.com
> <mailto:martongabesz at 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
> <martongabesz at gmail.com <mailto:martongabesz at 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.sidorin at ya.ru <mailto:alexey.v.sidorin at 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 at lists.llvm.org <mailto:lldb-dev at lists.llvm.org>
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180814/63239e01/attachment.html>
More information about the cfe-dev
mailing list