<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 20, 2015 at 11:13 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015 Jan 20, at 11:10, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Jan 20, 2015 at 10:58 AM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
><br>
><br>
> > Begin forwarded message:<br>
> ><br>
> > Subject: Re: [llvm] r226588 - Add an assertion and prefer a crash over an infinite loop.<br>
> > From: "Duncan P. N. Exon Smith" <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>><br>
> > Date: January 20, 2015 at 10:53:38 AM PST<br>
> > Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > To: Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>><br>
> ><br>
> ><br>
> >> On 2015 Jan 20, at 10:03, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
> >><br>
> >> Author: adrian<br>
> >> Date: Tue Jan 20 12:03:37 2015<br>
> >> New Revision: 226588<br>
> >><br>
> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=226588&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=226588&view=rev</a><br>
> >> Log:<br>
> >> Add an assertion and prefer a crash over an infinite loop.<br>
> >><br>
> >> Modified:<br>
> >> llvm/trunk/lib/IR/DebugInfo.cpp<br>
> >><br>
> >> Modified: llvm/trunk/lib/IR/DebugInfo.cpp<br>
> >> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=226588&r1=226587&r2=226588&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=226588&r1=226587&r2=226588&view=diff</a><br>
> >> ==============================================================================<br>
> >> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)<br>
> >> +++ llvm/trunk/lib/IR/DebugInfo.cpp Tue Jan 20 12:03:37 2015<br>
> >> @@ -527,12 +527,15 @@ bool DISubprogram::Verify() const {<br>
> >> while ((IA = DL.getInlinedAt()))<br>
> >> DL = DebugLoc::getFromDILocation(IA);<br>
> >> DL.getScopeAndInlinedAt(Scope, IA);<br>
> >> + assert(Scope && "debug location has no scope");<br>
> >> assert(!IA);<br>
> >> while (!DIDescriptor(Scope).isSubprogram()) {<br>
> >> DILexicalBlockFile D(Scope);<br>
> >> Scope = D.isLexicalBlockFile()<br>
> >> ? D.getScope()<br>
> >> : DebugLoc::getFromDILexicalBlock(Scope).getScope();<br>
> >> + if (!Scope)<br>
> >> + llvm_unreachable("lexical block file has no scope");<br>
> ><br>
> > It's strange that this isn't simply an assertion:<br>
> ><br>
> > assert(Scope && "lexical block file has no scope");<br>
> ><br>
> > But I guess that's so that it crashes even when NDEBUG? (Is that<br>
> > reliable?)<br>
><br>
> Yes, that’s the intention (and llvm_unreachable is meant to be used that way).<br>
><br>
> 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.<br>
<br>
</div></div>I don't mind an exception here myself (as long as there's a comment). In<br>
release builds this code is only exercised in the verifier (I think?), and<br>
I don't think it makes sense to infinite loop there.<br>
<br>
Although another option is:<br>
<br>
if (!Scope)<br>
return false;<br>
<br>
(That's certainly a valid answer.)<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> ><br>
> > If that's the reason, you should probably add a comment to that<br>
> > effect since otherwise I'm likely to switch it to an assert next<br>
> > time I look at it ;).<br>
> ><br>
> > (Also, do you know when this situation arises?)<br>
><br>
> Happened to me when I compiled an old .ll file without the MDLocation upgrade.<br>
><br>
> -- adrian<br>
><br>
> ><br>
> >> }<br>
> >> if (!DISubprogram(Scope).describes(F))<br>
> >> return false;<br>
> >><br>
> >><br>
> >> _______________________________________________<br>
> >> llvm-commits mailing list<br>
> >> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>