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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
