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

[Duncan P. N. Exon Smith]

> On 2015 Jan 20, at 11:28, Adrian Prantl <aprantl at apple.com> wrote:
> 
> 
>> On Jan 20, 2015, at 11:18 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> 
>> 
>> On Tue, Jan 20, 2015 at 11:13 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> > On 2015 Jan 20, at 11:10, 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.
>> 
>> I don't mind an exception here myself (as long as there's a comment).  In
>> release builds this code is only exercised in the verifier (I think?), and
>> I don't think it makes sense to infinite loop there.
>> 
>> Although another option is:
>> 
>>     if (!Scope)
>>       return false;
>> 
>> (That's certainly a valid answer.)
>> 
>> If that's the code, I'd expect a test case to exercise it (not sure how we are on testing the verifier) - and if such a test case exists then it certainly shouldn't be llvm_unreachable now.
> 
> I agree that returning false is better than llvm_unreachable here, but I have no idea how to test this in any meaningful way. Running an .ll file through opt will then result in it _silently_ dropping the malformed MDNode, but that is not a desirable behavior either.

The `false` answer might make this easy.  Turn on the debug-info
verifier, expect a failure, and check for the verification error.

    RUN: not llvm-as -disable-output -verify-debug-info < %s 2>&1 | FileCheck %s
    CHECK: error: ...