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

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 17 12:29:14 PDT 2017


On Fri, Mar 17, 2017 at 12:25 PM Paul Robinson via Phabricator <
reviews at reviews.llvm.org> wrote:

> probinson added inline comments.
>
>
> ================
> Comment at: include/llvm/Support/Dwarf.h:420
> +/// These functions return the DWARF version when a constant was first
> defined.
> +/// For most or all vendor extensions it returns 0.
> +///
> ----------------
> dblaikie wrote:
> > Not sure this comment quite makes sense (which is it, most or all? & it
> doesn't describe the contract). I'd be OK with "most" or "legacy" or some
> similar language?
> The policy for different constants will vary widely.  For example,
> DW_AT_MIPS_linkage_name really should be used only *before* the equivalent
> standard attribute existed.   The GNU forms for split DWARF should be used
> only in v4 with experimental split-DWARF.  The GNU TLS opcode should be
> used only with GDB tuning.
> This means I think there is no concise contract to specify; it would have
> to be something along these lines:
>
> For constants defined in the DWARF standard, returns the version when the
> constant was first defined.  For vendor extensions, returns a number that
> might be useful in policy decisions about when the constant can be used.
>

Sounds like it isn't well defined/have a use yet - perhaps put an assertion
in for now (return 0 for all cases that have a vendor version)? With a
FIXME or other thing that'll describe some possible uses (first DWARF
version it's compatible with, last DWARF version it was necessary
in/superceded by a standard feature, etc?)


>
>
>
> ================
> 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");
> +    }
> ----------------
> 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.
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:89
> +                   << " for DWARF version " << AP->getDwarfVersion() <<
> "\n");
> +      assert(0 && "Invalid form for specified DWARF version");
> +    }
> ----------------
> dblaikie wrote:
> > llvm_unreachable is preferred over assert(0)
> I knew that didn't look right...
>
>
> https://reviews.llvm.org/D30785
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170317/443546b3/attachment.html>


More information about the llvm-commits mailing list