[libc-commits] [libc] [libc][setjmp] fix setjmp test (PR #87837)

[Nick Desaulniers via libc-commits]

nickdesaulniers wrote:

> As I've said before, https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249/13 the setjmp function cannot be implemented in C.

Careful James, [only a Sith deals in absolutes](https://knowyourmeme.com/memes/only-a-sith-deals-in-absolutes).

With my change, the disassembly of `setjmp` is near ideal (modulo one extra push/pop of the frame pointer, which I think is because [we recently reintroduced frame pointers](https://github.com/llvm/llvm-project/pull/86288) but could probably be fixed by building this TU with `-fomit-frame-pointers`).

> 
> This change addresses ONE way in which things can go wrong by trying to have a C implementation, but does not address the underlying problem: it shouldn't be in C in the first place.

Ah! It seems that I'm late to the party, and this has already been discussed elsewhere. (Replying to points from that thread, though perhaps I should instead be making those points on that thread)

> Adding any .s files probably requires significant infrastructure which currently doesn’t exist in libc

I don't think so.  But I think the compiler provides significant controls that can be used before resorting to out of line assembly, and not all of those controls have been discussed or explored yet.  More so opening the door for out of line asm is something we'd like to avoid as then it can be used in future arguments for "here's memcpy in out of line asm; look, you use out of line asm here and here and here so I should be able to use it, too."

> I wanted to double-check my understanding of this policy and its aims, as to me it seems like writing out the desired code in a function with the naked attribute would be a better solution

You cannot refer to function parameters of naked functions: https://godbolt.org/z/b963seKxd

I guess that's why @efriedma-quic was using `offsetof`: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249/6?u=nickdesaulniers

@jrtc27 's [point about aarch64 GCC](https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249/9?u=nickdesaulniers) appears [still valid](https://godbolt.org/z/oKsccGr4M).  It wont matter for this function in this PR; but I should revisit the other implementations of setjmp/longjmp beyond just x86.

You also can't split a function into sub-functions which are naked then force them to inline: https://godbolt.org/z/rxzszr1PK (thank god, that would have been a war crime; I agree with points in that discord that wrapper functions here would be problematic).

But modulo that one extra push/pop (which I think is fixed with `-fomit-frame-pointer`), I don't see what benefit `naked` would provide us here; we already get optimal assembly.  (Just tested `-fomit-frame-pointer` and `-momit-leaf-frame-pointer` which did not omit the push/pop from the prolog/epilog, so perhaps then `naked` fn attrs with @efriedma-quic 's pattern is the final optimization we can do here).

> These are so magic, that basically any instrumentation that the compiler might add, like sanitizers, profiling, etc, will break them.

That is a valid point, and I agree 100% with you.  _But that doesn't preclude the usage of C here._

We solved that in the Linux kernel with a small handful of function attributes:
- https://clang.llvm.org/docs/AttributeReference.html#no-profile-instrument-function
- https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
- https://clang.llvm.org/docs/AttributeReference.html#noinline

At the very least, seeing `__builtin_frame_address` without either `always_inline` or `noinline` fn attrs is immediately a red flag.  While that might not matter for individual TUs, we 100% want to support LTO'ing the libc.  So adding `noinline` here seems like the most basic addition, but we'll probably want to add MORE function attributes to this implementation.

That's orthogonal to fixing this test though, since the failure is well understood.

https://github.com/llvm/llvm-project/pull/87837