[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. In D146466#4218011 , @efriedma wrote: > I think the reason "recoverable" ubsan causes trouble is that it introduces > branches that subsequent optimizations can abuse. So without ubsan, we just > have an udiv instruction. Wit

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. I think the reason "recoverable" ubsan causes trouble is that it introduces branches that subsequent optimizations can abuse. So without ubsan, we just have an udiv instruction. With ubsan, we conveniently have a branch on exactly the condition that would make the ud

[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. (recoverable feels like a bit of a distraction here? recoverable just means you've asked ubsan not to trap/stop on failure - but to let the program continue and do whatever it would've done without the sanitizer enabled - sometimes that's crash/trap anyway, sometimes i

[PATCH] D146466: [clang] diagnose function fallthrough

nikic added a comment. @nickdesaulniers I don't think we want to handle that on the LLVM side. That will be fundamentally unreliable. If Clang wishes to make "recoverable" ubsan have arbitrary but well-defined behavior in the case of UB, it needs to generate appropriate IR to model that. For ex

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers added a comment. In D146466#4217487 , @efriedma wrote: > The problem with a change like that is that it's not clear what the > underlying semantic model is. If we add a flag that says "try to generate > code that matches Linux kernel de

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. The problem with a change like that is that it's not clear what the underlying semantic model is. If we add a flag that says "try to generate code that matches Linux kernel developers' mental model of the underlying machine", or "loop unrolling should try to preserve

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers added a comment. In D146466#4217363 , @efriedma wrote: > There are limits to how much we can do by just changing the code clang > generates... but the two particular cases you mention probably could be > "fixed" by messing with the IR ge

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. There are limits to how much we can do by just changing the code clang generates... but the two particular cases you mention probably could be "fixed" by messing with the IR generated by clang. Sure, that probably makes sense to pursue. (If you're going to pick an ar

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers added a comment. In 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

[PATCH] D146466: [clang] diagnose function fallthrough

aaron.ballman added a comment. In D146466#4213814 , @nickdesaulniers wrote: > In D146466#4210607 , @aaron.ballman > wrote: > >> In D146466#4208216 , @dblaikie >> wrote:

[PATCH] D146466: [clang] diagnose function fallthrough

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

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers added a comment. In D146466#4214148 , @dblaikie wrote: > In D146466#4213814 , > @nickdesaulniers wrote: > >> In D146466#4207915 , @efriedma >> wrote: >>

[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. In D146466#4213814 , @nickdesaulniers wrote: > In D146466#4207915 , @efriedma > wrote: > >> I'm concerned about the potential for false positives: >> >> - If a user doesn't mark a funct

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers added a comment. In D146466#4207915 , @efriedma wrote: > I'm concerned about the potential for false positives: > > - If a user doesn't mark a function noreturn, but it doesn't actually ever > return. Ok, but a user //can// fix that in t

[PATCH] D146466: [clang] diagnose function fallthrough

aaron.ballman added a comment. In D146466#4208216 , @dblaikie wrote: > If the issue is with the actual fallthrough - that's been discussed elsewhere > recently, and I think it'd be reasonable to turn on TrapOnUnreachable > everywhere, basically (with No

[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. Yeah, we already have the return type warning and UBSan for the dynamic version of this check - seems OK? That a function lowers to unreachable isn't a bug - there's a ton of them in LLVM, quite intentionally as unimplemented functions that should never be called. If

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. I'm concerned about the potential for false positives: - If a user doesn't mark a function noreturn, but it doesn't actually ever return. - A function is conditionally broken, but the condition is never actually true (for example, jump threading creates a dead codepath

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers added inline comments. Comment at: llvm/test/CodeGen/AArch64/function-fallthrough.ll:7 +entry: + unreachable +} TODO: add test that the final MBB is a call to a noreturn function followed by unreachable Repository: rG LLVM Github Monorepo C

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers updated this revision to Diff 506760. nickdesaulniers added a comment. - git clang-format HEAD~ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146466/new/ https://reviews.llvm.org/D146466 Files: clang/docs/ReleaseNotes.rst clang

[PATCH] D146466: [clang] diagnose function fallthrough

nickdesaulniers created this revision. Herald added a subscriber: hiraditya. Herald added a project: All. nickdesaulniers requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Debugging functions that have fallen through to the