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

[David Blaikie]

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.


>
> >
> > >
> > > 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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150120/a38c7734/attachment.html>