[PATCH] D27170: Abstract almost all DwarfDebug out of the classes in DIE.cpp.

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

Will switch over to using PointerUnion to avoid the strict API contracts and fix the rest accordingly.



================
Comment at: include/llvm/CodeGen/DIE.h:87-88
   DIEAbbrev(dwarf::Tag T, bool C) : Tag(T), Children(C) {}
-
+  DIEAbbrev(const DIEAbbrev &RHS) = default;
+  DIEAbbrev(DIEAbbrev &&RHS) = default;
   /// Accessors.
----------------
dblaikie wrote:
> Aren't these already implicit?
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.


================
Comment at: include/llvm/CodeGen/DIE.h:616
+  /// pointer to it.
+  bool OwnedByDIEUnit = false;
+
----------------
Yes it is subtle API contract in an effort to save space.

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.

> Could we instead change 'Parent' to be a PointerUnion<DIE, DIEUnit> & wouldn't need the offset tricks anyway?

Great idea. That solves the problem without growing the size.



================
Comment at: include/llvm/CodeGen/DIE.h:721
+  DIEUnit(uint16_t Version, uint8_t AddrSize, dwarf::Tag UnitTag);
+  DIEUnit(DIEUnit &&RHS) = default;
+  /// Set the section that this DIEUnit will be emitted into.
----------------
dblaikie wrote:
> 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?)
I will think about this as I am switching over to use a PointerUnion and make any needed modifications.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:480
+      if (DD)
+        assert(!DD->useSplitDwarf() && "TODO: dwo files can't have relocations.");
+      MCSection *Section = Unit->getSection();
----------------
This is leaving in he assert that used to be on line 448 to the right? Am I missing something?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:489
   } else
-    AP->EmitInt32(Entry->getOffset());
+    AP->OutStreamer->EmitIntValue(Entry->getOffset(), SizeOf(AP, Form));
 }
----------------
dblaikie wrote:
> Is this an unrelated/orthogonal change?
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.


================
Comment at: tools/dsymutil/DwarfLinker.cpp:232
+  void setOutputUnitDIE(DIE &&Die) {
+    NewUnit.setUnitDie(std::move(Die));
+  }
----------------
I will switch to DIE to use a PointerUnion and fix accordingly. 


https://reviews.llvm.org/D27170