[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