[libc][setjmp] remove naked fn attr

[nickdesaulniers]

Even though I just added it in 66f968c (and fixed up in 46200fc),
while writing the i386 version, I found that simply using "m" constraints
allowed for me to avoid offsetof and naked function attribute, while generating
the same exact disassembly. Simplify x86_64 setjmp using this style.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

Even though I just added it in 66f968c (and fixed up in 46200fc),
while writing the i386 version, I found that simply using "m" constraints
allowed for me to avoid offsetof and naked function attribute, while generating
the same exact disassembly. Simplify x86_64 setjmp using this style.

---

Full diff: https://github.com/llvm/llvm-project/pull/112443.diff

3 Files Affected:

• (modified) libc/src/setjmp/setjmp_impl.h (-12)
• (modified) libc/src/setjmp/x86_64/CMakeLists.txt (+1)
• (modified) libc/src/setjmp/x86_64/setjmp.cpp (+18-17)

diff --git a/libc/src/setjmp/setjmp_impl.h b/libc/src/setjmp/setjmp_impl.h
index d035409e581954..1d9b9e3b5ca5b2 100644
--- a/libc/src/setjmp/setjmp_impl.h
+++ b/libc/src/setjmp/setjmp_impl.h
@@ -17,18 +17,6 @@
 
 namespace LIBC_NAMESPACE_DECL {
 
-// TODO(https://github.com/llvm/llvm-project/issues/112427)
-// Some of the architecture-specific definitions are marked `naked`, which in
-// GCC implies `nothrow`.
-//
-// Right now, our aliases aren't marked `nothrow`, so we wind up in a situation
-// where clang will emit -Wmissing-exception-spec if we add `nothrow` here, but
-// GCC will emit -Wmissing-attributes here without `nothrow`. We need to update
-// LLVM_LIBC_FUNCTION to denote when a function throws or not.
-
-#ifdef LIBC_COMPILER_IS_GCC
-[[gnu::nothrow]]
-#endif
 int setjmp(jmp_buf buf);
 
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/setjmp/x86_64/CMakeLists.txt b/libc/src/setjmp/x86_64/CMakeLists.txt
index b5b0d9ba65599c..464144758cbbea 100644
--- a/libc/src/setjmp/x86_64/CMakeLists.txt
+++ b/libc/src/setjmp/x86_64/CMakeLists.txt
@@ -8,6 +8,7 @@ add_entrypoint_object(
     libc.hdr.types.jmp_buf
   COMPILE_OPTIONS
     -O3
+    -fomit-frame-pointer
 )
 
 add_entrypoint_object(
diff --git a/libc/src/setjmp/x86_64/setjmp.cpp b/libc/src/setjmp/x86_64/setjmp.cpp
index f6e82642edd7da..489aadffd05321 100644
--- a/libc/src/setjmp/x86_64/setjmp.cpp
+++ b/libc/src/setjmp/x86_64/setjmp.cpp
@@ -6,7 +6,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "include/llvm-libc-macros/offsetof-macro.h"
 #include "src/__support/common.h"
 #include "src/__support/macros/config.h"
 #include "src/setjmp/setjmp_impl.h"
@@ -17,29 +16,31 @@
 
 namespace LIBC_NAMESPACE_DECL {
 
-[[gnu::naked]]
 LLVM_LIBC_FUNCTION(int, setjmp, (jmp_buf buf)) {
   asm(R"(
-      mov %%rbx, %c[rbx](%%rdi)
-      mov %%rbp, %c[rbp](%%rdi)
-      mov %%r12, %c[r12](%%rdi)
-      mov %%r13, %c[r13](%%rdi)
-      mov %%r14, %c[r14](%%rdi)
-      mov %%r15, %c[r15](%%rdi)
+      mov %%rbx, %[rbx]
+      mov %%rbp, %[rbp]
+      mov %%r12, %[r12]
+      mov %%r13, %[r13]
+      mov %%r14, %[r14]
+      mov %%r15, %[r15]
 
       lea 8(%%rsp), %%rax
-      mov %%rax, %c[rsp](%%rdi)
+      mov %%rax, %[rsp]
 
       mov (%%rsp), %%rax
-      mov %%rax, %c[rip](%%rdi)
-
-      xorl %%eax, %%eax
-      retq)" ::[rbx] "i"(offsetof(__jmp_buf, rbx)),
-      [rbp] "i"(offsetof(__jmp_buf, rbp)), [r12] "i"(offsetof(__jmp_buf, r12)),
-      [r13] "i"(offsetof(__jmp_buf, r13)), [r14] "i"(offsetof(__jmp_buf, r14)),
-      [r15] "i"(offsetof(__jmp_buf, r15)), [rsp] "i"(offsetof(__jmp_buf, rsp)),
-      [rip] "i"(offsetof(__jmp_buf, rip))
+      mov %%rax, %[rip]
+      )" ::
+      [rbx] "m"(buf->rbx),
+      [rbp] "m"(buf->rbp),
+      [r12] "m"(buf->r12),
+      [r13] "m"(buf->r13),
+      [r14] "m"(buf->r14),
+      [r15] "m"(buf->r15),
+      [rsp] "m"(buf->rsp),
+      [rip] "m"(buf->rip)
       : "rax");
+  return 0;
 }
 
 } // namespace LIBC_NAMESPACE_DECL

[jyknight]

This is unsafe. Without the naked attribute, the compiler could decide to emit a call-frame for this function, which would be totally broken. It might work today, under the compiler flag you tried with, but it is not guaranteed. I think llvm-libc should aspire not to depend on observed-but-not-guaranteed behaviors for correctness.

Additionally, the constraint string permits the compiler to allocate the addresses for the "m" constraints into registers which would be overwritten by the asm. So it's also unsafe due to that.

[nickdesaulniers]

the constraint string permits the compiler to allocate the addresses for the "m" constraints into registers

Doesn't m mean explicitly "not registers?" I didn't use r or rm, which to me implies "into registers."

[nickdesaulniers]

Doesn't m mean explicitly "not registers?" I didn't use r or rm, which to me implies "into registers."

#112437 (comment)

Will close this for now.

I could have avoided this mess by simply changing the original asm to use the register parameters as outputs rather than inputs.