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

[Lang Hames via cfe-commits]

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 <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)
>
>
>> 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/cc8cc7c9/attachment.html>