[cfe-dev] [lldb-dev] stable layout bug for imported record decls.
Lang Hames via cfe-dev
cfe-dev at lists.llvm.org
Mon Aug 13 13:28:21 PDT 2018
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>
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>
> 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>
> 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
> > > 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/20180813/0c45ca59/attachment.html>
More information about the cfe-dev
mailing list