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

[Aaron Ballman via cfe-commits]

On Sun, Mar 22, 2020 at 12:19 PM David Blaikie <dblaikie at gmail.com> wrote:
>
> 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)

The message text doesn't help when the code is reached in release
mode, which was the issue. Asserts + release you still get some
helpful text saying "you screwed up". llvm_unreachable in release
mode, you may get lucky or you may not (in this case, I didn't get
lucky -- there was no text, just a crash).

>> 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)

In the sense that normal code paths reach that code easily. Basically,
that code is checking to see whether a plugin you've written properly
sets up its options or not. When you're developing a plugin, it's
quite reasonable to expect you won't get it just right on the first
try, so you hit the code path but only as a result of you not writing
the plugin quite right. So under normal conditions (once the plugin is
working), the code path should not be reached but under development
the code path gets reached accidentally.

>> 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...

I'm opposed to using unreachability hints on control flow paths you
expect to reach -- the semantics are just plain wrong, even if you can
get the same end result of controlled crash + message. In this
particular case, the code is reachable but erroneous to do so -- and
that's what assertions are intended to be used for. My preference is
to use llvm_unreachable because the code is unreachable, not because
the code should not be reachable only if everything works out right.

It may be that we're not in agreement about the definition of "expects
to reach" here. To me, this code clearly expects to reach that path:
it's a search over a finite collection where it's possible the thing
being searched for is not found. The "we didn't find the thing we were
expecting to find" assertion is reasonable because this isn't the
result of end-user error (then we'd fire a diagnostic instead) but is
the result of a plugin author's error. If the collection and the input
to the search were both fully under control of the analyzer (so the
search cannot fail without exceptional circumstances), then I think
llvm_unreachable could be reasonable.

~Aaron

>
> - 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