[PATCH] D146466: [clang] diagnose function fallthrough

Wed Mar 22 11:02:37 PDT 2023

nickdesaulniers added a comment.

In D146466#4207915 <https://reviews.llvm.org/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 their code.  Also, I think we could possibly help by adding diagnostics for this.  Unconditionally calling a noreturn function or having an obviously infinite loop should trigger such a diagnostic with note encouraging the user to mark their function noreturn as well?

> - A function is conditionally broken, but the condition is never actually true (for example, jump threading creates a dead codepath).
> - A function might actually be unconditionally broken, but it doesn't matter because it never actually gets called.  This can easily happen with C++ templates, or an "outlining" optimization on LLVM IR.

These sound like bugs in LLVM.  Even if we don't expose function fallthrough to users as a diagnostic warning (via `-Wfunction-fallthrough`), perhaps a `-mllvm` flag is still handy for llvm developers to spot places where we could perhaps improve codegen.

I don't think we should //ever// emit a branch to an unconditional block; perhaps we need to be more aggressive about cleaning those up?  Otherwise it seems like a waste of instructions, which pollute I$.

> For some of these situations, there are somewhat straightforward workarounds we can suggest for users... but in some cases, there aren't.  I really don't want to to implement warnings where the best suggestion we can make to fix the warning is "Try refactoring your code; it might go away".

Contrast that with the intent of the feature request; diagnosing when a function may fall through to another. That is quite painful to debug when that occurs at runtime.  How do we reconcile?

In D146466#4208216 <https://reviews.llvm.org/D146466#4208216>, @dblaikie wrote:

> Yeah, we already have the return type warning

Which warning?

> and UBSan for the dynamic version of this check - seems OK?

Which ubsan check?

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

IDK about an entire function lowering to unreachable; more so that there exists a branch to an unreachable basic block (or simply an unreachable inst) where that results in one function falling through to another.

> If the issue is with the actual fallthrough - that's been discussed elsewhere recently,

Link?

> and I think it'd be reasonable to turn on TrapOnUnreachable everywhere, basically (with NoTrapAfterNoreturn too, perhaps, to keep the cost low) so that these trivial fallthroughs hit int3 instead of going on to the next function.

Right, at this point, I'm probably just going to turn on `-mllvm -trap-unreachable` in the Linux kernel.   I might create a `-f` flag for it, since I do my best to avoid using `-mllvm` in the kernel's build system.  I think this is unfortunate because I suspect it will increase the kernel image size and hide codegen bugs in LLVM, particularly when sanitizers are enabled. (i.e. places where we generate branches to unreachable).

In D146466#4210607 <https://reviews.llvm.org/D146466#4210607>, @aaron.ballman wrote:

> In D146466#4208216 <https://reviews.llvm.org/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 NoTrapAfterNoreturn too, perhaps, to keep the cost low) so that these trivial fallthroughs hit int3 instead of going on to the next function.
>
> As I understand things, folks are confused about the runtime behavior of falling through and magically calling some other function. If we can warn about that situation from the FE without false positives, then huttah, I think that's sufficient. But I don't think we know of a way to do that from the FE, so I think ensuring a runtime trap is more helpful to folks than the status quo. (Also, in general, I think we want to avoid emitting diagnostics from the backend -- that has usability issues where clean `-fsyntax-only` builds suddenly get warnings when doing full codegen, or in circumstances where the optimization level changes the diagnostic output, etc.)

The issue is that there are problems that may occur as a result of optimizations; if the front end does not do optimizations, it cannot diagnose these.  Some examples include:

- `-Wframe-larger-than=` (useful for embedded systems without "nearly unlimited" stack, though an imperfect proxy for maximal stack depth, which is what these systems actually want. Helpful for spotting large stack allocations that should come from the heap or static memory).
- `-Wattribute-warning` (some implementations of `_FORTIFY_SOURCE` such as the Linux kernel's rely on this; specifically that calls to functions with these attributes get optimized out. This provides //significant// protections during //compile time// when //due to optimizations// we have an out of bounds access, when compared to implementations that simply use `_Static_assert`)
- `-Wfunction-fallthrough` (hypothetical new diagnostic this patch is adding)

Even if Clang had a high level IR and did optimizations, it still could not guarantee the results of LLVM's subsequent optimizations.

Does that mean that `-Wframe-larger-than=` (and the like) should not exist because it's not diagnosable via `-fsyntax-only`?

I //do// think that there are a class of diagnostics that are useful to users that //depend// on optimizations being run.  While a policy around new diagnostics being diagnosable via `-fsyntax-only` is a simplistic litmus test, I do think it allows for our competition to implement diagnostics that may be more useful to users.  This hamstrings our competitiveness, IMO.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146466