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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.



================
Comment at: clang/lib/Analysis/CFG.cpp:1047
+          llvm_unreachable("Unexpected unary operator!");
           return llvm::None;
         }
----------------
This will create unreachable code warnings, which must be addressed before landing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135551/new/

https://reviews.llvm.org/D135551