<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Lang,<br>
<br>
There are only two users of ASTImporter known to me: CSA and LLDB.
And CSA doesn't use minimal import.<br>
<br>
<br>
13.08.2018 23:28, Lang Hames via lldb-dev пишет:<br>
</div>
<blockquote type="cite"
cite="mid:CALLttgoY7D5QQ6S3M37ZwkDBpYD=FHt8R9=HWxdo1z_fEJfXeg@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">Hi Alexey, Gábor,
<div><br>
</div>
<div>Thank you so much for this information. This is very
helpful!</div>
<div><br>
</div>
<div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">One
trivial solution would be to change `ImporDeclContext` to
behave<br>
the same way in both modes. This is somewhat similar to the
"brute<br>
force" method you mentioned.<br>
I am not an LLDB expert, so I am not sure if this is
acceptable, and<br>
really don't know how many LLDB tests would it break, but
this seems<br>
the simplest solution (and preferred) to me.</blockquote>
</div>
<div><br>
</div>
<div>Sounds good to me. I will try this, see whether anything
fails, and try to get a sense of the performance impact.</div>
<div><br>
</div>
<div>Do you know of anyone using minimal import mode other than
LLDB?</div>
<div><br>
</div>
<div>Cheers,</div>
<div>Lang</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Sat, Aug 11, 2018 at 12:20 AM Gábor Márton
<<a href="mailto:martongabesz@gmail.com"
moz-do-not-send="true">martongabesz@gmail.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">I have
forgot to include the matcher I used for the test:<br>
```<br>
AST_MATCHER_P(CXXRecordDecl, hasMethodOrder,
std::vector<StringRef>, Order) {<br>
size_t Index = 0;<br>
for (CXXMethodDecl *Method : Node.methods()) {<br>
if (!Method->isImplicit()) {<br>
if (Method->getName() != Order[Index])<br>
return false;<br>
++Index;<br>
}<br>
}<br>
return Index == Order.size();<br>
}<br>
```<br>
<br>
Gabor<br>
On Fri, Aug 10, 2018 at 6:42 PM Gábor Márton <<a
href="mailto:martongabesz@gmail.com" target="_blank"
moz-do-not-send="true">martongabesz@gmail.com</a>> wrote:<br>
><br>
> Hi Lang, Alexey,<br>
><br>
> I dug deeper into this and it seems like the issue
happens only when a<br>
> **minimal** import is used. LLDB uses the minimal import.
CrossTU<br>
> static analyzer uses the normal mode.<br>
> In normal import mode, in `ImportDeclContext` we do
import all the<br>
> methods in the correct order. However, in minimal mode we
early return<br>
> before importing the methods.<br>
> So by merging D44100 this particular problem will not be
solved.<br>
> Still, that patch sounds very reasonable and I support
that we should<br>
> reorder all imported Decls, not just fields.<br>
><br>
> One trivial solution would be to change
`ImporDeclContext` to behave<br>
> the same way in both modes. This is somewhat similar to
the "brute<br>
> force" method you mentioned.<br>
> I am not an LLDB expert, so I am not sure if this is
acceptable, and<br>
> really don't know how many LLDB tests would it break, but
this seems<br>
> the simplest solution (and preferred) to me.<br>
><br>
> The other solution if you'd like to keep the minimal
behavior is the<br>
> index based solution (as you mentioned).<br>
> You should compare the order of all the imported methods
(and fields)<br>
> to the order in the original class in the "From" context.
And I<br>
> believe you have to do that at the end of
VisitFunctionDecl. It would<br>
> not work if you did that check when the type become
complete, since in<br>
> minimal mode we never set the class to be incomplete.<br>
><br>
> I have created a unit test case, which fails in minimal
mode and<br>
> succeeds in normal mode. You can change the mode in<br>
> `ASTImporterTestBase::TU::lazyInitImporter`.<br>
> If you provide a patch then could you please also add
this test (or<br>
> similar) for both normal and minimal mode?<br>
> ```<br>
> TEST_P(ASTImporterTestBase, OrderOfVirtualMethods) {<br>
> auto Code =<br>
> R"(<br>
> class Base {<br>
> public:<br>
> virtual void bar() {}<br>
> virtual void foo() {}<br>
> };<br>
><br>
> class Derived : public Base {<br>
> public:<br>
> void foo() override {}<br>
> };<br>
> )";<br>
> Decl *FromTU = getTuDecl(Code, Lang_CXX11,
"input0.cc");<br>
> auto *DerivedFoo =
FirstDeclMatcher<FunctionDecl>().match(<br>
> FromTU, functionDecl(hasName("foo"),<br>
>
hasParent(cxxRecordDecl(hasName("Derived")))));<br>
> auto *BaseBar =
FirstDeclMatcher<FunctionDecl>().match(<br>
> FromTU, functionDecl(hasName("bar"),<br>
>
hasParent(cxxRecordDecl(hasName("Base")))));<br>
><br>
> Import(DerivedFoo, Lang_CXX);<br>
> // Importing Base::bar explicitly is needed only in
minimal mode,<br>
> // in normal mode we already imported all methods of
Base.<br>
> Import(BaseBar, Lang_CXX);<br>
><br>
> Decl *ToTU =
ToAST->getASTContext().getTranslationUnitDecl();<br>
> auto *ImportedBase =
FirstDeclMatcher<CXXRecordDecl>().match(<br>
> ToTU, cxxRecordDecl(hasName("Base")));<br>
> MatchVerifier<Decl> Verifier;<br>
> EXPECT_TRUE(Verifier.match(ImportedBase,<br>
>
cxxRecordDecl(hasMethodOrder({"bar", "foo"}))));<br>
> }<br>
> ```<br>
><br>
> Thanks,<br>
> Gabor<br>
> On Fri, Aug 10, 2018 at 3:47 PM Alexey Sidorin <<a
href="mailto:alexey.v.sidorin@ya.ru" target="_blank"
moz-do-not-send="true">alexey.v.sidorin@ya.ru</a>> wrote:<br>
> ><br>
> > (+ Gabor and Gabor)<br>
> ><br>
> > Hi Lang,<br>
> ><br>
> > 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: <a href="https://reviews.llvm.org/D44100"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://reviews.llvm.org/D44100</a>.
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.<br>
> ><br>
> > 09.08.2018 20:46, Lang Hames via lldb-dev пишет:<br>
> ><br>
> > Hi clang-dev, lldb-dev,<br>
> ><br>
> > 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.<br>
> ><br>
> > The bug itself is fairly straightforward. In r305850
I introduced the following method, which is called from
ASTNodeImporter::VisitFunctionDecl:<br>
> ><br>
> > void ASTNodeImporter::ImportOverrides(CXXMethodDecl
*ToMethod,<br>
> > CXXMethodDecl
*FromMethod) {<br>
> > for (auto *FromOverriddenMethod :
FromMethod->overridden_methods())<br>
> > ToMethod->addOverriddenMethod(<br>
> >
cast<CXXMethodDecl>(Importer.Import(const_cast<CXXMethodDecl*>(<br>
> >
FromOverriddenMethod))));<br>
> > }<br>
> ><br>
> > This will produce the correct override table, but
can also cause methods in the Base class to be visited in the
wrong order. Consider:<br>
> ><br>
> > class Base {<br>
> > public:<br>
> > virtual void bar() {}<br>
> > virtual void foo() {}<br>
> > };<br>
> ><br>
> > class Derived : public Base {<br>
> > public:<br>
> > void foo() override {}<br>
> > };<br>
> ><br>
> > 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.<br>
> ><br>
> > 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).<br>
> ><br>
> > 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.<br>
> ><br>
> > Any insight from ASTImporter experts would be
greatly appreciated. :)<br>
> ><br>
> > Cheers,<br>
> > Lang.<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > lldb-dev mailing list<br>
> > <a href="mailto:lldb-dev@lists.llvm.org"
target="_blank" moz-do-not-send="true">lldb-dev@lists.llvm.org</a><br>
> > <a
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev"
rel="noreferrer" target="_blank" moz-do-not-send="true">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
> ><br>
> ><br>
</blockquote>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>