<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 10, 2016 at 3:16 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">dblaikie added inline comments.<br></blockquote><div><br></div><div>Thanks for the comments! I had hoped to address these all before I left town, but ran out of time. A few responses below, will address the rest when I get back!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1386<br>
@@ +1385,3 @@<br>
+ // that is needed, and we should only invoke this for needed composite types.<br>
+ assert(RMI != RetainMap.end());<br>
+ // If we already created a new composite type declaration, use it.<br>
----------------<br>
I'm not sure this is true. Here's a type you might have to import that is not in the retained types list:<br>
<br>
namespace {<br>
struct foo { };<br>
void f(foo) { }<br>
}<br>
struct bar { };<br>
void f(bar) { }<br>
void f() {<br>
f(foo());<br>
f(bar());<br>
}<br>
<br>
The 'foo' type is not in the retained types list, but it may need to be imported if 'f(foo)' is imported, yes?<br></blockquote><div><br></div><div>While foo isn't in the retained types list, it is imported as it is reached via the DISubprogram for f(foo) and we do pull it in (confirmed with the above example and this patch). Perhaps the comment could be clarified to refer to needed retained types, not composite types, since this is only dealing with types reached via the retained types list.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1398<br>
@@ +1397,3 @@<br>
+ Composite->getFile(), Composite->getLine(), NewScope, NewBaseType,<br>
+ Composite->getSizeInBits(), Composite->getAlignInBits(),<br>
+ Composite->getOffsetInBits(), Composite->getFlags() | DINode::FlagFwdDecl,<br>
----------------<br>
Pretty sure you don't need the base type, size, alignment, and offset in a declaration - check equivalent declarations that are generated by clang? (check that the declarations this code creates look like natural declarations in the original source language, etc)<br></blockquote><div><br></div><div>They do seem to have size and alignment, but not baseType or offset. Will fix. Here's an example:</div><div><br></div><div> !1215 = !DICompositeType(tag: DW_TAG_class_type, name: "DOMText", scope: !524, file: !1216, line: 90, size: 64, align: 64, flags: DIFlagFwdDecl, identifier: "_ZTSN11xercesc_2_57DOMTextE")</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1411<br>
@@ +1410,3 @@<br>
+Metadata *<br>
+IRLinker::importType(Metadata *Ty,<br>
+ DenseMap<DICompositeType *, DICompositeType *> &RetainMap,<br>
----------------<br>
This name seems a bit misleading. If I'm reading the code correctly, this is used for any type's scope context - which might not be a type at all. (it could be a namespace, for example) - and I assume the non-type cases come out in the "map the type metadata normally" part?<br></blockquote><div><br></div><div>Correct, it will also handle DINamespace which will be mapped normally via MapMetadata. I will rename this to something better.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1427<br>
@@ +1426,3 @@<br>
+<br>
+void IRLinker::linkImportedCompileUnit() {<br>
+ NamedMDNode *SrcCompileUnits = SrcM.getNamedMetadata("<a href="http://llvm.dbg.cu" rel="noreferrer" target="_blank">llvm.dbg.cu</a>");<br>
----------------<br>
Name seems a bit off - this links in potentially multiple compile units, not just a single one.<br></blockquote><div><br></div><div>Right, will make the name plural.</div><div><br></div><div>This is as far as I got. Will address the rest when I am back in town Monday!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1444<br>
@@ +1443,3 @@<br>
<span class="">+ std::vector<DICompositeType *> RetainWorklist;<br>
+ if (!IsMetadataLinkingPostpass) {<br>
</span>+ for (DIType *Ty : CU->getRetainedTypes()) {<br>
----------------<br>
What's postpass metadata linking? (what's the alternative?)<br>
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1445<br>
@@ +1444,3 @@<br>
+ if (!IsMetadataLinkingPostpass) {<br>
+ for (DIType *Ty : CU->getRetainedTypes()) {<br>
+ // For non-postpass metadata linking, any metadata referenced<br>
----------------<br>
I'm not really following why we need to iterate the retained types list - wouldn't we import everything starting from the subprograms we needed to import, not the retained types list?<br>
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1534<br>
@@ +1533,3 @@<br>
+ stripNullSubprograms(NewCU);<br>
+ DestCompileUnits->addOperand(NewCU);<br>
+ }<br>
----------------<br>
What if nothing was imported from this CU?<br>
<br>
It seems to me we'd want to start at the imported functions, follow their DISubprograms, and import that way, rather than walking all the CUs and subprograms - maybe?<br>
<br>
<br>
<a href="http://reviews.llvm.org/D16440" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16440</a><br>
<br>
<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> 408-460-2413</td></tr></tbody></table></span></div>
</div></div>