<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 28, 2016 at 1:10 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added a comment.<br class="gmail_msg">
<br class="gmail_msg">
Will switch over to using PointerUnion to avoid the strict API contracts and fix the rest accordingly.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/CodeGen/DIE.h:87-88<br class="gmail_msg">
DIEAbbrev(dwarf::Tag T, bool C) : Tag(T), Children(C) {}<br class="gmail_msg">
-<br class="gmail_msg">
+ DIEAbbrev(const DIEAbbrev &RHS) = default;<br class="gmail_msg">
+ DIEAbbrev(DIEAbbrev &&RHS) = default;<br class="gmail_msg">
/// Accessors.<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Aren't these already implicit?<br class="gmail_msg">
Yep, they are. There was code in DwarfLinker a while back that was manually copying a DIEAbbrev for no reason so I added these when I removed that redundant code. These aren't needed.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/CodeGen/DIE.h:616<br class="gmail_msg">
+ /// pointer to it.<br class="gmail_msg">
+ bool OwnedByDIEUnit = false;<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
Yes it is subtle API contract in an effort to save space.<br class="gmail_msg">
<br class="gmail_msg">
This doesn't grow this class because dwarf::Tag in 16 bits (enum class uint16_t) and the next class will be 4 or 8 byte aligned so it will sit in the gap.<br class="gmail_msg">
<br class="gmail_msg">
> Could we instead change 'Parent' to be a PointerUnion<DIE, DIEUnit> & wouldn't need the offset tricks anyway?<br class="gmail_msg">
<br class="gmail_msg">
Great idea. That solves the problem without growing the size.<br class="gmail_msg"></blockquote><div><br>Great! Look forward to seeing how it turns out.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/CodeGen/DIE.h:721<br class="gmail_msg">
+ DIEUnit(uint16_t Version, uint8_t AddrSize, dwarf::Tag UnitTag);<br class="gmail_msg">
+ DIEUnit(DIEUnit &&RHS) = default;<br class="gmail_msg">
+ /// Set the section that this DIEUnit will be emitted into.<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> This is implicit/default, right? (but I suppose if this thing is movable, then having the DIE physically point to its DIEUnit could be tricky - equally, though, all the DIE's children parent pointers would be broken by such an API as well, right?)<br class="gmail_msg">
I will think about this as I am switching over to use a PointerUnion and make any needed modifications.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:480<br class="gmail_msg">
+ if (DD)<br class="gmail_msg">
+ assert(!DD->useSplitDwarf() && "TODO: dwo files can't have relocations.");<br class="gmail_msg">
+ MCSection *Section = Unit->getSection();<br class="gmail_msg">
----------------<br class="gmail_msg">
This is leaving in he assert that used to be on line 448 to the right? Am I missing something?<br class="gmail_msg"></blockquote><div><br></div><div>I meant specifically the fact that the code moved/sunk into the if(relocations) block - that change seems separate?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:489<br class="gmail_msg">
} else<br class="gmail_msg">
- AP->EmitInt32(Entry->getOffset());<br class="gmail_msg">
+ AP->OutStreamer->EmitIntValue(Entry->getOffset(), SizeOf(AP, Form));<br class="gmail_msg">
}<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Is this an unrelated/orthogonal change?<br class="gmail_msg">
Intentional change that fixes emitting DW_FORM_ref_addr for DWARF version 2 which was broken. It would always emit 4 bytes regardless of the DWARF version.<br class="gmail_msg"></blockquote><div><br></div><div>Be good to separate that out into its own patch (with test). Sounds like it could go before or after this change.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: tools/dsymutil/DwarfLinker.cpp:232<br class="gmail_msg">
+ void setOutputUnitDIE(DIE &&Die) {<br class="gmail_msg">
+ NewUnit.setUnitDie(std::move(Die));<br class="gmail_msg">
+ }<br class="gmail_msg">
----------------<br class="gmail_msg">
I will switch to DIE to use a PointerUnion and fix accordingly.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D27170" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D27170</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>