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

[David Blaikie via llvm-commits]

On Mon, Nov 28, 2016 at 1:10 PM Greg Clayton via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>

Great! Look forward to seeing how it turns out.


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

I meant specifically the fact that the code moved/sunk into the
if(relocations) block - that change seems separate?


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

Be good to separate that out into its own patch (with test). Sounds like it
could go before or after this change.


>
>
> ================
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161128/15bfbc8b/attachment.html>