[PATCH] D30785: [DWARF] Versioning for DWARF constants; verify FORMs

[David Blaikie via llvm-commits]

On Mon, Apr 10, 2017 at 4:41 PM Paul Robinson via Phabricator <
reviews at reviews.llvm.org> wrote:

> probinson added inline comments.
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:85-90
> +    if (!dwarf::isValidFormForVersion(AttrData.getForm(),
> +                                      AP->getDwarfVersion())) {
> +      DEBUG(dbgs() << "Invalid form " << format("0x%x",
> AttrData.getForm())
> +                   << " for DWARF version " << AP->getDwarfVersion() <<
> "\n");
> +      assert(0 && "Invalid form for specified DWARF version");
> +    }
> ----------------
> probinson wrote:
> > dblaikie wrote:
> > > Might it be more helpful to fail at the point where the DIEAbbrev is
> created, rather than emitted? (more debuggable - the smoking gun, as it
> were) - would also save/avoid the need to plumb the dwarf version down
> through some of the layers added in this change.
> > I will look into that.  I put it here because that's where the check for
> implicit_const had been.
> The form/value information is created by DIE::addValue() (inherited from
> DIEValueList::addValue()).  These places do not have direct access to the
> DWARF version, we'd have to follow the DIE parent chain up to the unit.  Is
> that cheap enough to do every time we add any attribute to any DIE?
> Alternatively, caching the version in the DIE makes every DIE a word
> bigger.  Or leave the check here.
>

Walking up to the unit for each form insertion, only in a +Asserts build
seems fine/not too expensive to me.


>
> (There's currently no "plumbing" of the version down through layers in
> this patch, except for dsymutil which for correctness has to remember the
> max version number it has seen anyway. Moving the check to DIEValueList
> would introduce some plumbing.)
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:297-298
>    Die.addValue(DIEValueAllocator, dwarf::DW_AT_signature,
> -               dwarf::DW_FORM_ref_sig8, DIEInteger(Signature));
> +               getDwarfVersion() >= 4 ? dwarf::DW_FORM_ref_sig8
> +                                      : dwarf::DW_FORM_data8,
> +               DIEInteger(Signature));
> ----------------
> dblaikie wrote:
> > Still have uncertainty about this change - type units are only supported
> in DWARF4 and above. I think you were saying this was used by a particular
> prototype of modular debug info that ended up being a dead-end and Adrian
> removed.
> >
> > Is this still needed?
> Now that Adrian has removed the other usage, yes this is no longer
> necessary.
>
>
> https://reviews.llvm.org/D30785
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170411/d269d0d4/attachment.html>