[PATCH] D146466: [clang] diagnose function fallthrough

Wed Mar 22 12:14:14 PDT 2023

dblaikie added a comment.

In D146466#4213814 <https://reviews.llvm.org/D146466#4213814>, @nickdesaulniers wrote:

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

But that's a false positive - there was no bug in their code. We try to avoid those in compile-time diagnostics. Maybe the noreturn function is a library they're calling outside their control.

>> - 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 understand why these would be bugs in LLVM - the LLVM source code itself has many functions that are intentionally unconditionally broken if you ever call them (they have just `llvm_unreachable` as their body) - and they are, in practice, never called. These aren't bugs/there isn't anything to do to "fix" this code and a warning/error that flagged them as such would be noise and inconvenience to LLVM developers.

A sanitizer error if they're ever called would be suitable, and does exist - if a developer introduced a bug where one of these got called, it could be diagnosed by -fsanitize=undefined.

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

Generally the solution LLVM/Clang prefers is compile-time diagnostics that can be implemented reliably/deterministically in the frontend, and sanitizers that fail fast at runtime for ~everything else.

> In D146466#4208216 <https://reviews.llvm.org/D146466#4208216>, @dblaikie wrote:
>
>> Yeah, we already have the return type warning
>
> Which warning?

`-Wreturn-type`: https://godbolt.org/z/cde5MGeac

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

https://godbolt.org/z/n1YGWc9Wb - "runtime error: execution reached the end of a value-returning function without returning a value"

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

https://discourse.llvm.org/t/can-we-keep-must-progress-optimizations-around-infinite-loop-in-c-while-avoiding-some-surprising-behavior/69205/3

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

What sort of codegen bugs would this hide? What's the problematic case with sanitizers you're picturing? I don't quite understand it - perhaps a godbolt link/example?

If this does increase size at all - beyond the necessary cases (adding an int3 on an empty and never-called function or a never taken branch that would've fallen through) those are bugs we could fix.

That's partly what I wanted to do when I started looking at enabling TrapOnUnreachable in general - trying to get to the minimal number of traps necessary, so code with an infinite-but-side-effecting (so valid C++/IR) loop shouldn't need a trap after it, and nothing branches to the unreachable so it's fine to omit any trap there. But I never could figure out how to do that nicely (basically is the last instruction before the end where the trap might go, an unconditional jump? Then no need for a trap, probably... (I mean, someone else might be jumping into this tail, so it's a bit trickier than just that, and even just that wasn't obvious to me))

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

Different tools for different folks - I don't think it's outright unacceptable to have optimizer-driven diagnostics, but I think we're understandably cautious of them.

I wouldn't totally mind if frame-large-than were a sanitizer failure only if/when the function is called. Doesn't seem so bad. But it's not the worst thing as it is today either.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146466