[llvm] r226588 - Add an assertion and prefer a crash over an infinite loop.

[Duncan P. N. Exon Smith]

> On 2015 Jan 20, at 11:52, Adrian Prantl <aprantl at apple.com> wrote:
> 
>> 
>> On Jan 20, 2015, at 11:46 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>> 
>> 
>>> On Jan 20, 2015, at 11:10 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>> 
>>> 
>>> 
>>> On Tue, Jan 20, 2015 at 10:58 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>> 
>>> 
>>> > Begin forwarded message:
>>> >
>>> > Subject: Re: [llvm] r226588 - Add an assertion and prefer a crash over an infinite loop.
>>> > From: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
>>> > Date: January 20, 2015 at 10:53:38 AM PST
>>> > Cc: llvm-commits at cs.uiuc.edu
>>> > To: Adrian Prantl <aprantl at apple.com>
>>> >
>>> >
>>> >> On 2015 Jan 20, at 10:03, Adrian Prantl <aprantl at apple.com> wrote:
>>> >>
>>> >> Author: adrian
>>> >> Date: Tue Jan 20 12:03:37 2015
>>> >> New Revision: 226588
>>> >>
>>> >> URL: http://llvm.org/viewvc/llvm-project?rev=226588&view=rev
>>> >> Log:
>>> >> Add an assertion and prefer a crash over an infinite loop.
>>> >>
>>> >> Modified:
>>> >>   llvm/trunk/lib/IR/DebugInfo.cpp
>>> >>
>>> >> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
>>> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=226588&r1=226587&r2=226588&view=diff
>>> >> ==============================================================================
>>> >> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
>>> >> +++ llvm/trunk/lib/IR/DebugInfo.cpp Tue Jan 20 12:03:37 2015
>>> >> @@ -527,12 +527,15 @@ bool DISubprogram::Verify() const {
>>> >>        while ((IA = DL.getInlinedAt()))
>>> >>          DL = DebugLoc::getFromDILocation(IA);
>>> >>        DL.getScopeAndInlinedAt(Scope, IA);
>>> >> +        assert(Scope && "debug location has no scope");
>>> >>        assert(!IA);
>>> >>        while (!DIDescriptor(Scope).isSubprogram()) {
>>> >>          DILexicalBlockFile D(Scope);
>>> >>          Scope = D.isLexicalBlockFile()
>>> >>                      ? D.getScope()
>>> >>                      : DebugLoc::getFromDILexicalBlock(Scope).getScope();
>>> >> +          if (!Scope)
>>> >> +            llvm_unreachable("lexical block file has no scope");
>>> >
>>> > It's strange that this isn't simply an assertion:
>>> >
>>> >    assert(Scope && "lexical block file has no scope");
>>> >
>>> > But I guess that's so that it crashes even when NDEBUG?  (Is that
>>> > reliable?)
>>> 
>>> Yes, that’s the intention (and llvm_unreachable is meant to be used that way).
>>> 
>>> I don't believe that's the intention of the LLVM style guide - generally we try to remove conditionals like this and replace the conditional with the assertion so the dead code isn't exercised in release builds.
>> 
>> Isn’t the point of “unreachable” to let the compiler do whatever it wants in this case?
>> I thought LLVM was free to just drop the test entirely in release mode?
>> 
> 
> After going back to ErrorHandling.h it looks as if report_fatal_error() would be more appropriate.
> llvm_unreachable() either becomes llvm_unreachable_internal() which invokes abort() and prints the message or __builtin_unreachable() which indeed drops the message.
> 
> -- adrian
> 

I'm liking the `return false` version more as I think about it.

We shouldn't crash or fatal error on syntactically valid IR; instead,
the verifier should fail (without crashing).

But `report_fatal_error()` is fine too if you think I'm missing
something.  Either one seems testable.

>> Mehdi
>> 
>> 
>>>  
>>> >
>>> > If that's the reason, you should probably add a comment to that
>>> > effect since otherwise I'm likely to switch it to an assert next
>>> > time I look at it ;).
>>> >
>>> > (Also, do you know when this situation arises?)
>>> 
>>> Happened to me when I compiled an old .ll file without the MDLocation upgrade.
>>> 
>>> -- adrian
>>> 
>>> >
>>> >>        }
>>> >>        if (!DISubprogram(Scope).describes(F))
>>> >>          return false;
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> llvm-commits mailing list
>>> >> llvm-commits at cs.uiuc.edu
>>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> >
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits