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

[Adrian Prantl via llvm-commits]

> On Mar 9, 2017, at 10:54 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Thu, Mar 9, 2017 at 10:49 AM Paul Robinson via Phabricator <reviews at reviews.llvm.org <mailto: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?

[Sorry, I was out for a few days.]
We don't use ref_sig8 (or any signatures) for "externally defined" types any more, so what David says sound fine to me.

-- adrian

> 
> 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 <https://reviews.llvm.org/D30785>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170313/b246a390/attachment.html>