<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">(+ 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 class="moz-txt-link-freetext" href="https://reviews.llvm.org/D44100">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>
</div>
<blockquote type="cite"
cite="mid:CALLttgqYkZWXkGzGMLFgGOh0X7GuVBGJY74z38TrQq_tZSYnMg@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">Hi clang-dev, lldb-dev,
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>The bug itself is fairly straightforward. In r305850 I
introduced the following method, which is called from
ASTNodeImporter::VisitFunctionDecl:</div>
<div><br>
</div>
<div><font face="monospace, monospace">void
ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,</font></div>
<div><font face="monospace, monospace">
CXXMethodDecl *FromMethod) {</font></div>
<div><font face="monospace, monospace"> for (auto
*FromOverriddenMethod : FromMethod->overridden_methods())</font></div>
<div><font face="monospace, monospace">
ToMethod->addOverriddenMethod(</font></div>
<div><font face="monospace, monospace">
cast<CXXMethodDecl>(Importer.Import(const_cast<CXXMethodDecl*>(</font></div>
<div><font face="monospace, monospace">
FromOverriddenMethod))));</font></div>
<div><font face="monospace, monospace">}</font></div>
<div><br>
</div>
<div>This will produce the correct override table, but can also
cause methods in the Base class to be visited in the wrong
order. Consider:</div>
<div><br>
</div>
<div><font face="monospace, monospace">class Base {</font></div>
<div><font face="monospace, monospace">public:</font></div>
<div><font face="monospace, monospace"> virtual void bar() {}</font></div>
<div><font face="monospace, monospace"> virtual void foo() {}</font></div>
<div><font face="monospace, monospace">};</font></div>
<div><font face="monospace, monospace"><br>
</font></div>
<div><font face="monospace, monospace">class Derived : public
Base {</font></div>
<div><font face="monospace, monospace">public:</font></div>
<div><font face="monospace, monospace"> void foo() override {}</font></div>
<div><font face="monospace, monospace">};</font></div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Any insight from ASTImporter experts would be greatly
appreciated. :)</div>
<div><br>
</div>
<div>Cheers,</div>
<div>Lang.</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
lldb-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a>
</pre>
</blockquote>
<p><br>
</p>
</body>
</html>