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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
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.
----------------
Aren't these already implicit?


================
Comment at: include/llvm/CodeGen/DIE.h:612-616
+  /// Set to true if this DIE object exists inside a DIEUnit class as the first
+  /// member variable. If true, then "this" can be cast to a "DIEUnit *" and
+  /// allows any DIE object to get to its DIEUnit without having to store a
+  /// pointer to it.
+  bool OwnedByDIEUnit = false;
----------------
That's a pretty subtle API contract.

Duncan tried to do something similar with StringMap maybe (granted, in that case the inner variable wasn't the first member of the outer struct so he was trying to use offset_of) & for some reason that wasn't feasible, if I recall correctly. Perhaps he can tell me/us more about what the issues were there & whether the same concerns apply here.

(in any case - due to alignment, doesn't this bool variable grow the size of the struct by a pointer size anyway? The struct has to be aligned to pointer size (due to the Parent member, and probably the Children member too) , so even in a 64 bit build - Offset, Size, AbbrevNumber, and Tag are 32 bits? So 'Children' appears on the next 64 bit boundary probably, etc... )

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


================
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.
----------------
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?)


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:478-480
+      const DwarfDebug *DD = AP->getDwarfDebug();
+      if (DD)
+        assert(!DD->useSplitDwarf() && "TODO: dwo files can't have relocations.");
----------------
Guessing this is an unrelated change - but seems OK (you could commit it separately/ahead of this patch, to reduce the diff here/simplify code review). 


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:485-486
+      AP->EmitLabelPlusOffset(SectionSym, Addr, DIEEntry::getRefAddrSize(AP));
+    }
     else
       AP->OutStreamer->EmitIntValue(Addr, DIEEntry::getRefAddrSize(AP));
----------------
formatting


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


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:303
   Die.addValue(DIEValueAllocator, Attribute,
-               EntryCU == DieCU ? dwarf::DW_FORM_ref4 : dwarf::DW_FORM_ref_addr,
+               EntryCU == CU ? dwarf::DW_FORM_ref4 : dwarf::DW_FORM_ref_addr,
                Entry);
----------------
Hey Adrian - did you happen to run size numbers on whether this is important? What would happen if we just always used ref_addr when we enabled cross-unit referencing? (wondering if the complexity is worth the benefit - 4 bytes per unit reference isn't nothing, for sure - just wondering how much it is in the end)


================
Comment at: tools/dsymutil/DwarfLinker.cpp:231-232
+  }
+  void setOutputUnitDIE(DIE &&Die) {
+    NewUnit.setUnitDie(std::move(Die));
+  }
----------------
Moving DIEs seems problematic - any sub-DIEs parent pointers will be invalidated by this, right?


================
Comment at: tools/dsymutil/DwarfLinker.cpp:3400-3403
+      if (OutputDIE)
+        CurrentUnit.setOutputUnitDIE(std::move(*OutputDIE));
+      else
+        continue;
----------------
You could reduce indentation here in one of a couple of ways:

Remove the outer scope and nest the variable declaration in the if:

  if (DIE *OutputDIE = cloneDIE(...))
    CurrentUnit.setOutputUnitDIE(std::move(*OutputDIE));
  else
    continue;

or keeping the outer scope (though I don't think it's really necessary - we have moved from objects regularly - you could null out the pointer to help a bit there, if you liked - though that might trigger dead-store warnings eventually) and flipping the if condition while removing the else:

  if (!OutputDIE)
    continue;
  CurrentUnit.setOutputUnitDIE(...);


https://reviews.llvm.org/D27170