Re: [lldb-dev] stable layout bug for imported record decls.
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
wrote:
> I have forgot to include the matcher I used for the test:
> ```
> AST_MATCHER_P(CXXRecordDecl, hasMethodOrder, std::vector,
> 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
> 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().match(
> > FromTU, functionDecl(hasName("foo"),
> >
> hasParent(cxxRecordDecl(hasName("Derived");
> > auto *BaseBar = FirstDeclMatcher().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().match(
> > ToTU, cxxRecordDecl(hasName("Base")));
> > MatchVerifier Verifier;
> > EXPECT_TRUE(Verifier.match(ImportedBase,
> > cxxRecordDecl(hasMethodOrder({"bar",
> "foo"};
> > }
> > ```
> >
> > Thanks,
> > Gabor
> > On Fri, Aug 10, 2018 at 3:47 PM Alexey Sidorin
> 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
Re: [lldb-dev] stable layout bug for imported record decls.
I have forgot to include the matcher I used for the test:
```
AST_MATCHER_P(CXXRecordDecl, hasMethodOrder, std::vector, 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 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().match(
> FromTU, functionDecl(hasName("foo"),
>hasParent(cxxRecordDecl(hasName("Derived");
> auto *BaseBar = FirstDeclMatcher().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().match(
> ToTU, cxxRecordDecl(hasName("Base")));
> MatchVerifier Verifier;
> EXPECT_TRUE(Verifier.match(ImportedBase,
> cxxRecordDecl(hasMethodOrder({"bar", "foo"};
> }
> ```
>
> Thanks,
> Gabor
> On Fri, Aug 10, 2018 at 3:47 PM Alexey Sidorin 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(Importer.Import(const_cast(
> >
Re: [lldb-dev] stable layout bug for imported record decls.
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().match(
FromTU, functionDecl(hasName("foo"),
hasParent(cxxRecordDecl(hasName("Derived");
auto *BaseBar = FirstDeclMatcher().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().match(
ToTU, cxxRecordDecl(hasName("Base")));
MatchVerifier Verifier;
EXPECT_TRUE(Verifier.match(ImportedBase,
cxxRecordDecl(hasMethodOrder({"bar", "foo"};
}
```
Thanks,
Gabor
On Fri, Aug 10, 2018 at 3:47 PM Alexey Sidorin 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(Importer.Import(const_cast(
> 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
