[PATCH] D146466: [clang] diagnose function fallthrough

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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

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

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

  void x (void) {
      for (;;);
  }
  int y (int x) {
      if (x)
          __builtin_unreachable();
      return 42;
  }
  __attribute__((noreturn))
  extern void z (void);
  
  void w (void) { z(); }

with this patch and none of the above produce such a false positive with my patch applied at any optimization level. (i.e. no `-Wfunction-fallthrough` produced; you'll notice I explicitly check for calls to noreturn functions).  So I'd like to make sure we're not arguing against a strawman here.  I'm happy to add more test cases to this patch to assure reviewers there are no false positives.

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

I'm not sure why you're referring to //calls//.  This diagnostic is strictly related to intra-function control flow that results in branch to unreachable, which generally is lowered to a branch off the end of a function.  I would think that would be considered unacceptable.

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

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.

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

Strange; we get different behavior between C and C++ here in clang wrt. whether a `ret` is inserted or not. But for C, I think it demonstrates that that diagnostic is not enough to diagnose function fallthrough; try adding `__builtin_unreachable()` and notice how `-Wreturn-type` is of no help.

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

(Strange again; we get different behavior here between C and C++ in clang)

That said, it would be nice to diagnose function fallthrough at compile time rather than having to have a distinct UBSan build catch this at runtime.

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

Yes, this seems similar.  Thanks for the link; I'll watch that thread and think about whether there's common solutions to both problems here.

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

Here's a reduced test case from a recent kernel incident:
https://godbolt.org/z/q8TWrjMf6
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.

Note: that test case has an off by one bug in the do while loop that was fixed; but it's additionally problematic that LLVM generated code that was not "recoverable."
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=efbcbb12ee99f750c9f25c873b55ad774871de2a

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

Quick stats for a kernel build (x86 defconfig):
Default:
85635360 B == ~81.66 MB
Default+`-mllvm -trap-unreachable`:
85826000 B == ~81.85 MB
(so a 0.36% increase in binary size)

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

As you note <https://discourse.llvm.org/t/can-we-keep-must-progress-optimizations-around-infinite-loop-in-c-while-avoiding-some-surprising-behavior/69205/5?u=nickdesaulniers>, it is interesting to see `TargetOptions::TrapUnreachable` enabled by default for specific targets.  It is probably worthwhile pursuing getting it enabled for more targets.  Were there patches to that effect?

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

Honestly, I should probably write a new diagnostic for a single object being stack allocated if it's greater than a given threshold; that's probably more useful for what the kernel is looking for; otherwise -Wframe-larger-than= can be death by a thousand papercuts due to inlining decisions.

Making `-Wframe-larger-than` into a sanitizer is perhaps interesting; it would probably require careful interaction with the runtime to know what the "current stack depth" was at all times.  Though, "we prefer to diagnose things at compile time, when possible."  @kees might know if the kernel has any protection to detect a potential stack overflow at runtime, or what the requirements for some such thing might look like.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146466