[clang] replace `assert(0)` with `llvm_unreachable` NFC

[inclyc]

Summary

The comment where llvm_unreachable says

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, if the platform does not support a builtin unreachable
then we call an internal LLVM runtime function. Otherwise the behavior is
controlled by the CMake flag
  -DLLVM_UNREACHABLE_OPTIMIZE
* When "ON" (default) llvm_unreachable() becomes an optimizer hint
  that the current location is not supposed to be reachable: the hint
  turns such code path into undefined behavior.  On compilers that don't
  support such hints, prints a reduced message instead and aborts the
  program.
* When "OFF", a builtin_trap is emitted instead of an
  optimizer hint or printing a reduced message.

Use this instead of assert(0). It conveys intent more clearly, suppresses
diagnostics for unreachable code paths, and allows compilers to omit
unnecessary code.

We have discussions on the discourse here: https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587/8

Link: https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125
Link: https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587/8

[inclyc]

TODO: Fix CI

[inclyc]

Fix CI issue

[aaron.ballman]

I don't know if that discussion reached a conclusion to move forward with this change -- my reading of the conversation was that efforts would be better spend on fuzzing instead of changing policy about using unreachable vs assert(0).

In general, I'm a bit hesitant to make this change. On the one hand, it's logically no worse than using assert(0) in a release build (if you hit this code path, bad things are going to happen). But __builtin_unreachable can have time travel optimization effects that assert doesn't have, and so the kind of bad things which can happen are different between the two (and use of unreachable on reachable code paths might make for harder debugging in RelWithDebInfo builds). Historically, we've usually used llvm_unreachable for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); } and we've used assert(0) for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on." The two situations are similar, but still different enough that I don't think we should wholesale change from one form to another.

[inclyc]

In D135551#3846592, @aaron.ballman wrote:

I don't know if that discussion reached a conclusion to move forward with this change -- my reading of the conversation was that efforts would be better spend on fuzzing instead of changing policy about using unreachable vs assert(0).

In general, I'm a bit hesitant to make this change. On the one hand, it's logically no worse than using assert(0) in a release build (if you hit this code path, bad things are going to happen). But __builtin_unreachable can have time travel optimization effects that assert doesn't have, and so the kind of bad things which can happen are different between the two (and use of unreachable on reachable code paths might make for harder debugging in RelWithDebInfo builds). Historically, we've usually used llvm_unreachable for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); } and we've used assert(0) for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on." The two situations are similar, but still different enough that I don't think we should wholesale change from one form to another.

but still different enough that I don't think we should wholesale change from one form to another.

In general we can control the behavior here via -DLLVM_UNREACHABLE_OPTIMIZE to choose making assumptions or traps (looks better than assertions to me).

https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125

(This change was landed 7 months ago https://reviews.llvm.org/D121750)

[aaron.ballman]

In D135551#3846603, @inclyc wrote:

In D135551#3846592, @aaron.ballman wrote:

I don't know if that discussion reached a conclusion to move forward with this change -- my reading of the conversation was that efforts would be better spend on fuzzing instead of changing policy about using unreachable vs assert(0).

In general, I'm a bit hesitant to make this change. On the one hand, it's logically no worse than using assert(0) in a release build (if you hit this code path, bad things are going to happen). But __builtin_unreachable can have time travel optimization effects that assert doesn't have, and so the kind of bad things which can happen are different between the two (and use of unreachable on reachable code paths might make for harder debugging in RelWithDebInfo builds). Historically, we've usually used llvm_unreachable for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); } and we've used assert(0) for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on." The two situations are similar, but still different enough that I don't think we should wholesale change from one form to another.

but still different enough that I don't think we should wholesale change from one form to another.

In general we can control the behavior here via -DLLVM_UNREACHABLE_OPTIMIZE to choose making assumptions or traps (looks better than assertions to me).

https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125

(This change was landed 7 months ago https://reviews.llvm.org/D121750)

That doesn't change the underlying concern that assert(0) and unreachable are used for different purposes and trying to unify those use cases might lose some expressivity in the code base.

I've left some comments in the review about examples of my concerns (it's not an exhaustive review).

clang/tools/libclang/CIndex.cpp

5191 ↗

