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

[David Blaikie via llvm-commits]

On Thu, Mar 9, 2017 at 10:49 AM Paul Robinson via Phabricator <
reviews at reviews.llvm.org> wrote:

> probinson added inline comments.
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:82-89
> +    if (!dwarf::isValidFormForVersion(AttrData.getForm(),
> +                                      AP->getDwarfVersion())) {
> +      std::string msg;
> +      raw_string_ostream Msg(msg);
> +      Msg << "Invalid form " << format("0x%x", AttrData.getForm())
> +          << " for DWARF version " << AP->getDwarfVersion();
> +      report_fatal_error(Msg.str());
> ----------------
> dblaikie wrote:
> > Should this be an assertion instead?
> I dithered on that.  An assertion would tell you "something bad happened
> somewhere" with no details, because there's no way I know of to report a
> runtime value in an assertion. So you don't know what FORM is causing the
> problem.  Doing it this way was more valuable when figuring out the test
> failures that I had to fix.
> But, it's true if we detect a problem here, that indicates an internal
> inconsistency rather than a problem with our external input, so on that
> basis an assertion would be more the thing.
>

I believe there are various forms of debug logging that are used for adding
more complex messages near/with an assert. Perhaps you can use that? (I'd
search for 'DEBUG' and 'dbg' in the codebase)


>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:918-919
>      addFlag(Buffer, dwarf::DW_AT_declaration);
> -    return addDIETypeSignature(Buffer, dwarf::DW_AT_signature,
> Identifier);
> +    if (getDwarfVersion() >= 4)
> +      addDIETypeSignature(Buffer, dwarf::DW_AT_signature, Identifier);
> +    return;
> ----------------
> dblaikie wrote:
> > This is probably incorrect (short of the broader discussion I started
> about how references to type units should be implemented - I should ping
> that thread).
> >
> > If the user requests type units, either it should fail if asking for a
> DWARF version that doesn't support type units - or ignore the conflict and
> produce the signature anyway?
> The signature attribute was coming out even without type units.  This was
> added by Adrian for "external" types, something to do with modules.
> I would be okay with emitting the v4 attribute in earlier versions, but
> not the v4 FORM.  We'd have to pick something other than FORM_ref_sig8 for
> that case.
>

Adrian - want to weigh in here?

Much like type units, though, I think the same should hold - rather than
silently omitting it, what's probably required is to either emit it anyway
because the user asked for the feature and must have a compatible consumer
to cope with it, or reject the combination of features that appear to be
contradictory.


>
>
> https://reviews.llvm.org/D30785
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170309/3898eebb/attachment.html>