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

[Adrian Prantl via llvm-commits]

> On 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 <mailto: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).

Could you perhaps paste this paragraph into CodingStandards.rst? Might be useful to have this as a reference when this question unavoidably comes up again in the future.

-- adrian

> 
> 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 <https://reviews.llvm.org/D30785>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170420/972f14f2/attachment.html>