(On Diff #466471)

This one is a bit questionable -- this is part of the C interface we expose, which is ABI stable, so the assert was alerting users to potential mismatches between versions of the library.

clang/tools/libclang/CXCursor.cpp

1490 ↗

(On Diff #466471)

Each of these is actually reachable -- the asserts exist specifically to tell users of the C interface about problems with their assumptions. In each of these cases, the assert is avoiding the need for a local variable to assert on.

clang/utils/TableGen/ClangSyntaxEmitter.cpp

120 ↗

(On Diff #466471)

This should not be using unreachable -- the code is very much reachable. This should have changed from assert to PrintFatalError.

[inclyc]

Address comments

[inclyc]

I've left some comments in the review about examples of my concerns (it's not an exhaustive review).

Thanks @aaron.ballman ! I didn't quite understand the original meaning of this code here (e.g. libclang), and I have now removed the relevant changes. I think this patch should replace the code that accidentally misuses of assert(0) with llvm_unreachable().

[dblaikie]

I thought this was settled quite a while ago and enshrined in the style guide: https://llvm.org/docs/CodingStandards.html#assert-liberally

assert(0) should not be used if something is reachable. We shouldn't have a "this violates an invariant, but if you don't have asserts enabled you do get some maybe-acceptable behavior".

I feel fairly strongly that any cases of "reachable asserts" should be changed to valid error handling or llvm_report_error and remaining assert(0) should be transformed to llvm_unreachable. (also, ideally, don't use branch-to-unreachable where possible, instead assert the condition - in cases where the if has side effects, sometimes that's the nicest way to write it, but might be clearer, if more verbose to use a local variable for the condition, then assert that the variable is true (and have the requisite "variable might be unused" cast))

Historically, we've usually used llvm_unreachable for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); } and we've used assert(0) for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on."

I don't think those are different things though - violating invariants is ~= something going seriously wrong.

(sorry, I guess I should debate this on the thread instead of here - but I think most of the folks on that thread did agree with the LLVM style guide/the direction here)

This, I think was also discussed about a decade ago in the LLVM community and resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which specifically "Suggests llvm_unreachable over assert(0)" and is the policy of LLVM - this change is consistent with that policy.

[dblaikie]

Can't seem to find an llvm-dev/commits discussion for r166821, but I remember discussing it several times before, perhaps this one happened on IRC and so we may not have any record of it.

[aaron.ballman]

In D135551#3847444, @dblaikie wrote:

I thought this was settled quite a while ago and enshrined in the style guide: https://llvm.org/docs/CodingStandards.html#assert-liberally

assert(0) should not be used if something is reachable. We shouldn't have a "this violates an invariant, but if you don't have asserts enabled you do get some maybe-acceptable behavior".

I feel fairly strongly that any cases of "reachable asserts" should be changed to valid error handling or llvm_report_error and remaining assert(0) should be transformed to llvm_unreachable. (also, ideally, don't use branch-to-unreachable where possible, instead assert the condition - in cases where the if has side effects, sometimes that's the nicest way to write it, but might be clearer, if more verbose to use a local variable for the condition, then assert that the variable is true (and have the requisite "variable might be unused" cast))

I would be okay with that, but that's not what this patch was doing -- it was changing assert(0) into an llvm_unreachable more mechanically, and I don't think that's a valid transformation. The key, to me, is not losing the distinction between "reaching here is a programming mistake that you'd make during active development" vs "we never expect to reach this patch and want to optimize accordingly." __builtin_unreachable changes the debugging landscape far too much for me to want to see folks using it for "reaching here is a programming mistake" situations, *especially* in RelWithDebInfo builds where optimizations are enabled and may result in surprising call stacks and time travel debugging.

Historically, we've usually used llvm_unreachable for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); } and we've used assert(0) for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on."

I don't think those are different things though - violating invariants is ~= something going seriously wrong.

(sorry, I guess I should debate this on the thread instead of here - but I think most of the folks on that thread did agree with the LLVM style guide/the direction here)

This, I think was also discussed about a decade ago in the LLVM community and resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which specifically "Suggests llvm_unreachable over assert(0)" and is the policy of LLVM - this change is consistent with that policy.

I don't have the context for those changes in my email either, but regardless of what we thought ten years ago, we have code in the code base today that assumes a difference in severity between kinds of unreachable statements so we need to be careful when correcting mistakes. I think we're still in agreement that llvm_unreachable should be preferred over assert(0) in situations where the code is expected to be impossible to reach. I think we're also still in agreement that "correct error reporting" is preferred over assert(0). Where we might still have daylight are the occasional situations where assert(0) gives a better experience -- when the code is possible to reach but reaching it signifies a developer (not user) mistake that is plausible to make when doing new development, such as when misusing the C interface (where there isn't always an error that should be reported via the API). I think these uses should generally be rare, but I don't think it's a "never do this" kind of situation.

llvm_unreachable asserts in debug mode, so it has the nice property we need when doing new development. But it doesn't convey the distinction in semantics. Maybe another approach is: #define developer_bugcheck(msg) llvm_unreachable(msg) (or something along those lines). We still get the assertion in debug mode, we then get the better optimization in release mode, but we don't lose the semantic information in the code base. It doesn't really help the RelWithDebInfo case though (where call stacks may be unrecognizable as a result of time travel optimizations) but maybe that's a tradeoff worth making?

[dexonsmith]

I would think we could convert every assert(0) to either llvm::report_fatal_error (guaranteed trap) or llvm_unreachable() (trap or optimize, depending on CMAKE configuration). The C API usage checks seem like good candidates for the former.

Also, not sure if everyone noticed, but the latter can now be configured to always trap by turning off the “optimize” CMAKE flag. This seems useful for fuzzing situations where you may not want asserts builds.

[dblaikie]

In D135551#3847607, @aaron.ballman wrote:

In D135551#3847444, @dblaikie wrote:

I thought this was settled quite a while ago and enshrined in the style guide: https://llvm.org/docs/CodingStandards.html#assert-liberally

assert(0) should not be used if something is reachable. We shouldn't have a "this violates an invariant, but if you don't have asserts enabled you do get some maybe-acceptable behavior".

I feel fairly strongly that any cases of "reachable asserts" should be changed to valid error handling or llvm_report_error and remaining assert(0) should be transformed to llvm_unreachable. (also, ideally, don't use branch-to-unreachable where possible, instead assert the condition - in cases where the if has side effects, sometimes that's the nicest way to write it, but might be clearer, if more verbose to use a local variable for the condition, then assert that the variable is true (and have the requisite "variable might be unused" cast))

I would be okay with that, but that's not what this patch was doing -- it was changing assert(0) into an llvm_unreachable more mechanically, and I don't think that's a valid transformation. The key, to me, is not losing the distinction between "reaching here is a programming mistake that you'd make during active development" vs "we never expect to reach this patch and want to optimize accordingly."

I don't really think those are different things, though. Violating invariants is UB and there's no discussion to be had about how the program (in a non-asserts build) behaves when those invariants are violated - all bets are off, whether it's assert or unreachable.

__builtin_unreachable changes the debugging landscape far too much for me to want to see folks using it for "reaching here is a programming mistake" situations, *especially* in RelWithDebInfo builds where optimizations are enabled and may result in surprising call stacks and time travel debugging.

Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? (that's the first thing any of us do is reproduce with an assertions build that may fail miles away from where a crash occurred because an invariant was violated much earlier) - that cast won't crash/will continue on happily in a non-asserts build seems like a much larger hole to debuggability of a non-asserts build than any unreachable?

Historically, we've usually used llvm_unreachable for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); } and we've used assert(0) for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on."

I don't think those are different things though - violating invariants is ~= something going seriously wrong.

(sorry, I guess I should debate this on the thread instead of here - but I think most of the folks on that thread did agree with the LLVM style guide/the direction here)

This, I think was also discussed about a decade ago in the LLVM community and resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which specifically "Suggests llvm_unreachable over assert(0)" and is the policy of LLVM - this change is consistent with that policy.

I don't have the context for those changes in my email either, but regardless of what we thought ten years ago, we have code in the code base today that assumes a difference in severity between kinds of unreachable statements so we need to be careful when correcting mistakes. I think we're still in agreement that llvm_unreachable should be preferred over assert(0) in situations where the code is expected to be impossible to reach. I think we're also still in agreement that "correct error reporting" is preferred over assert(0). Where we might still have daylight are the occasional situations where assert(0) gives a better experience -- when the code is possible to reach but reaching it signifies a developer (not user) mistake that is plausible to make when doing new development, such as when misusing the C interface (where there isn't always an error that should be reported via the API). I think these uses should generally be rare, but I don't think it's a "never do this" kind of situation.

That still seems like something that should be caught by an asserts build and is UB otherwise. If you're developing against LLVM's APIs in a non-assertions build, there's a lot of other invariants that won't be checked (cast being a pretty core example) and will make debugging really difficult.

llvm_unreachable asserts in debug mode, so it has the nice property we need when doing new development. But it doesn't convey the distinction in semantics. Maybe another approach is: #define developer_bugcheck(msg) llvm_unreachable(msg) (or something along those lines). We still get the assertion in debug mode, we then get the better optimization in release mode, but we don't lose the semantic information in the code base. It doesn't really help the RelWithDebInfo case though (where call stacks may be unrecognizable as a result of time travel optimizations) but maybe that's a tradeoff worth making?

I don't really see the distinction, though - it's a violated invariant, which is a developer bug, whether it's from an assert on unreachable. They're both expressing invariants - not of different strength. "Valid code should never reach this branch" and "valid code should never produce a false result here".

I'd really like to avoid what I see as cleanup - replacing one construct (assert(false)) with another (llvm_unreachable) of identical (to me) contractual strength - being slowed because of this distinction. Replacing reachable-unreachables, or reachable-false-assertions with llvm_report_error, to me, is orthogonal to this cleanup.

(& for the C API - I think assertions/unreachable are the right thing, not llvm_report_error - I'm sure there's many more ways to violate the C API contract that would result in inexplicable/confusing behavior and that will only ever be protected by assertions, than we might cover by a few llvm_report_errors on the interface & that should be the direction we encourage people to go down - develop with assertions enabled)

Basically llvm_report_error ends up/should be a "we couldn't find a better way to do error handling here, but it is really a reachable error we have to do something with" - each one is a bug in the library-ness of LLVM. (or it's used up in a tool implementation (above the library layer) and then it's fine/just a convenient way to error/exit, though probably still isn't so good for diagnostic quality from such tools). If it's not reachable by a correct usage (be it from a command line tool or API) then it should be an assertion, not an llvm_report_error.

But I know this issue comes up from time to time and I've yet to figure out a way to come to shared understandings on it :/ so I don't expect my rather narrow and firm definition to necessarily carry the day on this.

[dblaikie]

Because I was looking, some other assert(0) -> llvm_unreachable cleanups (though, yes, even the earliest cleanups include some assert(0) -> report_fatal_error, but for externally/user-reachable failures, like invalid bitcode, I think). Some of these are more blanket/wide reaching than others, for sure. (& no doubt the naive search through the commit log doesn't find all the cleanups)

I /think/ when I was looking I did find one from 2017 that Richard Smith approved that included llvm_unreachable in the Clang C API, in terms of more recent precedent for some of this... but can't find that again right now.

rGabd1561f15e:[LLDBAssert] Use unreachable instead of assert(0)
rGf9da10ce4544ea66fe6fad5b943d3700d192a1e1:Change to assert(0,x) to llvm_unreachable(x)
rG7a247f709baa2cd19111af9e18965df4e419949a:Turn effective assert(0) into llvm_unreachable
rG35b2f75733c98e5904c5a75f8bcedeb96c4f4eda:Convert some assert(0) to llvm_unreachable or fold an 'if' condition into the assert.
rGd8d43191d8afe2c4b5b2d3be62cd73ddc3ddc6c9:Replace some assert(0)'s with llvm_unreachable.
rG2a30d7889fc54c8a74d73b79be3dd030bac41b06:Replace some assert(0)'s with llvm_unreachable.
rG0039f3f0607702f2d16d60addff74c67869e2144:Replace some assert(0)'s with llvm_unreachable.
rGc7193c48d9d7e44e9fd0c39205e8b7cfbf5d5458:Convert assert(0) to llvm_unreachable to silence a warning about Addend being uninitialized in default case.
rG7b7a67c5c8daa051e42837dc3e5e65adab9cf09c:[ARM64] Fix 'assert("...")' to be 'assert(0 && "...")'. Otherwise, it is no assert at all. ;] Some of these should probably be switched to llvm_unreachable, but I didn't want to perturb the behavior in this patch.
rGeaa3a7efab65d0a65ddda7fdb7e5fbdbd5f897ad:Use llvm_unreachable instead of assert(0)
rGd7fd95a5c1eb19e5754281b540f09e86ced1b9d4:Change assert(0 && "text") to llvm_unreachable(0 && "text")
rG2962d9599e463265edae599285bbc6351f1cc0ef:Suggest llvm_unreachable over assert(0).
rG2e007de42de48dc05bdb7aa9d3c9e8902ee720fe:Revert "Target/AMDGPU/AMDILIntrinsicInfo.cpp: Use llvm_unreachable() in nonreturn function, instead of assert(0)."
rGdc4261794fdbf2e3001f369d8b8bbd77eb923602:Target/AMDGPU/AMDILIntrinsicInfo.cpp: Use llvm_unreachable() in nonreturn function, instead of assert(0).
rG751eb3d2b30d90069b1797a31f8e489f0763c585:use llvm_unreachable() instead of assert(0) for invalid enum values in switch statements
rGbdf39a46a302df7cb07e948a6780265b3335fbda:Convert assert(0) to llvm_unreachable.
rGeb455832b4c7cb1046ab9fb9f6f8ca40202874a4:Silence various build warnings from Hexagon backend that show up in release builds. Mostly converting 'assert(0)' to 'llvm_unreachable' to silence warnings about missing returns. Also fold some variable declarations into asserts to prevent the variables from being unused in release builds.
rGabd1561f15ee466c0fd9abeede2cdcde2ebb2cec:[LLDBAssert] Use unreachable instead of assert(0)
rGf9da10ce4544ea66fe6fad5b943d3700d192a1e1:Change to assert(0,x) to llvm_unreachable(x)
rG7a247f709baa2cd19111af9e18965df4e419949a:Turn effective assert(0) into llvm_unreachable
rG35b2f75733c98e5904c5a75f8bcedeb96c4f4eda:Convert some assert(0) to llvm_unreachable or fold an 'if' condition into the assert.
rGd8d43191d8afe2c4b5b2d3be62cd73ddc3ddc6c9:Replace some assert(0)'s with llvm_unreachable.
rG2a30d7889fc54c8a74d73b79be3dd030bac41b06:Replace some assert(0)'s with llvm_unreachable.
rG0039f3f0607702f2d16d60addff74c67869e2144:Replace some assert(0)'s with llvm_unreachable.
rGc7193c48d9d7e44e9fd0c39205e8b7cfbf5d5458:Convert assert(0) to llvm_unreachable to silence a warning about Addend being uninitialized in default case.
rG7b7a67c5c8daa051e42837dc3e5e65adab9cf09c:[ARM64] Fix 'assert("...")' to be 'assert(0 && "...")'. Otherwise, it is no assert at all. ;] Some of these should probably be switched to llvm_unreachable, but I didn't want to perturb the behavior in this patch.
rGeaa3a7efab65d0a65ddda7fdb7e5fbdbd5f897ad:Use llvm_unreachable instead of assert(0)
rGd7fd95a5c1eb19e5754281b540f09e86ced1b9d4:Change assert(0 && "text") to llvm_unreachable(0 && "text")
rG2962d9599e463265edae599285bbc6351f1cc0ef:Suggest llvm_unreachable over assert(0).
rG2e007de42de48dc05bdb7aa9d3c9e8902ee720fe:Revert "Target/AMDGPU/AMDILIntrinsicInfo.cpp: Use llvm_unreachable() in nonreturn function, instead of assert(0)."
rGdc4261794fdbf2e3001f369d8b8bbd77eb923602:Target/AMDGPU/AMDILIntrinsicInfo.cpp: Use llvm_unreachable() in nonreturn function, instead of assert(0).
rG751eb3d2b30d90069b1797a31f8e489f0763c585:use llvm_unreachable() instead of assert(0) for invalid enum values in switch statements
rGbdf39a46a302df7cb07e948a6780265b3335fbda:Convert assert(0) to llvm_unreachable.
rGeb455832b4c7cb1046ab9fb9f6f8ca40202874a4:Silence various build warnings from Hexagon backend that show up in release builds. Mostly converting 'assert(0)' to 'llvm_unreachable' to silence warnings about missing returns. Also fold some variable declarations into asserts to prevent the variables from being unused in release builds.
rG8619c37b5b8795fcc722373f5fd9a5d0c07195af:Replace assert(0) with llvm_unreachable to avoid warnings about dropping off the end of a non-void function in Release builds.
rGa2886c21d9a08d63c324cc61aa91ae0893507a31:Convert assert(0) to llvm_unreachable
rGe55c556a247a9c0decb4e256d9e897dfc9cf841d:Convert assert(0) to llvm_unreachable
rGc514b5474a3feb6b4b2775b75c4f8eaf0676a9d0:Convert assert(0) to llvm_unreachable
rGee4dab5f1f7a0c32167b8b91c5733e77d4d88dcc:Convert assert(0) to llvm_unreachable
rGc4965bce14249ad13bd532af827bdd5c21b340fd:Convert assert(0) to llvm_unreachable
rG4ed7278ff4ef206d01867fda76bf90df36398c4c:Convert assert(0) to llvm_unreachable in X86 Target directory.
rG83f3bdaa457e7ac28fdda724808be8fd4f1d275d:Convert some assert(0) in default of switch statements to llvm_unreachable.
rG83d382b1cad133cb163a68dd7149fae2802275e1:Switch assert(0/false) llvm_unreachable.
rGea431722972fa481bbe2898d180678a08f977fa0:Prefer llvm_unreachable to assert(0)
rG1ab40bef8dfe416cc82455eea39d01d496752d90:After converting assert(0) to LLVM_UNREACHABLE we lost file/line location. Fix by making the LLVM_UNREACHABLE pass FILE and LINE to llvm_unreachable.
rG56d065972602c45a4109617f32eb8605e5017c5e:assert(0) -> LLVM_UNREACHABLE. Make llvm_unreachable take an optional string, thus moving the cerr<< out of line. LLVM_UNREACHABLE is now a simple wrapper that makes the message go away for NDEBUG builds.
rG6dd2730024c20647b92ddfbb80e9a8bf33308ccd:Start converting to new error handling API. cerr+abort -> llvm_report_error assert(0)+abort -> LLVM_UNREACHABLE (assert(0)+llvm_unreachable-> abort() included)

[aaron.ballman]

In D135551#3847962, @dblaikie wrote:

In D135551#3847607, @aaron.ballman wrote:

In D135551#3847444, @dblaikie wrote:

I thought this was settled quite a while ago and enshrined in the style guide: https://llvm.org/docs/CodingStandards.html#assert-liberally

assert(0) should not be used if something is reachable. We shouldn't have a "this violates an invariant, but if you don't have asserts enabled you do get some maybe-acceptable behavior".

I feel fairly strongly that any cases of "reachable asserts" should be changed to valid error handling or llvm_report_error and remaining assert(0) should be transformed to llvm_unreachable. (also, ideally, don't use branch-to-unreachable where possible, instead assert the condition - in cases where the if has side effects, sometimes that's the nicest way to write it, but might be clearer, if more verbose to use a local variable for the condition, then assert that the variable is true (and have the requisite "variable might be unused" cast))

I would be okay with that, but that's not what this patch was doing -- it was changing assert(0) into an llvm_unreachable more mechanically, and I don't think that's a valid transformation. The key, to me, is not losing the distinction between "reaching here is a programming mistake that you'd make during active development" vs "we never expect to reach this patch and want to optimize accordingly."

I don't really think those are different things, though. Violating invariants is UB and there's no discussion to be had about how the program (in a non-asserts build) behaves when those invariants are violated - all bets are off, whether it's assert or unreachable.

I think they're the same thing in terms of runtime behavior, but I feel (rather strongly) that they're different in terms of documentation when reading the source. This code pattern exists and keeps coming up year after year, which is sufficient to inform me that the community thinks there is *some* distinction to be made there. Also, the fact that we have report_fatal_error *and* unreachable APIs signals that we already understand there's a distinction between "reaching here will never happen; optimize accordingly" and "reaching here is a surprising mistake".

__builtin_unreachable changes the debugging landscape far too much for me to want to see folks using it for "reaching here is a programming mistake" situations, *especially* in RelWithDebInfo builds where optimizations are enabled and may result in surprising call stacks and time travel debugging.

Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? (that's the first thing any of us do is reproduce with an assertions build that may fail miles away from where a crash occurred because an invariant was violated much earlier) - that cast won't crash/will continue on happily in a non-asserts build seems like a much larger hole to debuggability of a non-asserts build than any unreachable?

This might be true -- personally, I tend to only use debug builds with MSVC because RelWithDebInfo isn't sufficient for my daily needs. However, I've definitely heard of folks who use RelWithDebInfo for their daily work (RelWithDebInfo + Asserts specifically, IIRC) because of the improved build times and runtime performance; we should be sure we're not disrupting that workflow too much.

Historically, we've usually used llvm_unreachable for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); } and we've used assert(0) for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on."

I don't think those are different things though - violating invariants is ~= something going seriously wrong.

(sorry, I guess I should debate this on the thread instead of here - but I think most of the folks on that thread did agree with the LLVM style guide/the direction here)

This, I think was also discussed about a decade ago in the LLVM community and resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which specifically "Suggests llvm_unreachable over assert(0)" and is the policy of LLVM - this change is consistent with that policy.

I don't have the context for those changes in my email either, but regardless of what we thought ten years ago, we have code in the code base today that assumes a difference in severity between kinds of unreachable statements so we need to be careful when correcting mistakes. I think we're still in agreement that llvm_unreachable should be preferred over assert(0) in situations where the code is expected to be impossible to reach. I think we're also still in agreement that "correct error reporting" is preferred over assert(0). Where we might still have daylight are the occasional situations where assert(0) gives a better experience -- when the code is possible to reach but reaching it signifies a developer (not user) mistake that is plausible to make when doing new development, such as when misusing the C interface (where there isn't always an error that should be reported via the API). I think these uses should generally be rare, but I don't think it's a "never do this" kind of situation.

That still seems like something that should be caught by an asserts build and is UB otherwise. If you're developing against LLVM's APIs in a non-assertions build, there's a lot of other invariants that won't be checked (cast being a pretty core example) and will make debugging really difficult.

llvm_unreachable asserts in debug mode, so it has the nice property we need when doing new development. But it doesn't convey the distinction in semantics. Maybe another approach is: #define developer_bugcheck(msg) llvm_unreachable(msg) (or something along those lines). We still get the assertion in debug mode, we then get the better optimization in release mode, but we don't lose the semantic information in the code base. It doesn't really help the RelWithDebInfo case though (where call stacks may be unrecognizable as a result of time travel optimizations) but maybe that's a tradeoff worth making?

I don't really see the distinction, though - it's a violated invariant, which is a developer bug, whether it's from an assert on unreachable. They're both expressing invariants - not of different strength. "Valid code should never reach this branch" and "valid code should never produce a false result here".

There are different kinds of developer bugs and it's reasonable for people to want to express them differently, IMO. Concretely with a contrived example:

enum E { Zero, One, Two };

int func(E Val) {
  switch (Val) {
  case Zero: return 12;
  case One: return 200;
  case Two: return -1;
  }
  llvm_unreachable("never get here");
}

This is a case where the code is technically reachable (someone can call func((E)3)) but the programmer's intent it "nobody will ever do that" because it's an internal API, not exposed to C, other safeguards like diagnostics ensure it, or whatever. Contrast this with:

enum E { Zero, One, Two };

extern "C" int func(unsigned Val) {
  switch (Val) {
  case Zero: return 12;
  case One: return 200;
  case Two: return -1;
  }
  assert(0 && "never get here");
}

Same general situation, but now the safeguards are no longer in place. The function is externally callable by anyone who wants to use it, the interface has lost the connection to the enumeration, etc. So the code is also technically reachable, but the author wants to signal the difference in plausibility of reaching the erroneous state.

I definitely agree that the end results of reaching the unreachable bits are the same in terms of what happens at runtime. But I'm worried about maintaining the code -- losing the distinction between the two situations makes the person debugging the failure have to figure out whether someone missed adding a safeguard elsewhere vs something more difficult to solve such as a miscompile from the host implementation, stack stomping, etc.

I don't care whether we spell it assert(0) or report_fatal_error() or something else. I care that we don't have a policy requiring folks to not make a distinction between the two scenarios aside from leaving comments in the code as I think that's a step backwards.

I'd really like to avoid what I see as cleanup - replacing one construct (assert(false)) with another (llvm_unreachable) of identical (to me) contractual strength - being slowed because of this distinction. Replacing reachable-unreachables, or reachable-false-assertions with llvm_report_error, to me, is orthogonal to this cleanup.

Understood. I'm pushing back on this being a cleanup in all cases.

(& for the C API - I think assertions/unreachable are the right thing, not llvm_report_error - I'm sure there's many more ways to violate the C API contract that would result in inexplicable/confusing behavior and that will only ever be protected by assertions, than we might cover by a few llvm_report_errors on the interface & that should be the direction we encourage people to go down - develop with assertions enabled)

Basically llvm_report_error ends up/should be a "we couldn't find a better way to do error handling here, but it is really a reachable error we have to do something with" - each one is a bug in the library-ness of LLVM. (or it's used up in a tool implementation (above the library layer) and then it's fine/just a convenient way to error/exit, though probably still isn't so good for diagnostic quality from such tools). If it's not reachable by a correct usage (be it from a command line tool or API) then it should be an assertion, not an llvm_report_error.

But I know this issue comes up from time to time and I've yet to figure out a way to come to shared understandings on it :/ so I don't expect my rather narrow and firm definition to necessarily carry the day on this.

Concrete guidance around reachability that I would be happy with is:

• Prefer a user-facing diagnostic in any circumstance that's under the user's direct control (through command line options, source code, etc); users should not get failed assertions/crashes as a form of error reporting.
• Prefer assert(condition) in any circumstance under which there is a condition to be checked to validate the state of the system. The only use of a literal in such an assertion should be a string literal to use as a message. e.g., no assert(0);
• Prefer llvm_report_error() in any circumstance under which a code path is functionally possible to reach, but only in erroneous executions that signify a mistake on the part of the LLVM developer elsewhere in the program.
• Prefer llvm_unreachable() in any circumstance under which a code path is believed to be functionally impossible to reach (even if technically possible to reach). The API is now self-documenting to mean "this code really should be totally unreachable".

Something is functionally possible to reach when there are missing safeguards elsewhere that should have prevented calling the function in that state. Something is functionally impossible to reach when miscompiles, undefined behavior, or malicious users modifying memory with a debugger (etc) are the realistic ways to reach that state. When in doubt as to whether something is functionally possible to reach or not, use your best judgement but prefer llvm_report_error on the assumption that most reachability mistakes are LLVM developer errors rather than "extenuating circumstance" kind of situations.

That said, I have no idea if I'm being too pedantic here. I'm basing this off my own experiences with the code base as well as internal discussions with other folks at Intel working on the project and what their expectations are (both as new-to-the-code-base folks and long-time contributors), but this is a pretty small selection of people.

[dblaikie]

Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? (that's the first thing any of us do is reproduce with an assertions build that may fail miles away from where a crash occurred because an invariant was violated much earlier) - that cast won't crash/will continue on happily in a non-asserts build seems like a much larger hole to debuggability of a non-asserts build than any unreachable?

This might be true -- personally, I tend to only use debug builds with MSVC because RelWithDebInfo isn't sufficient for my daily needs. However, I've definitely heard of folks who use RelWithDebInfo for their daily work (RelWithDebInfo + Asserts specifically, IIRC) because of the improved build times and runtime performance; we should be sure we're not disrupting that workflow too much.

Changing assert(0) to llvm_unreachable does not change the behavior of any +Asserts build. The behavior of unreachable is the same as assert(0) in a +Asserts build.

Is this observation enough to undeadlock this conversation?

[aaron.ballman]

In D135551#3849816, @dblaikie wrote:

Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? (that's the first thing any of us do is reproduce with an assertions build that may fail miles away from where a crash occurred because an invariant was violated much earlier) - that cast won't crash/will continue on happily in a non-asserts build seems like a much larger hole to debuggability of a non-asserts build than any unreachable?

This might be true -- personally, I tend to only use debug builds with MSVC because RelWithDebInfo isn't sufficient for my daily needs. However, I've definitely heard of folks who use RelWithDebInfo for their daily work (RelWithDebInfo + Asserts specifically, IIRC) because of the improved build times and runtime performance; we should be sure we're not disrupting that workflow too much.

Changing assert(0) to llvm_unreachable does not change the behavior of any +Asserts build. The behavior of unreachable is the same as assert(0) in a +Asserts build.

Is this observation enough to undeadlock this conversation?

No, it doesn't address the key point I have which is that I want different APIs to express intent instead of using llvm_unreachable in all cases. To me, it is a mistake to label functionally reachable code as being unconditionally unreachable. It requires everyone reading that code to understand our nuances to know that the API signals aspirationally unreachable code rather than functionally unreachable code. These are different scenarios and I don't want to lose that distinction in the places where it matters.

[inclyc]

• Prefer llvm_report_error() in any circumstance under which a code path is functionally possible to reach, but only in erroneous executions that signify a mistake on the part of the LLVM developer elsewhere in the program.
• Prefer llvm_unreachable() in any circumstance under which a code path is believed to be functionally impossible to reach (even if technically possible to reach). The API is now self-documenting to mean "this code really should be totally unreachable".

I think llvm_unreachable already has the functionality reporting bugs for developer in our implementation, with +Assertions by default

[aaron.ballman]

In D135551#3849944, @inclyc wrote:

• Prefer llvm_report_error() in any circumstance under which a code path is functionally possible to reach, but only in erroneous executions that signify a mistake on the part of the LLVM developer elsewhere in the program.
• Prefer llvm_unreachable() in any circumstance under which a code path is believed to be functionally impossible to reach (even if technically possible to reach). The API is now self-documenting to mean "this code really should be totally unreachable".

I think llvm_unreachable already has the functionality reporting bugs for developer in our implementation, with +Assertions by default

Yes, in terms of its runtime behavior. So long as we're not making debugging harder for some large group of people, the runtime behavior is not really what I'm concerned by though. I'm focusing more on code reviewers and project newcomers and whether our code is self-documenting or not. Having a policy to use an API that says code is not reachable in situations where that code is very much reachable is the crux of my problem -- the API is sometimes a lie (and a lie with optimization impacts, at that) and we force everyone to pay the cognitive costs associated with that when reading code.

If we end up with two APIs that have the same runtime behavior, I'm okay with that.

[inclyc]

In D135551#3849983, @aaron.ballman wrote:

In D135551#3849944, @inclyc wrote:

• Prefer llvm_report_error() in any circumstance under which a code path is functionally possible to reach, but only in erroneous executions that signify a mistake on the part of the LLVM developer elsewhere in the program.
• Prefer llvm_unreachable() in any circumstance under which a code path is believed to be functionally impossible to reach (even if technically possible to reach). The API is now self-documenting to mean "this code really should be totally unreachable".

I think llvm_unreachable already has the functionality reporting bugs for developer in our implementation, with +Assertions by default

Yes, in terms of its runtime behavior. So long as we're not making debugging harder for some large group of people, the runtime behavior is not really what I'm concerned by though. I'm focusing more on code reviewers and project newcomers and whether our code is self-documenting or not. Having a policy to use an API that says code is not reachable in situations where that code is very much reachable is the crux of my problem -- the API is sometimes a lie (and a lie with optimization impacts, at that) and we force everyone to pay the cognitive costs associated with that when reading code.

If we end up with two APIs that have the same runtime behavior, I'm okay with that.

Could you elaborate on "aspirationally" vs "functionally" unreachable code here?

[aaron.ballman]

In D135551#3850132, @inclyc wrote:

In D135551#3849983, @aaron.ballman wrote:

In D135551#3849944, @inclyc wrote:

• Prefer llvm_report_error() in any circumstance under which a code path is functionally possible to reach, but only in erroneous executions that signify a mistake on the part of the LLVM developer elsewhere in the program.
• Prefer llvm_unreachable() in any circumstance under which a code path is believed to be functionally impossible to reach (even if technically possible to reach). The API is now self-documenting to mean "this code really should be totally unreachable".

I think llvm_unreachable already has the functionality reporting bugs for developer in our implementation, with +Assertions by default

Yes, in terms of its runtime behavior. So long as we're not making debugging harder for some large group of people, the runtime behavior is not really what I'm concerned by though. I'm focusing more on code reviewers and project newcomers and whether our code is self-documenting or not. Having a policy to use an API that says code is not reachable in situations where that code is very much reachable is the crux of my problem -- the API is sometimes a lie (and a lie with optimization impacts, at that) and we force everyone to pay the cognitive costs associated with that when reading code.

If we end up with two APIs that have the same runtime behavior, I'm okay with that.

Could you elaborate on "aspirationally" vs "functionally" unreachable code here?

Sure!

enum E { Zero, One, Two };

int func(E Val) {
  switch (Val) {
  case Zero: return 12;
  case One: return 200;
  case Two: return -1;
  }
  llvm_unreachable("never get here"); // Functionally unreachable; we can't think of a reasonable way to get here without other alarms going off
}

vs

enum E { Zero, One, Two };

extern "C" int func(unsigned Val) {
  switch (Val) {
  case Zero: return 12;
  case One: return 200;
  case Two: return -1;
  }
  assert(0 && "never get here"); // Aspirationally unreachable; we HOPE we don't get here but it's plausible that we do if someone made a logical mistake calling the API
}

[inclyc]

This makes sense! However I think assert(0) should not be used in this case, we could expose another llvm_unreachable-like api and probably llvm_report_error shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

[dblaikie]

In D135551#3850266, @inclyc wrote:

This makes sense! However I think assert(0) should not be used in this case, we could expose another llvm_unreachable-like api and probably llvm_report_error shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

No, I really don't think we should go down that path.

I believe these are not actually distinct cases - in either case, the program has UB if they violated the invariants/preconditions - whether or not they called through the C API.

unreachable is no more a guarantee/proven thing than an assertion - both are written by humans and a claim "if this is reached-or-false, there is a bug in some code, somewhere". The statement is not stronger in the unreachable case and the style guide supports that perspective and the way we triage/treat bugs is pretty consistent with that - we get bugs all the time when an unreachable is reached and that doesn't seem to surprise most/anyone - we treat it the same as a bug when an assertion fires.

The discourse discussion, I think, supports this ^ perspective.

As there's still disagreement, should this escalate to the RFC process to change the style guide, Aaron?

[aaron.ballman]

In D135551#3850308, @dblaikie wrote:

In D135551#3850266, @inclyc wrote:

This makes sense! However I think assert(0) should not be used in this case, we could expose another llvm_unreachable-like api and probably llvm_report_error shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

No, I really don't think we should go down that path.

I believe these are not actually distinct cases - in either case, the program has UB if they violated the invariants/preconditions - whether or not they called through the C API.

The C Index test cases I commented on earlier in the review are a good example of when there's no UB but we still want to alert people to the problem of code they should not be reaching. The assumption that "reached here unexpectedly" == "UB" is invalid. Some things are logic bugs that exhibit no UB.

unreachable is no more a guarantee/proven thing than an assertion - both are written by humans and a claim "if this is reached-or-false, there is a bug in some code, somewhere". The statement is not stronger in the unreachable case and the style guide supports that perspective and the way we triage/treat bugs is pretty consistent with that - we get bugs all the time when an unreachable is reached and that doesn't seem to surprise most/anyone - we treat it the same as a bug when an assertion fires.

The discourse discussion, I think, supports this ^ perspective.

As there's still disagreement, should this escalate to the RFC process to change the style guide, Aaron?

Yes, I would appreciate that. I don't think we're interpreting our policy the same way. Specifically "Use llvm_unreachable to mark a specific point in code that should never be reached." -- "should" is turning out to be interpreted in two ways:

• "used to indicate obligation, duty, or correctness, typically when criticizing someone's actions. e.g., he should have been careful": I am asserting it is impossible to reach this.
• "used to indicate what is probable. e.g., $348 million should be enough to buy him out": I am asserting you probably won't get here, but you won't be happy if you do.

In D135551#3850266, @inclyc wrote:

This makes sense! However I think assert(0) should not be used in this case, we could expose another llvm_unreachable-like api and probably llvm_report_error shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

I'm totally fine not using assert(0) and using an llvm_unreachable-like API (or even using a macro to dispatch to llvm_unreachable under a different name).

There are more aspirationally unreachable issues in this patch, I've commented on the ones I spotted, but I stopped commenting pretty quickly because I think a lot of the cases are made slightly worse by switching to llvm_unreachable instead of more targeted changes. I'd be especially curious to hear what @dblaikie thinks of the suggestions I have though -- it might be easier to see the distinction with real world code (or it might not!).

clang/include/clang/AST/Redeclarable.h

265

This looks like it should probably be: assert(!PassedFirst && "Passed first decl twice, invalid redecl chain!"); rather than an if statement with recovery mechanisms.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

601

This looks like it possibly should have been an error reported to the user? If not, this is a wonderful example of what I mean by aspirationally unreachable code. We aspire to not getting here -- but we can get here just the same and there is not UB as a result (we return a valid object, UB might happen elsewhere based on invalid assumptions of what is returned).

clang/lib/AST/ASTImporter.cpp

9976

According to our style guide, this probably should have been assert(isa<ObjCInterfaceDecl, ObjCProtocolDecl, TagDecl>(D) && "CompleteDecl called on a Decl that can't be completed"); but IMO that's worse than using assert(0) because it's less maintainable (any time you add a new else if chain you have to also update the assert). I think llvm_unreachable is wrong to use here.

clang/lib/AST/ExprConstant.cpp

7561–7564

Probably should be assert(Source != E && "OpaqueValueExpr recursively refers to itself");

clang/lib/AST/Interp/ByteCodeExprGen.cpp

136–137

I think this can just be removed, right? There's another unreachable for falling out of the switch that seems to be covering the same situation.

598

The rest of the ones here are somewhat interesting in that the interpreter is an experiment under active development and is known to be incomplete. In all of these cases, I think the switch to unreachable is flat-out wrong -- these asserts serve explicitly to find unimplemented cases when we hit them.

clang/lib/Basic/SourceManager.cpp

62–65

assert(Buffer != nullptr && "Buffer should never be null");

but that said, this one might be an optimization hint that suggests we should be using __builtin_assume(Buffer != nullptr), I'm not certain.

863–866

assert(SLocOffset >= CurrentLoadedOffset && "Invalid SLocOffset or bad function choice");

[dblaikie]

In D135551#3850391, @aaron.ballman wrote:

In D135551#3850308, @dblaikie wrote:

In D135551#3850266, @inclyc wrote:

This makes sense! However I think assert(0) should not be used in this case, we could expose another llvm_unreachable-like api and probably llvm_report_error shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

No, I really don't think we should go down that path.

I believe these are not actually distinct cases - in either case, the program has UB if they violated the invariants/preconditions - whether or not they called through the C API.

The C Index test cases I commented on earlier in the review are a good example of when there's no UB but we still want to alert people to the problem of code they should not be reaching. The assumption that "reached here unexpectedly" == "UB" is invalid. Some things are logic bugs that exhibit no UB.

unreachable is no more a guarantee/proven thing than an assertion - both are written by humans and a claim "if this is reached-or-false, there is a bug in some code, somewhere". The statement is not stronger in the unreachable case and the style guide supports that perspective and the way we triage/treat bugs is pretty consistent with that - we get bugs all the time when an unreachable is reached and that doesn't seem to surprise most/anyone - we treat it the same as a bug when an assertion fires.

The discourse discussion, I think, supports this ^ perspective.

As there's still disagreement, should this escalate to the RFC process to change the style guide, Aaron?

Yes, I would appreciate that. I don't think we're interpreting our policy the same way. Specifically "Use llvm_unreachable to mark a specific point in code that should never be reached."

In the same way that an assert says "This condition should never be false" - I use "should" in the same sense in both unreachable and assert, and I believe that's the prevailing opinion of LLVM developers/the LLVM style guide.

Perhaps we are also at a deadlock as to who should write the proposal... :/

• "should" is turning out to be interpreted in two ways:

• "used to indicate obligation, duty, or correctness, typically when criticizing someone's actions. e.g., he should have been careful": I am asserting it is impossible to reach this.

This is the "should" ^ I mean, and what every assert should mean too. This code assumes this property to be true - this is a precondition of the code.

We should not be using asserts where we don't mean this. I'm OK assuming every assert does mean "this is a precondition" and treating them that way in terms of transforming them to unreachable or anything else we might do with them - and if some of them don't mean it, then they're buggy and we can fix them, but assert->unreachable doesn't make the situation any worse.

Any code behind/after an assert is untested and unvalidated - we can't say "if you violate this constraint it'll actually be OK" because we've never tested that/don't know that.

• "used to indicate what is probable. e.g., $348 million should be enough to buy him out": I am asserting you probably won't get here, but you won't be happy if you do.

In D135551#3850266, @inclyc wrote:

This makes sense! However I think assert(0) should not be used in this case, we could expose another llvm_unreachable-like api and probably llvm_report_error shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

I'm totally fine not using assert(0) and using an llvm_unreachable-like API (or even using a macro to dispatch to llvm_unreachable under a different name).

There are more aspirationally unreachable issues in this patch, I've commented on the ones I spotted, but I stopped commenting pretty quickly because I think a lot of the cases are made slightly worse by switching to llvm_unreachable instead of more targeted changes. I'd be especially curious to hear what @dblaikie thinks of the suggestions I have though -- it might be easier to see the distinction with real world code (or it might not!).

Maybe - happy to talk about a few of the examples, but I'm not feeling super optimistic that we'll come to an understanding here, unfortunately :/

@rnk's comment here ( https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587/3 ) pretty well sums up my understanding/values here and it looks like on that thread, mostly the long term LLVM developers agree with this perspective and are trying to explain it to the (so far as I can tell) relative new/outside developer.

clang/include/clang/AST/Redeclarable.h

265

Yep, I've commented on similar things in the past - not sure we ever got it into the style guide, but "branch to unreachable" should be avoided in favor of assert-the-branch-condition (assuming it has no side effects, or if it does have side effects - do the thing, store the result in a local variable, void cast it to suppress unused-variable warnings and assert it)

but I still think this is a valid transformation - it's just not the whole transformation, so I'm OK with things like this being done mechanically and then cleaned up further (possibly also mechanically) later.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

601

"we" can get here in what sense? Is there source code that can be passed to the static analyzer that can reach this? Then it shouldn't be an assert, it should be exercised and tested - even if that test says "hey, this doesn't do anything good yet". I'd assume, whether I see the assert or the unreachable that it isn't reachable from clang on the command line - and that any code that makes it reachable/calls it under this condition is incorrect (for now, until support is added) and would be considered a bug to be fixed. Whether it's the assert or the unreachable, I consider it to be the same statement of intent.

clang/lib/AST/ASTImporter.cpp

9976

Yep, this is a case where branch-to-unreachable might be nicer than contorting the situation into assert(isa...) - actually, probably not even that. I'd likely change this code to unconditionally cast, relying on the cast's assertion.

Why is llvm_unreachable wrong here? If you end up with a non-TagDecl here, you're certainly in UB territory - sooner or later, you're going to treat the non-TagDecl as a TagDecl and it's not going to be pretty.

clang/lib/AST/ExprConstant.cpp

7561–7564

Yep, though, again, I think it's a valid transformation even if it doesn't go as far as it could.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

136–137

Presumably the switch isn't fully covered (otherwise we'd get a -Wcovered-switch-default warning).

Might be that the unreachable after the switch can be removed in this case.

598

& I don't see why unreachable is any different a statement than assert(false) in these cases... - it's the same statement of intent. "if this is reached you've found a bug" (in this case, a missing feature)

But I'd be sort of OK changing all these to report_fatal_error. But, again, I think the assert(false) -> unreachable is a valid transformation and doesn't make anything worse than it already is, but improves the code by being more consistent and removing this confusion that there might be something different about assert(false) when, I believe, there isn't.

[aaron.ballman]

In D135551#3850511, @dblaikie wrote:

In D135551#3850391, @aaron.ballman wrote:

In D135551#3850308, @dblaikie wrote:

In D135551#3850266, @inclyc wrote:

This makes sense! However I think assert(0) should not be used in this case, we could expose another llvm_unreachable-like api and probably llvm_report_error shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

No, I really don't think we should go down that path.

I believe these are not actually distinct cases - in either case, the program has UB if they violated the invariants/preconditions - whether or not they called through the C API.

The C Index test cases I commented on earlier in the review are a good example of when there's no UB but we still want to alert people to the problem of code they should not be reaching. The assumption that "reached here unexpectedly" == "UB" is invalid. Some things are logic bugs that exhibit no UB.

unreachable is no more a guarantee/proven thing than an assertion - both are written by humans and a claim "if this is reached-or-false, there is a bug in some code, somewhere". The statement is not stronger in the unreachable case and the style guide supports that perspective and the way we triage/treat bugs is pretty consistent with that - we get bugs all the time when an unreachable is reached and that doesn't seem to surprise most/anyone - we treat it the same as a bug when an assertion fires.

The discourse discussion, I think, supports this ^ perspective.

As there's still disagreement, should this escalate to the RFC process to change the style guide, Aaron?

Yes, I would appreciate that. I don't think we're interpreting our policy the same way. Specifically "Use llvm_unreachable to mark a specific point in code that should never be reached."

In the same way that an assert says "This condition should never be false" - I use "should" in the same sense in both unreachable and assert, and I believe that's the prevailing opinion of LLVM developers/the LLVM style guide.

I believe our code base says something different than our "prevailing opinion" and we should not be discounting the reality of how our code is written today.

Perhaps we are also at a deadlock as to who should write the proposal... :/

Agreed, you and I are probably both too close to the issue to write the proposal right now. If nobody else does it first, maybe the two of us could circle back in a few months after we've had time to research and think more deeply and we could co-write something (even if it is a multiple choice RFC between conflicting directions).

• "should" is turning out to be interpreted in two ways:

• "used to indicate obligation, duty, or correctness, typically when criticizing someone's actions. e.g., he should have been careful": I am asserting it is impossible to reach this.

This is the "should" ^ I mean, and what every assert should mean too. This code assumes this property to be true - this is a precondition of the code.

We should not be using asserts where we don't mean this. I'm OK assuming every assert does mean "this is a precondition" and treating them that way in terms of transforming them to unreachable or anything else we might do with them - and if some of them don't mean it, then they're buggy and we can fix them, but assert->unreachable doesn't make the situation any worse.

Any code behind/after an assert is untested and unvalidated - we can't say "if you violate this constraint it'll actually be OK" because we've never tested that/don't know that.

Just to double-check... are you opposed to the idea of differentiating between ways of saying "the reachability of this code is in question" or are you opposed to use of assert (or something that smells too similar) specifically? Because I don't care about *how* we spell the differentiation, just that there's not a policy limiting my ability to express my intent in code. I'd be perfectly fine with #define some_name_we_agree_upon(msg) llvm_unreachable(msg) being the facility we use instead of assert(0);.

• "used to indicate what is probable. e.g., $348 million should be enough to buy him out": I am asserting you probably won't get here, but you won't be happy if you do.

I'm arguing that we have a not-insignificant amount of uses that have this^ interpretation and I want a way to express that distinction in code. I have an allergic reaction to using llvm_unreachable to express known-to-be-potentially reachable code because my complaint is that the annotation causes the code to be *less readable* as a result. I have to know about our community's novel definition of what "unreachable" means in order to read that code correctly. I think we'd be better served to leave "unreachable" annotations for places that we know we can't reach and use literally any other named API to express situations we know can potentially be reached (the assumption that we're going to have test coverage for all of these situations is a non-starter to me; until the community starts showing a stronger interest in tracking test coverage metrics and holding ourselves accountable for that coverage, it's not a compelling argument that "tests should catch this" because we know they won't).

@rnk's comment here ( https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587/3 ) pretty well sums up my understanding/values here and it looks like on that thread, mostly the long term LLVM developers agree with this perspective and are trying to explain it to the (so far as I can tell) relative new/outside developer.

I don't want to belabor this topic any longer as I think we've both said our pieces enough by now. I don't think we should make any code changes at this point unless there's agreement that the code is actually improved by the change. I think we can identify some of those noncontroversial cases in this review so that we get some benefit from all this effort. But I think we should leave anything that's contentious alone for the time being.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

601

"we" can get here in what sense? Is there source code that can be passed to the static analyzer that can reach this?

You are asking the question I claim the original source already answered and the new source fails to answer. Use of assert(false) like that tells me as a code reviewer "this code could potentially be reached and if it does, that's a problem" and so I know to go look for those cases to make sure we handle them properly, or if I've found a bug after the code was released I then have a better idea of what possible problems exist. Asserting "this is unreachable" tells me "the developer already did that audit and knows this is unreachable" and sends me down other, less productive paths before I finally go "oh, that annotation was lying to me, this isn't actually unreachable."

Then it shouldn't be an assert, it should be exercised and tested - even if that test says "hey, this doesn't do anything good yet".

I like that idea. In reality, we're nowhere near having test coverage like that (at least in Clang).

clang/lib/AST/ExprConstant.cpp

7561–7564

One thing that's worth noting -- the proposed code and my "probably" observation from above have the result of making the code *less* robust because they remove the perfectly reasonable recovery mechanism of saying there's an error. So they both go from "may give odd results but is unlikely to cause a vulnerability" to "vulnerability more possible" by losing that property.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

136–137

Fair -- so long as we only end up with one unreachable, I think that's an improvement.

598

& I don't see why unreachable is any different a statement than assert(false) in these cases... - it's the same statement of intent. "if this is reached you've found a bug" (in this case, a missing feature)

You are asserting it's the same statement of intent and I keep pointing out that people use the different constructs in practice because they're different statements of intent. I don't know how to resolve this difference of opinion, but I can say as someone doing code review in this area recently that your interpretation is wrong according to what we were after with this code.

I'd be fine changing it to report_fatal_error instead of assert(false); I'd be strongly opposed to switching to llvm_unreachable.

[rnk]

I think the disagreement here highlights the need to have a serious discussion about the future of error handling across the LLVM project. As you say, it sounds like you're not going to reach agreement on this code review, so maybe the best short term next step is to land the uncontroversial changes that Aaron agrees with.

Regarding error handling and the wide usage of assertions to guard UB across LLVM, we need to decide what our goals are as a community. Is it actually the goal of the project that Clang and LLVM that no input can lead to UB? If we can't guarantee that, is there some error budget we consider acceptable (fuzzer runs for 24hrs and can't find bugs)? How does that goal rate against our other goals, like performance? We could just ship with assertions enabled, sacrifice 20% code size and performance, and call it a day. We used to do that for Chromium, but users complained that compiles were too slow and we stopped doing it.

I think the status quo has real problems. We pretend that we can do both of these:

• Assert liberally, with the understanding that assertion failures lead to UB (failed bad cast check, bounds checks, unreachable code, etc)
• We can actually find and fix all cases that violate those inputs to the point that clang is stable and secure enough for our satisfaction

Currently, it is really easy to run fuzzers and find crash bugs in clang. I think the lesson we should take from that is that we are compromising goal 2 here, and we shouldn't kid ourselves about it.

Maybe the goal is not security, but is instead something about user or developer experience, but we should go through some higher level process to clarify that goal so we can write it down and agree on it.

clang/lib/Analysis/CFG.cpp

1047

This will create unreachable code warnings, which must be addressed before landing.

[arsenm]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

598

I use llvm_unreachable as a nicer to use assert in if/else chains like this. I also see no difference in the intent between assert and unreachable; assert(0 && "message") is just uglier.

report_fatal_error is for something a user could plausibly run into but also isn't worth wiring into a proper error diagnostic (which happens a lot in codegen)

[shafik]

In D135551#3853365, @rnk wrote:

I think the status quo has real problems. We pretend that we can do both of these:

• Assert liberally, with the understanding that assertion failures lead to UB (failed bad cast check, bounds checks, unreachable code, etc)
• We can actually find and fix all cases that violate those inputs to the point that clang is stable and secure enough for our satisfaction

Currently, it is really easy to run fuzzers and find crash bugs in clang. I think the lesson we should take from that is that we are compromising goal 2 here, and we shouldn't kid ourselves about it.

Maybe the goal is not security, but is instead something about user or developer experience, but we should go through some higher level process to clarify that goal so we can write it down and agree on it.

+1 to all of this

[probinson]

dblaikie:

In the same way that an assert says "This condition should never be false" - I use "should" in the same sense in both unreachable and assert, and I believe that's the prevailing opinion of LLVM developers/the LLVM style guide.

aaronballman:

I believe our code base says something different than our "prevailing opinion" and we should not be discounting the reality of how our code is written today.

So, there are lots of ways and places where "the reality of how our code is written today" fails to conform to the style guide.
One conclusion is that the style guide is wrong and should be changed to reflect reality.
Another conclusion is that conforming to the style guide is the goal and one aspect of development is to strive to reach the goal. The imperfection of the state of the code today merely reflects an imperfect understanding of the goal.

I suggest a Round Table at the (less than one month away) Dev Meeting, assuming that the active parties in this discussion will be there. Put off any RFC until after that.

clang/include/clang/AST/Redeclarable.h

public:
     pointer operator->() const { return Current; }
 
     redecl_iterator& operator++() {
       assert(Current && "Advancing while iterator has reached end");
       // Make sure we don't infinitely loop on an invalid redecl chain. This
       // should never happen.
       if (Current->isFirstDecl()) {
         if (PassedFirst) {
-          assert(0 && "Passed first decl twice, invalid redecl chain!");
+          llvm_unreachable("Passed first decl twice, invalid redecl chain!");
           Current = nullptr;
           return *this;
         }
         PassedFirst = true;
       }
 
       // Get either previous decl or latest decl.
       decl_type *Next = Current->getNextRedeclaration();

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

 public:
   QualType getLocationType() const override {
     const ASTContext &Ctx = getContext();
     if (const auto *D = dyn_cast<FunctionDecl>(FD)) {
       return Ctx.getPointerType(D->getType());
     }
 
     assert(isa<ObjCMethodDecl>(FD));
-    assert(false && "Getting the type of ObjCMethod is not supported yet");
+    llvm_unreachable("Getting the type of ObjCMethod is not supported yet");
 
     // TODO: We might want to return a different type here (ex: id (*ty)(...))
     //       depending on how it is used.
     return {};
   }
 
   const NamedDecl *getDecl() const {
     return FD;

clang/lib/AST/ASTImporter.cpp

void ASTImporter::CompleteDecl (Decl *D) {
   }
   else if (auto *TD = dyn_cast<TagDecl>(D)) {
     if (!TD->getDefinition() && !TD->isBeingDefined()) {
       TD->startDefinition();
       TD->setCompleteDefinition(true);
     }
   }
   else {
-    assert(0 && "CompleteDecl called on a Decl that can't be completed");
+    llvm_unreachable("CompleteDecl called on a Decl that can't be completed");
   }
 }
 
 Decl *ASTImporter::MapImported(Decl *From, Decl *To) {
   llvm::DenseMap<Decl *, Decl *>::iterator Pos = ImportedDecls.find(From);
   assert((Pos == ImportedDecls.end() || Pos->second == To) &&
       "Try to import an already imported Decl");
   if (Pos != ImportedDecls.end())

clang/lib/AST/ExprConstant.cpp

public:
 
   bool VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
     if (APValue *Value = Info.CurrentCall->getCurrentTemporary(E))
       return DerivedSuccess(*Value, E);
 
     const Expr *Source = E->getSourceExpr();
     if (!Source)
       return Error(E);
     if (Source == E) {
-      assert(0 && "OpaqueValueExpr recursively refers to itself");
+      llvm_unreachable("OpaqueValueExpr recursively refers to itself");
       return Error(E);
     }
     return StmtVisitorTy::Visit(Source);
   }
 
   bool VisitPseudoObjectExpr(const PseudoObjectExpr *E) {
     for (const Expr *SemE : E->semantics()) {
       if (auto *OVE = dyn_cast<OpaqueValueExpr>(SemE)) {
         // FIXME: We can't handle the case where an OpaqueValueExpr is also the
         // result expression: there could be two different LValues that would

clang/lib/AST/Interp/ByteCodeExprGen.cpp

if (!this->Visit(SubExpr))
       return false;
 
     return this->emitCast(*FromT, *ToT, CE);
   }
 
   case CK_ToVoid:
     return discard(SubExpr);
 
   default:
-    assert(false && "Cast not implemented");
+    llvm_unreachable("Cast not implemented");
   }
   llvm_unreachable("Unhandled clang::CastKind enum");
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitIntegerLiteral(const IntegerLiteral *LE) {
   if (DiscardResult)
     return true;
for (const Expr *Init : InitList->inits()) {
           return false;
       } else if (Optional<PrimType> T = classify(InitType)) {
         // Visit the primitive element like normal.
         if (!this->visit(Init))
           return false;
         if (!this->emitInitElem(*T, ElementIndex, Init))
           return false;
       } else {
-        assert(false && "Unhandled type in array initializer initlist");
+        llvm_unreachable("Unhandled type in array initializer initlist");
       }
 
       ++ElementIndex;
     }
 
   } else {
-    assert(false && "Unknown expression for array initialization");
+    llvm_unreachable("Unknown expression for array initialization");
   }
 
   return true;
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitInitializer(const Expr *Initializer) {
   QualType InitializerType = Initializer->getType();
if (T || ReturnType->isVoidType()) {
         if (!this->visit(Arg))
           return false;
       }
 
       if (T)
         return this->emitCall(*T, Func, E);
       return this->emitCallVoid(Func, E);
     } else {
-      assert(false && "Can't classify function return type");
+      llvm_unreachable("Can't classify function return type");
     }
 
   } else {
-    assert(false && "We don't support non-FunctionDecl callees right now.");
+    llvm_unreachable("We don't support non-FunctionDecl callees right now.");
   }
 
   return false;
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitCXXDefaultArgExpr(
     const CXXDefaultArgExpr *E) {
return dereference(
         [this, E](PrimType T) {
           return DiscardResult ? this->emitPop(T, E) : true;
         });
   case UO_Not:    // ~x
   case UO_Real:   // __real x
   case UO_Imag:   // __imag x
   case UO_Extension:
   case UO_Coawait:
-    assert(false && "Unhandled opcode");
+    llvm_unreachable("Unhandled opcode");
   }
 
   return false;
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitDeclRefExpr(const DeclRefExpr *E) {
   const auto *Decl = E->getDecl();

clang/lib/Analysis/CFG.cpp

if (const auto *UnOp = dyn_cast<UnaryOperator>(E->IgnoreParens())) {
           return Value;
         case UO_Minus:
           return -Value;
         case UO_Not:
           return ~Value;
         case UO_LNot:
           return llvm::APInt(Context->getTypeSize(Context->IntTy), !Value);
         default:
-          assert(false && "Unexpected unary operator!");
+          llvm_unreachable("Unexpected unary operator!");
           return llvm::None;
         }
       }
     } else if (const auto *IntLiteral =
                    dyn_cast<IntegerLiteral>(E->IgnoreParens()))
       return IntLiteral->getValue();
 
     return llvm::None;
   }

clang/lib/Basic/SourceManager.cpp

 /// ContentCache. This can be 0 if the MemBuffer was not actually expanded.
 unsigned ContentCache::getSizeBytesMapped() const {
   return Buffer ? Buffer->getBufferSize() : 0;
 }
 
 /// Returns the kind of memory used to back the memory buffer for
 /// this content cache.  This is used for performance analysis.
 llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
   if (Buffer == nullptr) {
-    assert(0 && "Buffer should never be null");
+    llvm_unreachable("Buffer should never be null");
     return llvm::MemoryBuffer::MemoryBuffer_Malloc;
   }
   return Buffer->getBufferKind();
 }
 
 /// getSize - Returns the size of the content encapsulated by this ContentCache.
 ///  This can be the size of the source file or the size of an arbitrary
 ///  scratch buffer.  If the ContentCache encapsulates a source file, that
 ///  file is not lazily brought in from disk to satisfy this query.
 unsigned ContentCache::getSize() const {
FileID SourceManager::getFileIDLocal(SourceLocation::UIntTy SLocOffset) const {
   }
 }
 
 /// Return the FileID for a SourceLocation with a high offset.
 ///
 /// This function knows that the SourceLocation is in a loaded buffer, not a
 /// local one.
 FileID SourceManager::getFileIDLoaded(SourceLocation::UIntTy SLocOffset) const {
   if (SLocOffset < CurrentLoadedOffset) {
-    assert(0 && "Invalid SLocOffset or bad function choice");
+    llvm_unreachable("Invalid SLocOffset or bad function choice");
     return FileID();
   }
 
   // Essentially the same as the local case, but the loaded array is sorted
   // in the other direction (decreasing order).
   // GreaterIndex is the one where the offset is greater, which is actually a
   // lower index!
   unsigned GreaterIndex = 0;
   unsigned LessIndex = LoadedSLocEntryTable.size();
   if (LastFileIDLookup.ID < 0) {
while (true) {
     ++NumProbes;
     unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
     const SrcMgr::SLocEntry &E = getLoadedSLocEntry(MiddleIndex, &Invalid);
     if (Invalid)
       return FileID(); // invalid entry.
 
     if (E.getOffset() > SLocOffset) {
       if (GreaterIndex == MiddleIndex) {
-        assert(0 && "binary search missed the entry");
+        llvm_unreachable("binary search missed the entry");
         return FileID();
       }
       GreaterIndex = MiddleIndex;
       continue;
     }
 
     if (isOffsetInFileID(FileID::get(-int(MiddleIndex) - 2), SLocOffset)) {
       FileID Res = FileID::get(-int(MiddleIndex) - 2);
       LastFileIDLookup = Res;
       NumBinaryProbes += NumProbes;
       return Res;
     }
 
     if (LessIndex == MiddleIndex) {
-      assert(0 && "binary search missed the entry");
+      llvm_unreachable("binary search missed the entry");
       return FileID();
     }
     LessIndex = MiddleIndex;
   }
 }
 
 SourceLocation SourceManager::
 getExpansionLocSlowCase(SourceLocation Loc) const {

clang/lib/Basic/Targets/NVPTX.cpp

std::string CUDAArchCode = [this] {
       case CudaArch::GFX1101:
       case CudaArch::GFX1102:
       case CudaArch::GFX1103:
       case CudaArch::Generic:
       case CudaArch::LAST:
         break;
       case CudaArch::UNUSED:
       case CudaArch::UNKNOWN:
-        assert(false && "No GPU arch when compiling CUDA device code.");
+        llvm_unreachable("No GPU arch when compiling CUDA device code.");
         return "";
       case CudaArch::SM_20:
         return "200";
       case CudaArch::SM_21:
         return "210";
       case CudaArch::SM_30:
         return "300";
       case CudaArch::SM_32:

clang/lib/CodeGen/CGHLSLRuntime.cpp

void CGHLSLRuntime::annotateHLSLResource(const VarDecl *D, GlobalVariable *GV) {
   uint32_t Counter = ResourceCounters[static_cast<uint32_t>(RC)]++;
 
   NamedMDNode *ResourceMD = nullptr;
   switch (RC) {
   case HLSLResourceAttr::ResourceClass::UAV:
     ResourceMD = CGM.getModule().getOrInsertNamedMetadata("hlsl.uavs");
     break;
   default:
-    assert(false && "Unsupported buffer type!");
+    llvm_unreachable("Unsupported buffer type!");
     return;
   }
 
   assert(ResourceMD != nullptr &&
          "ResourceMD must have been set by the switch above.");
 
   auto &Ctx = CGM.getModule().getContext();
   IRBuilder<> B(Ctx);

 llvm::Value *CGHLSLRuntime::emitInputSemantic(IRBuilder<> &B,
                                               const ParmVarDecl &D) {
   assert(D.hasAttrs() && "Entry parameter missing annotation attribute!");
   if (D.hasAttr<HLSLSV_GroupIndexAttr>()) {
     llvm::Function *DxGroupIndex =
         CGM.getIntrinsic(Intrinsic::dx_flattened_thread_id_in_group);
     return B.CreateCall(FunctionCallee(DxGroupIndex));
   }
-  assert(false && "Unhandled parameter attribute");
+  llvm_unreachable("Unhandled parameter attribute");
   return nullptr;
 }
 
 void CGHLSLRuntime::emitEntryFunction(const FunctionDecl *FD,
                                       llvm::Function *Fn) {
   llvm::Module &M = CGM.getModule();
   llvm::LLVMContext &Ctx = M.getContext();
   auto *EntryTy = llvm::FunctionType::get(llvm::Type::getVoidTy(Ctx), false);

clang/lib/CodeGen/CodeGenModule.cpp

if (FD->isTargetMultiVersion()) {
         if (Version.startswith("arch="))
           Architecture = Version.drop_front(sizeof("arch=") - 1);
         else if (Version != "default")
           Feature.push_back(Version);
 
         Options.emplace_back(cast<llvm::Function>(Func), Architecture, Feature);
       }
     } else {
-      assert(0 && "Expected a target or target_clones multiversion function");
+      llvm_unreachable(
+          "Expected a target or target_clones multiversion function");
       continue;
     }
 
     llvm::Constant *ResolverConstant = GetOrCreateMultiVersionResolver(GD);
     if (auto *IFunc = dyn_cast<llvm::GlobalIFunc>(ResolverConstant))
       ResolverConstant = IFunc->getResolver();
     llvm::Function *ResolverFunc = cast<llvm::Function>(ResolverConstant);
 

clang/lib/Driver/Multilib.cpp

bool MultilibSet::select(const Multilib::flags_list &Flags, Multilib &M) const {
   });
 
   if (Filtered[0].priority() > Filtered[1].priority()) {
     M = Filtered[0];
     return true;
   }
 
   // TODO: We should consider returning llvm::Error rather than aborting.
-  assert(false && "More than one multilib with the same priority");
+  llvm_unreachable("More than one multilib with the same priority");
   return false;
 }
 
 LLVM_DUMP_METHOD void MultilibSet::dump() const {
   print(llvm::errs());
 }
 
 void MultilibSet::print(raw_ostream &OS) const {

clang/lib/Driver/ToolChains/Flang.cpp

if (JA.getType() == types::TY_Nothing) {
                JA.getType() == types::TY_LTO_IR) {
       CmdArgs.push_back("-emit-llvm");
     } else if (JA.getType() == types::TY_LLVM_BC ||
                JA.getType() == types::TY_LTO_BC) {
       CmdArgs.push_back("-emit-llvm-bc");
     } else if (JA.getType() == types::TY_PP_Asm) {
       CmdArgs.push_back("-S");
     } else {
-      assert(false && "Unexpected output type!");
+      llvm_unreachable("Unexpected output type!");
     }
   } else if (isa<AssembleJobAction>(JA)) {
     CmdArgs.push_back("-emit-obj");
   } else {
-    assert(false && "Unexpected action class for Flang tool.");
+    llvm_unreachable("Unexpected action class for Flang tool.");
   }
 
   const InputInfo &Input = Inputs[0];
   types::ID InputType = Input.getType();
 
   // Add preprocessing options like -I, -D, etc. if we are using the
   // preprocessor (i.e. skip when dealing with e.g. binary files).
   if (types::getPreprocessedType(InputType) != types::TY_INVALID)

clang/lib/Format/Format.cpp

while (Idx < Tokens.size()) {
       auto SR = CharSourceRange::getCharRange(Tokens[St]->Tok.getLocation(),
                                               Tokens[End]->Tok.getEndLoc());
       auto Err =
           Fixes.add(tooling::Replacement(Env.getSourceManager(), SR, ""));
       // FIXME: better error handling. for now just print error message and skip
       // for the release version.
       if (Err) {
         llvm::errs() << llvm::toString(std::move(Err)) << "\n";
-        assert(false && "Fixes must not conflict!");
+        llvm_unreachable("Fixes must not conflict!");
       }
       Idx = End + 1;
     }
 
     return Fixes;
   }
 
   // Class for less-than inequality comparason for the set `RedundantTokens`.

clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp

bool RewriteModernObjC::RewriteObjCFieldDeclType(QualType &Type,
   else if (Type->isRecordType()) {
     RecordDecl *RD = Type->castAs<RecordType>()->getDecl();
     if (RD->isCompleteDefinition()) {
       if (RD->isStruct())
         Result += "\n\tstruct ";
       else if (RD->isUnion())
         Result += "\n\tunion ";
       else
-        assert(false && "class not allowed as an ivar type");
+        llvm_unreachable("class not allowed as an ivar type");
 
       Result += RD->getName();
       if (GlobalDefinedTags.count(RD)) {
         // struct/union is defined globally, use it.
         Result += " ";
         return true;
       }
       Result += " {\n";
ConditionalOperator *CondExpr = new (Context) ConditionalOperator(
         cast<Expr>(RHSStmt), Exp->getType(), VK_PRValue, OK_Ordinary);
     return CondExpr;
   } else if (const ObjCIvarRefExpr *IRE = dyn_cast<ObjCIvarRefExpr>(BlockExp)) {
     CPT = IRE->getType()->getAs<BlockPointerType>();
   } else if (const PseudoObjectExpr *POE
                = dyn_cast<PseudoObjectExpr>(BlockExp)) {
     CPT = POE->getType()->castAs<BlockPointerType>();
   } else {
-    assert(false && "RewriteBlockClass: Bad type");
+    llvm_unreachable("RewriteBlockClass: Bad type");
   }
   assert(CPT && "RewriteBlockClass: Bad type");
   const FunctionType *FT = CPT->getPointeeType()->getAs<FunctionType>();
   assert(FT && "RewriteBlockClass: Bad type");
   const FunctionProtoType *FTP = dyn_cast<FunctionProtoType>(FT);
   // FTP will be null for closures that don't take arguments.
 
   RecordDecl *RD = RecordDecl::Create(*Context, TTK_Struct, TUDecl,

clang/lib/Frontend/Rewrite/RewriteObjC.cpp

ConditionalOperator *CondExpr = new (Context) ConditionalOperator(
         cast<Expr>(RHSStmt), Exp->getType(), VK_PRValue, OK_Ordinary);
     return CondExpr;
   } else if (const ObjCIvarRefExpr *IRE = dyn_cast<ObjCIvarRefExpr>(BlockExp)) {
     CPT = IRE->getType()->getAs<BlockPointerType>();
   } else if (const PseudoObjectExpr *POE
                = dyn_cast<PseudoObjectExpr>(BlockExp)) {
     CPT = POE->getType()->castAs<BlockPointerType>();
   } else {
-    assert(false && "RewriteBlockClass: Bad type");
+    llvm_unreachable("RewriteBlockClass: Bad type");
   }
   assert(CPT && "RewriteBlockClass: Bad type");
   const FunctionType *FT = CPT->getPointeeType()->getAs<FunctionType>();
   assert(FT && "RewriteBlockClass: Bad type");
   const FunctionProtoType *FTP = dyn_cast<FunctionProtoType>(FT);
   // FTP will be null for closures that don't take arguments.
 
   RecordDecl *RD = RecordDecl::Create(*Context, TTK_Struct, TUDecl,

clang/lib/Frontend/SARIFDiagnostic.cpp

case DiagnosticsEngine::Warning:
     break;
   case DiagnosticsEngine::Error:
     Config = Config.setLevel(SarifResultLevel::Error).setRank(50);
     break;
   case DiagnosticsEngine::Fatal:
     Config = Config.setLevel(SarifResultLevel::Error).setRank(100);
     break;
   case DiagnosticsEngine::Ignored:
-    assert(false && "Invalid diagnostic type");
+    llvm_unreachable("Invalid diagnostic type");
   }
 
   return Rule.setDefaultConfiguration(Config);
 }
 
 llvm::StringRef SARIFDiagnostic::emitFilename(StringRef Filename,
                                               const SourceManager &SM) {
   if (DiagOpts->AbsolutePath) {

 ///
 /// This method handlen the emission of the diagnostic location information.
 /// This includes extracting as much location information as is present for
 /// the diagnostic and printing it, as well as any include stack or source
 /// ranges necessary.
 void SARIFDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
                                         DiagnosticsEngine::Level Level,
                                         ArrayRef<CharSourceRange> Ranges) {
-  assert(false && "Not implemented in SARIF mode");
+  llvm_unreachable("Not implemented in SARIF mode");
 }
 
 void SARIFDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  assert(false && "Not implemented in SARIF mode");
+  llvm_unreachable("Not implemented in SARIF mode");
 }
 
 void SARIFDiagnostic::emitImportLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                                          StringRef ModuleName) {
-  assert(false && "Not implemented in SARIF mode");
+  llvm_unreachable("Not implemented in SARIF mode");
 }
 
 void SARIFDiagnostic::emitBuildingModuleLocation(FullSourceLoc Loc,
                                                  PresumedLoc PLoc,
                                                  StringRef ModuleName) {
-  assert(false && "Not implemented in SARIF mode");
+  llvm_unreachable("Not implemented in SARIF mode");
 }
 } // namespace clang

