[clang] 4a0267e - Convert a reachable llvm_unreachable into an assert.

[David Blaikie via cfe-commits]

On Sun, Mar 22, 2020 at 6:34 AM Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Sat, Mar 21, 2020 at 11:31 PM David Blaikie <dblaikie at 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 at 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 at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200322/3ee96f04/attachment-0001.html>