Hi Dave, Aaron, and I spent a lot more time debugging than I should have because I was using > a release + asserts build and the semantics of llvm_unreachable made > unfortunate codegen (switching to an assert makes the issue > immediately obvious).
Huh. I think we should be using llvm_unreachable here, but I would have expected it to behave similarly to assert(false && ...) in +Asserts builds. If it isn't that might be a bug? -- Lang. On Sun, Mar 22, 2020 at 9:19 AM David Blaikie <dblai...@gmail.com> wrote: > On Sun, Mar 22, 2020 at 6:34 AM Aaron Ballman <aa...@aaronballman.com> > wrote: > >> On Sat, Mar 21, 2020 at 11:31 PM David Blaikie <dblai...@gmail.com> >> wrote: >> > >> > Why the change? this seems counter to LLVM's style which pretty >> consistently uses unreachable rather than assert(false), so far as I know? >> >> We're not super consistent (at least within Clang), but the rules as >> I've generally understood them are to use llvm_unreachable only for >> truly unreachable code and to use assert(false) when the code is >> technically reachable but is a programmer mistake to have gotten >> there. > > > I don't see those as two different things personally - llvm_unreachable is > used when the programmer believes it to be unreachable (not that it must be > proven to be unreachable - we have message text there so it's informative > if the assumption turns out not to hold) > > >> In this particular case, the code is very much reachable > > > In what sense? If it is actually reachable - shouldn't it be tested? (& in > which case the assert(false) will get in the way of that testing) > > >> and I >> spent a lot more time debugging than I should have because I was using >> a release + asserts build and the semantics of llvm_unreachable made >> unfortunate codegen (switching to an assert makes the issue >> immediately obvious). >> > > I think it might be reasonable to say that release+asserts to have > unreachable behave the same way unreachable behaves at -O0 (which is to > say, much like assert(false)). Clearly release+asserts effects code > generation, so there's nothing like the "debug info invariance" goal that > this would be tainting, etc. > > Certainly opinions vary here, but here are some commits that show the sort > of general preference: > http://llvm.org/viewvc/llvm-project?view=revision&revision=259302 > http://llvm.org/viewvc/llvm-project?view=revision&revision=253005 > http://llvm.org/viewvc/llvm-project?view=revision&revision=251266 > > And some counter examples: > http://llvm.org/viewvc/llvm-project?view=revision&revision=225043 > Including this thread where Chandler originally (not sure what his take on > it is these days) expressed some ideas more along your lines: > > http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110919/thread.html#46583 > > But I'm always pretty concerned about the idea that assertions should be > used in places where the behavior of the program has any constraints when > the assertion is false... > > - Dave > > >> >> > >> > On Tue, Mar 10, 2020 at 11:22 AM Aaron Ballman via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >> >> >> >> >> Author: Aaron Ballman >> >> Date: 2020-03-10T14:22:21-04:00 >> >> New Revision: 4a0267e3ad8c4d47f267d7d960f127e099fb4818 >> >> >> >> URL: >> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818 >> >> DIFF: >> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818.diff >> >> >> >> LOG: Convert a reachable llvm_unreachable into an assert. >> >> >> >> Added: >> >> >> >> >> >> Modified: >> >> clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp >> >> >> >> Removed: >> >> >> >> >> >> >> >> >> ################################################################################ >> >> diff --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp >> b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp >> >> index 01ac2bc83bb6..99e16752b51a 100644 >> >> --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp >> >> +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp >> >> @@ -134,9 +134,9 @@ StringRef >> AnalyzerOptions::getCheckerStringOption(StringRef CheckerName, >> >> CheckerName = CheckerName.substr(0, Pos); >> >> } while (!CheckerName.empty() && SearchInParents); >> >> >> >> - llvm_unreachable("Unknown checker option! Did you call >> getChecker*Option " >> >> - "with incorrect parameters? User input must've >> been " >> >> - "verified by CheckerRegistry."); >> >> + assert(false && "Unknown checker option! Did you call >> getChecker*Option " >> >> + "with incorrect parameters? User input must've been >> " >> >> + "verified by CheckerRegistry."); >> >> >> >> return ""; >> >> } >> >> >> >> >> >> >> >> _______________________________________________ >> >> cfe-commits mailing list >> >> cfe-commits@lists.llvm.org >> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits