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

[David Blaikie via llvm-commits]

Also, Adrian - still curious to hear if you have any thoughts about /where/
that check should go (see the review history discussing failing when the
attribute is added, rather than later when the abbreviation is created) as
well as how it should be implemented (assert V report fatal, etc)

On Thu, Apr 20, 2017 at 10:19 AM David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Apr 20, 2017 at 9:49 AM Adrian Prantl via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> aprantl added a comment.
>>
>> Besides my above comment, I'm very much looking forward to this commit.
>> It will also help a lot with implementing dwarfdump -verify functionality.
>>
>>
>>
>> ================
>> 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());
>> ----------------
>> probinson wrote:
>> > 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.
>> This is something I'm still getting confused about. Doesn't
>> `llvm_unreachable()` give the compiler the license to optimize away
>> everything in the path leading up to the unreachable?
>
>
> Yep.
>
>
>> Would `report_fatal_error()` be more appropriate here?
>>
>
> Not if it's a programmer error, imho. That's what asserts/unreachable are
> for - but a correct program should never hit these and so it doesn't make
> sense to have an error path here (any error path should have a test case -
> if it's untestable because only incorrect code would reach it, then it
> should be an assertion).
>
> It's a bit of an awkward thing to explain well, I'm sorry - happy to try
> more, maybe over IRC, etc.
>
> - Dave
>
>
>>
>>
>> https://reviews.llvm.org/D30785
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170420/c47cb049/attachment.html>