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

[David Blaikie via llvm-commits]

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/f2153cc9/attachment.html>