[PATCH] D146466: [clang] diagnose function fallthrough

Thu Mar 23 10:47:53 PDT 2023

nickdesaulniers added a comment.

In D146466#4214770 <https://reviews.llvm.org/D146466#4214770>, @efriedma wrote:

>> Note how __ubsan_handle_out_of_bounds is literally marked RECOVERABLE in https://github.com/llvm/llvm-project/blob/fb8d894f23c5e805f0c87d89fb9d6c0eed3a0e72/compiler-rt/lib/ubsan/ubsan_handlers.h#L90-L91, and yet we generate a call to it that will fall off the end of the function when that callback returns.
>
> Using "recoverable" UBSan doesn't actually affect the code we generate after the diagnostic, and it doesn't suppress compiler optimizations that turn provably undefined code into "unreachable".  So often the code blows up anyway.  Maybe we should try to do something different here in some cases.  For out-of-bounds specifically, I'm not sure what a fix for that would look like, though; there isn't any intuitive behavior for an out-of-bounds access.

Recoverable is important concept for ubsan callbacks; some are expected for the program to continue some are not.  If we enable UBSan to help us find UB in our program, but the presence of said UB causes our program to go off the rails, then that would not be considered recoverable.  A less charitable interpretation would be to call that sanitizer broken.

I wonder if the issue I'm seeing in https://godbolt.org/z/aaj6KTrPe is actually something better solved by additional use of `freeze` on `poison`? (uh oh, I've summoned Cthulhu (and the Cenobites) by uttering those two terms in the same sentence! A blood sacrifice is necessary!)

For example, where does the unreachable come from in that example?

SCCPPass is turning a `br i1 poison, ...` into an `unreachable`.

Where did the branch on poison come from?

LoopFullUnrollPass introduced a bunch of branches on poison when unrolling the loop.

I'd bet that if we "froze" the poison first before branching on it (and only did so when `-fsanitize=array-bounds` was enabled) then we'd never end up replacing the branch with unreachable, which is destroying our control flow and making our sanitizer not actually recoverable.

---

Come to think of it, the other test of "broken code" I've seen in the past (an alluded to upthread) was related to the divide by zero sanitizer.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.3-rc3&id=e5d523f1ae8f2cef01f8e071aeee432654166708
I should reduce a test case from that, and see if perhaps `freeze` is also a solution there, too.  Then maybe warn on fallthrough becomes moot (or less interesting) if these are simply codegen bugs for various UBSanitizers.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146466