Thank you for the discussion on IRC about this topic. For reference, this generated https://reviews.llvm.org/D76721 to clarify the documentation. I've also reverted the change in this thread in commit 7339fca25facb566e969b6ce01f23ac96499d574, so we're back to llvm_unreachable in this case.
~Aaron On Mon, Mar 23, 2020 at 6:36 PM David Blaikie <dblai...@gmail.com> wrote: > > > > On Mon, Mar 23, 2020 at 5:24 AM Aaron Ballman <aa...@aaronballman.com> wrote: >> >> On Sun, Mar 22, 2020 at 3:31 PM David Blaikie <dblai...@gmail.com> wrote: >> > >> > On Sun, Mar 22, 2020 at 9:34 AM Aaron Ballman <aa...@aaronballman.com> >> > wrote: >> >> >> >> On Sun, Mar 22, 2020 at 12:19 PM 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) >> >> >> >> 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). >> > >> > >> > That doesn't seem to be what it's documented to do: >> > >> > /// Marks that the current location is not supposed to be reachable. >> > /// In !NDEBUG builds, prints the message and location info to stderr. >> > /// In NDEBUG builds, becomes an optimizer hint that the current location >> > /// is not supposed to be reachable. On compilers that don't support >> > /// such hints, prints a reduced message instead. >> > >> > & certainly I think the documentation is what it /should/ be doing. >> > >> > /maybe/ >> > https://reviews.llvm.org/rG5f4535b974e973d52797945fbf80f19ffba8c4ad broke >> > that contract on Windows, but I'm not sure how? (an unreachable at the end >> > of that function shouldn't cause the whole function to be unreachable - >> > because abort could have side effects and halt the program before the >> > unreachable is reached) >> >> Agreed. It could also be that my machine is in a weird state (I'm >> currently battling a situation where the command line parser appears >> to be totally broken on Windows due to misuse of a ManagedStatic >> somewhere but I've not seen any commits that relate to the issues). >> >> >> >> 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. >> > >> > >> > Ah, OK - my approach is generally that programmer errors are programmer >> > errors, whether the mistake is in LLVM code or in code using LLVM and in >> > all cases asserts and unreachable express an intent about the invariants >> > of the code - ie: any violation of them represents a bug where the fix is >> > changing the code (either LLVM code or client code). >> > >> > I think in both cases (LLVM internal developers and LLVM external/client >> > developers) we should do what we can to make those failures actionable >> > with an asserts build & I think unreachable is at least /intended/ to >> > provide that functionality when an intended-to-be-unreachable path is >> > mistakenly reached for any reason. >> >> Let's ignore the behavioral issues, which we're agreed should behave >> consistently with assert(false) in a release+asserts build. If there >> are lingering issues here, I can look into fixing them. >> >> What I think we need to clarify in our public documentation is whether >> reachable code should be marked with llvm_unreachable or not, because >> it's not clear from the docs or the API itself. My personal position >> is: do not use llvm_unreachable on code that is possible to reach >> through typical program control flow. e.g., if there's a valid way to >> execute the tool such that you hit that code path (so the control flow >> path is not a bug) > > > I'm not sure I understand the "valid way to execute the tool" - if it's code > bug to execute this way, I would define that as "invalid" & if it fails an > assertion I'd say that's "invalid". > > Much like if user code passed in a null pointer - we could assert it's > non-null, but we'd continue on to dereference it, so it's not valid to pass > in non-null values as it sounds like it's not valid to getChecker*Option that > doesn't meet the CheckerRegistry's validation? That's part of the contract of > this function, by the looks of it (by the presence of this assertion) > >> >> then llvm_unreachable is the wrong tool to reach >> for because the name causes confusion (the name implies "ignore this, >> the code cannot matter" but the code can still be executed so it has >> security implications, etc). > > > Not sure I follow - there should be no code in an unreachable branch, because > it can/will be optimized away - you can't put security back-stop code in an > unreachable block, it won't be preserved. > >> >> If the community consensus is that we do >> want to use llvm_unreachable in this sort of case, then I think we >> should rename llvm_unreachable to something more clear, like >> llvm_bug_if_reached (name can be bikeshed). > > > I'm open to renaming it, though I think that'll be a bigger discussion on > llvm-dev and I'm not sure. > > Though I'm trying to understand - what did the original llvm_unreachable mean > to you that was different from what llvm_bug_if_reached would've communicated > to you in that context? > >> >> Either way, I think we >> should clarify the developer docs to make an explicit statement about >> this. WDYT? > > > Likely, yeah - I'd be happy to make it more explicit/clear about what these > are for. (could cover things like "don't write "if (X) unreachable", instead > assert(!X)" which comes up sometimes too) > > - Dave > >> >> >> ~Aaron >> >> >> > >> > - Dave >> > >> >> >> >> >> >> ~Aaron >> >> >> >> > >> >> > - 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
