efriedma added a comment.

> Yes, but kernel developers would prefer to diagnose issues at compile time 
> when possible, rather that at runtime. Function fallthough is diagnosable at 
> compile time, as this patch demonstrates.

I'm not saying the diagnostic is useless.  I'm saying users are going to find 
cases where their perfectly good code blows up, and so the warning will get 
disabled for every large codebase except the Linux kernel.  And every user that 
goes through the process of seeing the false positive and disabling the warning 
will start distrusting other warnings that are actually reliable.

> 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.

> Can you or @efriedma demonstrate a concrete case you're thinking of? I've 
> tried:

The cases I'm thinking of are cases which *do* actually appear to fall through, 
but don't because of invariants that aren't visible to the compiler.  Functions 
that look like `void f() { __builtin_unreachable() }` actually exist in LLVM.  
(The pattern tends to show up with virtual functions...)  Or a function may or 
may not be noreturn depending on the argument values.  Or jump threading 
generated a codepath that has unconditional undefined behavior, but we can't 
actually prove it because there's a call to a function that isn't marked 
willreturn.

Have you tried building LLVM with this to see what shows up?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146466

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to