clang/lib/Lex/PPDirectives.cpp

case tok::pp_elif:
       break;
     case tok::pp_elifdef:
       Callbacks->Elifdef(ElifToken.getLocation(), ConditionRange, CI.IfLoc);
       break;
     case tok::pp_elifndef:
       Callbacks->Elifndef(ElifToken.getLocation(), ConditionRange, CI.IfLoc);
       break;
     default:
-      assert(false && "unexpected directive kind");
+      llvm_unreachable("unexpected directive kind");
       break;
     }
   }
 
   bool RetainExcludedCB = PPOpts->RetainExcludedConditionalBlocks &&
     getSourceManager().isInMainFile(ElifToken.getLocation());
 
   if ((PPOpts->SingleFileParseMode && !CI.FoundNonSkip) || RetainExcludedCB) {

clang/lib/Lex/PreprocessingRecord.cpp

 /// \see getPreprocessedEntitiesInRange.
 bool PreprocessingRecord::isEntityInFileID(iterator PPEI, FileID FID) {
   if (FID.isInvalid())
     return false;
 
   int Pos = std::distance(iterator(this, 0), PPEI);
   if (Pos < 0) {
     if (unsigned(-Pos-1) >= LoadedPreprocessedEntities.size()) {
-      assert(0 && "Out-of bounds loaded preprocessed entity");
+      llvm_unreachable("Out-of bounds loaded preprocessed entity");
       return false;
     }
     assert(ExternalSource && "No external source to load from");
     unsigned LoadedIndex = LoadedPreprocessedEntities.size()+Pos;
     if (PreprocessedEntity *PPE = LoadedPreprocessedEntities[LoadedIndex])
       return isPreprocessedEntityIfInFileID(PPE, FID, SourceMgr);
 
     // See if the external source can see if the entity is in the file without
     // deserializing it.
     Optional<bool> IsInFile =
         ExternalSource->isPreprocessedEntityInFileID(LoadedIndex, FID);
     if (IsInFile)
       return IsInFile.value();
 
     // The external source did not provide a definite answer, go and deserialize
     // the entity to check it.
     return isPreprocessedEntityIfInFileID(
                                        getLoadedPreprocessedEntity(LoadedIndex),
                                           FID, SourceMgr);
   }
 
   if (unsigned(Pos) >= PreprocessedEntities.size()) {
-    assert(0 && "Out-of bounds local preprocessed entity");
+    llvm_unreachable("Out-of bounds local preprocessed entity");
     return false;
   }
   return isPreprocessedEntityIfInFileID(PreprocessedEntities[Pos],
                                         FID, SourceMgr);
 }
 
 /// Returns a pair of [Begin, End) iterators of preprocessed entities
 /// that source range \arg R encompasses.

clang/lib/Parse/ParseDecl.cpp

ParseIdentifier: {
         // attribute declaration and continue.
         if (NextToken().is(tok::l_paren)) {
           // Consume the __declspec identifier.
           ConsumeToken();
 
           // Eat the parens and everything between them.
           BalancedDelimiterTracker T(*this, tok::l_paren);
           if (T.consumeOpen()) {
-            assert(false && "Not a left paren?");
+            llvm_unreachable("Not a left paren?");
             return;
           }
           T.skipToEnd();
           continue;
         }
       }
 
       // In C++, check to see if this is a scope specifier like foo::bar::, if

clang/lib/Sema/SemaChecking.cpp

if (BuiltinID == AArch64::BI__builtin_arm_subp) {
     if (isNull(ArgB))
       ArgExprB = ImpCastExprToType(ArgExprB.get(), ArgTypeA, CK_NullToPointer);
 
     TheCall->setArg(0, ArgExprA.get());
     TheCall->setArg(1, ArgExprB.get());
     TheCall->setType(Context.LongLongTy);
     return false;
   }
-  assert(false && "Unhandled ARM MTE intrinsic");
+  llvm_unreachable("Unhandled ARM MTE intrinsic");
   return true;
 }
 
 /// SemaBuiltinARMSpecialReg - Handle a check if argument ArgNum of CallExpr
 /// TheCall is an ARM/AArch64 special register string literal.
 bool Sema::SemaBuiltinARMSpecialReg(unsigned BuiltinID, CallExpr *TheCall,
                                     int ArgNum, unsigned ExpectedFieldNum,
                                     bool AllowName) {

clang/lib/Sema/SemaCodeComplete.cpp

case tok::plusplus:
     if (ContextType.isNull())
       return S.getASTContext().IntTy;
     // leave as is, these operators typically return the same type.
     return ContextType;
   case tok::kw___real:
   case tok::kw___imag:
     return QualType();
   default:
-    assert(false && "unhandled unary op");
+    llvm_unreachable("unhandled unary op");
     return QualType();
   }
 }
 
 void PreferredTypeBuilder::enterBinary(Sema &S, SourceLocation Tok, Expr *LHS,
                                        tok::TokenKind Op) {
   if (!Enabled)
     return;

clang/lib/Serialization/ASTReader.cpp

if (D) {
         Merged.push_back(ID);
     }
     return D;
   }
 
   unsigned Index = ID - NUM_PREDEF_DECL_IDS;
 
   if (Index >= DeclsLoaded.size()) {
-    assert(0 && "declaration ID out-of-range for AST file");
+    llvm_unreachable("declaration ID out-of-range for AST file");
     Error("declaration ID out-of-range for AST file");
     return nullptr;
   }
 
   return DeclsLoaded[Index];
 }
 
 Decl *ASTReader::GetDecl(DeclID ID) {
   if (ID < NUM_PREDEF_DECL_IDS)
     return GetExistingDecl(ID);
 
   unsigned Index = ID - NUM_PREDEF_DECL_IDS;
 
   if (Index >= DeclsLoaded.size()) {
-    assert(0 && "declaration ID out-of-range for AST file");
+    llvm_unreachable("declaration ID out-of-range for AST file");
     Error("declaration ID out-of-range for AST file");
     return nullptr;
   }
 
   if (!DeclsLoaded[Index]) {
     ReadDeclRecord(ID);
     if (DeserializationListener)
       DeserializationListener->DeclRead(ID, DeclsLoaded[Index]);

clang/lib/Serialization/ASTWriter.cpp

void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
   std::vector<uint32_t> MacroOffsets;
 
   for (unsigned I = 0, N = MacroInfosToEmit.size(); I != N; ++I) {
     const IdentifierInfo *Name = MacroInfosToEmit[I].Name;
     MacroInfo *MI = MacroInfosToEmit[I].MI;
     MacroID ID = MacroInfosToEmit[I].ID;
 
     if (ID < FirstMacroID) {
-      assert(0 && "Loaded MacroInfo entered MacroInfosToEmit ?");
+      llvm_unreachable("Loaded MacroInfo entered MacroInfosToEmit ?");
       continue;
     }
 
     // Record the local offset of this macro.
     unsigned Index = ID - FirstMacroID;
     if (Index >= MacroOffsets.size())
       MacroOffsets.resize(Index + 1);
 
TypeID ASTWriter::GetOrCreateTypeID(QualType T) {
   return MakeTypeID(*Context, T, [&](QualType T) -> TypeIdx {
     if (T.isNull())
       return TypeIdx();
     assert(!T.getLocalFastQualifiers());
 
     TypeIdx &Idx = TypeIdxs[T];
     if (Idx.getIndex() == 0) {
       if (DoneWritingDeclsAndTypes) {
-        assert(0 && "New type seen after serializing all the types to emit!");
+        llvm_unreachable(
+            "New type seen after serializing all the types to emit!");
         return TypeIdx();
       }
 
       // We haven't seen this type before. Assign it a new ID and put it
       // into the queue of types to emit.
       Idx = TypeIdx(NextTypeID++);
       DeclTypesToEmit.push(T);
     }
DeclID ASTWriter::GetDeclRef(const Decl *D) {
   // fixed.
   if (D->isFromASTFile())
     return D->getGlobalID();
 
   assert(!(reinterpret_cast<uintptr_t>(D) & 0x01) && "Invalid decl pointer");
   DeclID &ID = DeclIDs[D];
   if (ID == 0) {
     if (DoneWritingDeclsAndTypes) {
-      assert(0 && "New decl seen after serializing all the decls to emit!");
+      llvm_unreachable(
+          "New decl seen after serializing all the decls to emit!");
       return 0;
     }
 
     // We haven't seen this declaration before. Give it a new ID and
     // enqueue it in the list of declarations to emit.
     ID = NextDeclID++;
     DeclTypesToEmit.push(const_cast<Decl *>(D));
   }

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

case ProgramPoint::BlockEdgeKind:
       HandleBlockEdge(Loc.castAs<BlockEdge>(), Pred);
       break;
 
     case ProgramPoint::BlockEntranceKind:
       HandleBlockEntrance(Loc.castAs<BlockEntrance>(), Pred);
       break;
 
     case ProgramPoint::BlockExitKind:
-      assert(false && "BlockExit location never occur in forward analysis.");
+      llvm_unreachable("BlockExit location never occur in forward analysis.");
       break;
 
     case ProgramPoint::CallEnterKind:
       HandleCallEnter(Loc.castAs<CallEnter>(), Pred);
       break;
 
     case ProgramPoint::CallExitBeginKind:
       ExprEng.processCallExit(Pred);

clang/lib/StaticAnalyzer/Core/SVals.cpp

case nonloc::PointerToMemberKind: {
 
         os << I->getType();
       }
 
       os << '}';
       break;
     }
     default:
-      assert(false && "Pretty-printed not implemented for this NonLoc.");
+      llvm_unreachable("Pretty-printed not implemented for this NonLoc.");
       break;
   }
 }
 
 void Loc::dumpToStream(raw_ostream &os) const {
   switch (getSubKind()) {
     case loc::ConcreteIntKind:
       os << castAs<loc::ConcreteInt>().getValue().getZExtValue() << " (Loc)";

clang/tools/clang-refactor/TestSupport.cpp

if (!RangeRegex.match(Comment, &Matches) || Comment.contains("CHECK")) {
         return None;
       continue;
     }
     unsigned Offset = Tok.getEndLoc().getRawEncoding();
     unsigned ColumnOffset = 0;
     if (!Matches[2].empty()) {
       // Don't forget to drop the '+'!
       if (Matches[2].drop_front().getAsInteger(10, ColumnOffset))
-        assert(false && "regex should have produced a number");
+        llvm_unreachable("regex should have produced a number");
     }
     Offset = addColumnOffset(Source, Offset, ColumnOffset);
     unsigned EndOffset;
 
     if (!Matches[3].empty()) {
       static const Regex EndLocRegex(
           "->[[:blank:]]*(\\+[[:digit:]]+):([[:digit:]]+)");
       SmallVector<StringRef, 4> EndLocMatches;
       if (!EndLocRegex.match(Matches[3], &EndLocMatches)) {
         if (DetectMistypedCommand())
           return None;
         continue;
       }
       unsigned EndLineOffset = 0, EndColumn = 0;
       if (EndLocMatches[1].drop_front().getAsInteger(10, EndLineOffset) ||
           EndLocMatches[2].getAsInteger(10, EndColumn))
-        assert(false && "regex should have produced a number");
+        llvm_unreachable("regex should have produced a number");
       EndOffset = addEndLineOffsetAndEndColumn(Source, Offset, EndLineOffset,
                                                EndColumn);
     } else {
       EndOffset = Offset;
     }
     TestSelectionRange Range = {Offset, EndOffset};
     auto It = GroupedRanges.insert(std::make_pair(
         Matches[1].str(), SmallVector<TestSelectionRange, 8>{Range}));