Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc][setjmp] remove naked fn attr #112443

Closed
wants to merge 4 commits into from

Conversation

nickdesaulniers
Copy link
Member

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.

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 llvmbot added the libc label Oct 15, 2024
@nickdesaulniers nickdesaulniers changed the title y [libc][setjmp] remove naked fn attr Oct 15, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 15, 2024

@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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants