[libc-commits] [libc] [libc][setjmp][x86] implement setjmp in terms of out of line asm (PR #88157)

Nick Desaulniers via libc-commits libc-commits at lists.llvm.org
Mon Apr 22 14:15:21 PDT 2024


https://github.com/nickdesaulniers updated https://github.com/llvm/llvm-project/pull/88157

>From a449c60dcc35cd350b5e3db5ae0136ae6a3f5411 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Tue, 9 Apr 2024 10:16:31 -0700
Subject: [PATCH 1/4] [libc][setjmp][x86] implement setjmp in terms of out of
 line asm

This fixes libc_setjmp_unittests for me.

This would consistently fail for me locally, to the point where I could not run
ninja libc-unit-tests without ninja libc_setjmp_unittests failing.

Turns out that since I enabled -ftrivial-auto-var-init=pattern in
commit 1d5c16d ("[libc] default enable -ftrivial-auto-var-init=pattern (#78776)")
this has been a problem. Our x86_64 setjmp definition disabled -Wuninitialized,
so we wound up clobbering these registers and instead backing up
0xAAAAAAAAAAAAAAAA rather than the actual register value.

Adding out of line asm to llvm-libc is opening pandora's box. This should not
be merged without discussion and buy in from maintainers. We should have a
policy in place for _when_ it's acceptable to use out of line asm or not.

Link: https://github.com/llvm/llvm-project/pull/87837
Link: https://github.com/llvm/llvm-project/pull/88054
Link: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249
---
 libc/src/setjmp/x86_64/CMakeLists.txt |  7 +---
 libc/src/setjmp/x86_64/setjmp.S       | 50 ++++++++++++++++++++++++
 libc/src/setjmp/x86_64/setjmp.cpp     | 56 ---------------------------
 libc/test/include/CMakeLists.txt      | 11 ++++++
 libc/test/include/setjmp_test.cpp     | 29 ++++++++++++++
 5 files changed, 91 insertions(+), 62 deletions(-)
 create mode 100644 libc/src/setjmp/x86_64/setjmp.S
 delete mode 100644 libc/src/setjmp/x86_64/setjmp.cpp
 create mode 100644 libc/test/include/setjmp_test.cpp

diff --git a/libc/src/setjmp/x86_64/CMakeLists.txt b/libc/src/setjmp/x86_64/CMakeLists.txt
index 9899c00e7c4a65..38f2b872e673cf 100644
--- a/libc/src/setjmp/x86_64/CMakeLists.txt
+++ b/libc/src/setjmp/x86_64/CMakeLists.txt
@@ -1,14 +1,9 @@
 add_entrypoint_object(
   setjmp
   SRCS
-    setjmp.cpp
+    setjmp.S
   HDRS
     ../setjmp_impl.h
-  DEPENDS
-    libc.include.setjmp
-  COMPILE_OPTIONS
-    -O3
-    -fno-omit-frame-pointer
 )
 
 add_entrypoint_object(
diff --git a/libc/src/setjmp/x86_64/setjmp.S b/libc/src/setjmp/x86_64/setjmp.S
new file mode 100644
index 00000000000000..f16a9c9c6610bd
--- /dev/null
+++ b/libc/src/setjmp/x86_64/setjmp.S
@@ -0,0 +1,50 @@
+//===-- Implementation of setjmp --------------------------------*- ASM -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#define paste(ns) _ZN22 ## ns ## 6setjmpEP9__jmp_buf
+#define expand(x) paste(x)
+#define LIBC_NAMESPACE_setjump expand(LIBC_NAMESPACE)
+// aka _ZN22__llvm_libc_19_0_0_git6setjmpEP9__jmp_buf
+// aka __llvm_libc_19_0_0_git::setjmp(__jmp_buf*)
+
+// Brittle! Changing the layout of __jmp_buf will break this!
+#define RBX_OFFSET 0
+#define RBP_OFFSET 8
+#define R12_OFFSET 16
+#define R13_OFFSET 24
+#define R14_OFFSET 32
+#define R15_OFFSET 40
+#define RSP_OFFSET 48
+#define RIP_OFFSET 56
+
+.global setjump
+.global LIBC_NAMESPACE_setjump
+
+.type setjump, @function
+.type LIBC_NAMESPACE_setjump, @function
+
+setjump:
+LIBC_NAMESPACE_setjump:
+
+  mov %rbx, RBX_OFFSET(%rdi)
+  mov %rbp, RBP_OFFSET(%rdi)
+  mov %r12, R12_OFFSET(%rdi)
+  mov %r13, R13_OFFSET(%rdi)
+  mov %r14, R14_OFFSET(%rdi)
+  mov %r15, R15_OFFSET(%rdi)
+  lea 8(%rsp), %rax
+  mov %rax, RSP_OFFSET(%rdi)
+  mov (%rsp), %rax
+  mov %rax, RIP_OFFSET(%rdi)
+  xor %eax, %eax
+  ret
+
+.size setjump, . - setjump
+.size LIBC_NAMESPACE_setjump, . - LIBC_NAMESPACE_setjump
+
+.section .note.GNU-stack, "", @progbits
diff --git a/libc/src/setjmp/x86_64/setjmp.cpp b/libc/src/setjmp/x86_64/setjmp.cpp
deleted file mode 100644
index 8b6981d4cc34a2..00000000000000
--- a/libc/src/setjmp/x86_64/setjmp.cpp
+++ /dev/null
@@ -1,56 +0,0 @@
-//===-- Implementation of setjmp ------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "src/__support/common.h"
-#include "src/setjmp/setjmp_impl.h"
-
-#if !defined(LIBC_TARGET_ARCH_IS_X86_64)
-#error "Invalid file include"
-#endif
-
-namespace LIBC_NAMESPACE {
-
-LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
-  register __UINT64_TYPE__ rbx __asm__("rbx");
-  register __UINT64_TYPE__ r12 __asm__("r12");
-  register __UINT64_TYPE__ r13 __asm__("r13");
-  register __UINT64_TYPE__ r14 __asm__("r14");
-  register __UINT64_TYPE__ r15 __asm__("r15");
-
-  // We want to store the register values as is. So, we will suppress the
-  // compiler warnings about the uninitialized variables declared above.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wuninitialized"
-  LIBC_INLINE_ASM("mov %1, %0\n\t" : "=m"(buf->rbx) : "r"(rbx) :);
-  LIBC_INLINE_ASM("mov %1, %0\n\t" : "=m"(buf->r12) : "r"(r12) :);
-  LIBC_INLINE_ASM("mov %1, %0\n\t" : "=m"(buf->r13) : "r"(r13) :);
-  LIBC_INLINE_ASM("mov %1, %0\n\t" : "=m"(buf->r14) : "r"(r14) :);
-  LIBC_INLINE_ASM("mov %1, %0\n\t" : "=m"(buf->r15) : "r"(r15) :);
-#pragma GCC diagnostic pop
-
-  // We want the rbp of the caller, which is what __builtin_frame_address(1)
-  // should return. But, compilers generate a warning that calling
-  // __builtin_frame_address with non-zero argument is unsafe. So, we use
-  // the knowledge of the x86_64 ABI to fetch the callers rbp. As per the ABI,
-  // the rbp of the caller is pushed on to the stack and then new top is saved
-  // in this function's rbp. So, we fetch it from location at which this
-  // functions's rbp is pointing.
-  buf->rbp = *reinterpret_cast<__UINTPTR_TYPE__ *>(__builtin_frame_address(0));
-
-  // The callers stack address is exactly 2 pointer widths ahead of the current
-  // frame pointer - between the current frame pointer and the rsp of the caller
-  // are the return address (pushed by the x86_64 call instruction) and the
-  // previous stack pointer as required by the x86_64 ABI.
-  // The stack pointer is ahead because the stack grows down on x86_64.
-  buf->rsp = reinterpret_cast<__UINTPTR_TYPE__>(__builtin_frame_address(0)) +
-             sizeof(__UINTPTR_TYPE__) * 2;
-  buf->rip = reinterpret_cast<__UINTPTR_TYPE__>(__builtin_return_address(0));
-  return 0;
-}
-
-} // namespace LIBC_NAMESPACE
diff --git a/libc/test/include/CMakeLists.txt b/libc/test/include/CMakeLists.txt
index 8d8dff53169f6a..4d42f348d5e00c 100644
--- a/libc/test/include/CMakeLists.txt
+++ b/libc/test/include/CMakeLists.txt
@@ -69,3 +69,14 @@ add_libc_test(
   DEPENDS
     libc.include.llvm-libc-macros.stdckdint_macros
 )
+
+add_libc_test(
+  setjmp_test
+  SUITE
+    libc_include_tests
+  SRCS
+    setjmp_test.cpp
+  DEPENDS
+    libc.include.llvm-libc-macros.offsetof_macro
+    libc.include.llvm-libc-types.jmp_buf
+  )
diff --git a/libc/test/include/setjmp_test.cpp b/libc/test/include/setjmp_test.cpp
new file mode 100644
index 00000000000000..e82d27a438b23c
--- /dev/null
+++ b/libc/test/include/setjmp_test.cpp
@@ -0,0 +1,29 @@
+//===-- Unittests for setjmp ----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDSList-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "include/llvm-libc-macros/offsetof-macro.h"
+#include "include/llvm-libc-types/jmp_buf.h"
+#include "test/UnitTest/Test.h"
+
+// If this test fails, then *_OFFSET macro definitions in
+// libc/src/setjmp/x86_64/setjmp.S need to be fixed. This is a simple change
+// detector.
+TEST(LlvmLibcSetjmpTest, JmpBufLayout) {
+#ifdef __x86_64__
+  ASSERT_EQ(offsetof(__jmp_buf, rbx), 0UL);
+  ASSERT_EQ(offsetof(__jmp_buf, rbp), 8UL);
+  ASSERT_EQ(offsetof(__jmp_buf, r12), 16UL);
+  ASSERT_EQ(offsetof(__jmp_buf, r13), 24UL);
+  ASSERT_EQ(offsetof(__jmp_buf, r14), 32UL);
+  ASSERT_EQ(offsetof(__jmp_buf, r15), 40UL);
+  ASSERT_EQ(offsetof(__jmp_buf, rsp), 48UL);
+  ASSERT_EQ(offsetof(__jmp_buf, rip), 56UL);
+  ASSERT_EQ(sizeof(__jmp_buf), 64UL);
+  ASSERT_EQ(alignof(__jmp_buf), 8UL);
+#endif // __x86_64__
+}

>From fa3e8174238de4d30b2298e962ebe0c8a895acfb Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Mon, 22 Apr 2024 13:38:37 -0700
Subject: [PATCH 2/4] define NO_EXEC_STACK_DIRECTIVE

---
 libc/src/__support/assembly.h   | 22 ++++++++++++++++++++++
 libc/src/setjmp/x86_64/setjmp.S |  4 +++-
 2 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 libc/src/__support/assembly.h

diff --git a/libc/src/__support/assembly.h b/libc/src/__support/assembly.h
new file mode 100644
index 00000000000000..9a9de72991705c
--- /dev/null
+++ b/libc/src/__support/assembly.h
@@ -0,0 +1,22 @@
+//===-- assembly.h - libc assembler support macros based on compiler-rt's -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_LIBC_SRC___SUPPORT_ASSEMBLY_H
+#define LLVM_LIBC_SRC___SUPPORT_ASSEMBLY_H
+
+#ifndef __ASSEMBLER__
+#error "No not include assembly.h in non-asm sources"
+#endif
+
+#if defined(ELF) && (defined(__GNU__) || defined(__FreeBSD__) ||               \
+                     defined(__Fuchsia__) || defined(__linux__))
+#define NO_EXEC_STACK_DIRECTIVE .section.note.GNU - stack, "", % progbits
+#else
+#define NO_EXEC_STACK_DIRECTIVE
+#endif
+
+#endif // LLVM_LIBC_SRC___SUPPORT_ASSEMBLY_H
diff --git a/libc/src/setjmp/x86_64/setjmp.S b/libc/src/setjmp/x86_64/setjmp.S
index f16a9c9c6610bd..074addcdb93b92 100644
--- a/libc/src/setjmp/x86_64/setjmp.S
+++ b/libc/src/setjmp/x86_64/setjmp.S
@@ -6,6 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "src/__support/assembly.h"
+
 #define paste(ns) _ZN22 ## ns ## 6setjmpEP9__jmp_buf
 #define expand(x) paste(x)
 #define LIBC_NAMESPACE_setjump expand(LIBC_NAMESPACE)
@@ -47,4 +49,4 @@ LIBC_NAMESPACE_setjump:
 .size setjump, . - setjump
 .size LIBC_NAMESPACE_setjump, . - LIBC_NAMESPACE_setjump
 
-.section .note.GNU-stack, "", @progbits
+NO_EXEC_STACK_DIRECTIVE

>From 913e955ee5349bb067c53e3b2cfcd9a80fbfc528 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Mon, 22 Apr 2024 14:10:14 -0700
Subject: [PATCH 3/4] use shared header for offsets

---
 libc/src/setjmp/x86_64/setjmp.S   | 11 +----------
 libc/src/setjmp/x86_64/setjmp.h   | 22 ++++++++++++++++++++++
 libc/test/include/setjmp_test.cpp | 17 +++++++++--------
 3 files changed, 32 insertions(+), 18 deletions(-)
 create mode 100644 libc/src/setjmp/x86_64/setjmp.h

diff --git a/libc/src/setjmp/x86_64/setjmp.S b/libc/src/setjmp/x86_64/setjmp.S
index 074addcdb93b92..76836be18c4c08 100644
--- a/libc/src/setjmp/x86_64/setjmp.S
+++ b/libc/src/setjmp/x86_64/setjmp.S
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/__support/assembly.h"
+#include "src/setjmp/x86_64/setjmp.h"
 
 #define paste(ns) _ZN22 ## ns ## 6setjmpEP9__jmp_buf
 #define expand(x) paste(x)
@@ -14,16 +15,6 @@
 // aka _ZN22__llvm_libc_19_0_0_git6setjmpEP9__jmp_buf
 // aka __llvm_libc_19_0_0_git::setjmp(__jmp_buf*)
 
-// Brittle! Changing the layout of __jmp_buf will break this!
-#define RBX_OFFSET 0
-#define RBP_OFFSET 8
-#define R12_OFFSET 16
-#define R13_OFFSET 24
-#define R14_OFFSET 32
-#define R15_OFFSET 40
-#define RSP_OFFSET 48
-#define RIP_OFFSET 56
-
 .global setjump
 .global LIBC_NAMESPACE_setjump
 
diff --git a/libc/src/setjmp/x86_64/setjmp.h b/libc/src/setjmp/x86_64/setjmp.h
new file mode 100644
index 00000000000000..686f8ec005551d
--- /dev/null
+++ b/libc/src/setjmp/x86_64/setjmp.h
@@ -0,0 +1,22 @@
+
+// TODO: license block
+#ifndef LLVM_LIBC_SRC_SETJMP_X86_64_SETJMP_H
+#define LLVM_LIBC_SRC_SETJMP_X86_64_SETJMP_H
+
+#ifdef __ASSEMBLER__
+#define UL(x) x
+#else
+#define UL(x) x##UL
+#endif
+
+// Brittle! Changing the layout of __jmp_buf will break this!
+#define RBX_OFFSET UL(0)
+#define RBP_OFFSET UL(8)
+#define R12_OFFSET UL(16)
+#define R13_OFFSET UL(24)
+#define R14_OFFSET UL(32)
+#define R15_OFFSET UL(40)
+#define RSP_OFFSET UL(48)
+#define RIP_OFFSET UL(56)
+
+#endif // LLVM_LIBC_SRC_SETJMP_X86_64_SETJMP_H
diff --git a/libc/test/include/setjmp_test.cpp b/libc/test/include/setjmp_test.cpp
index e82d27a438b23c..14d1e89b317d28 100644
--- a/libc/test/include/setjmp_test.cpp
+++ b/libc/test/include/setjmp_test.cpp
@@ -8,6 +8,7 @@
 
 #include "include/llvm-libc-macros/offsetof-macro.h"
 #include "include/llvm-libc-types/jmp_buf.h"
+#include "src/setjmp/x86_64/setjmp.h"
 #include "test/UnitTest/Test.h"
 
 // If this test fails, then *_OFFSET macro definitions in
@@ -15,14 +16,14 @@
 // detector.
 TEST(LlvmLibcSetjmpTest, JmpBufLayout) {
 #ifdef __x86_64__
-  ASSERT_EQ(offsetof(__jmp_buf, rbx), 0UL);
-  ASSERT_EQ(offsetof(__jmp_buf, rbp), 8UL);
-  ASSERT_EQ(offsetof(__jmp_buf, r12), 16UL);
-  ASSERT_EQ(offsetof(__jmp_buf, r13), 24UL);
-  ASSERT_EQ(offsetof(__jmp_buf, r14), 32UL);
-  ASSERT_EQ(offsetof(__jmp_buf, r15), 40UL);
-  ASSERT_EQ(offsetof(__jmp_buf, rsp), 48UL);
-  ASSERT_EQ(offsetof(__jmp_buf, rip), 56UL);
+  ASSERT_EQ(offsetof(__jmp_buf, rbx), RBX_OFFSET);
+  ASSERT_EQ(offsetof(__jmp_buf, rbp), RBP_OFFSET);
+  ASSERT_EQ(offsetof(__jmp_buf, r12), R12_OFFSET);
+  ASSERT_EQ(offsetof(__jmp_buf, r13), R13_OFFSET);
+  ASSERT_EQ(offsetof(__jmp_buf, r14), R14_OFFSET);
+  ASSERT_EQ(offsetof(__jmp_buf, r15), R15_OFFSET);
+  ASSERT_EQ(offsetof(__jmp_buf, rsp), RSP_OFFSET);
+  ASSERT_EQ(offsetof(__jmp_buf, rip), RIP_OFFSET);
   ASSERT_EQ(sizeof(__jmp_buf), 64UL);
   ASSERT_EQ(alignof(__jmp_buf), 8UL);
 #endif // __x86_64__

>From ea7f70cf94c681310660d8c70984837a4db7e2a3 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Mon, 22 Apr 2024 14:14:50 -0700
Subject: [PATCH 4/4] fix NO_EXEC_STACK_DIRECTIVE

---
 libc/src/__support/assembly.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libc/src/__support/assembly.h b/libc/src/__support/assembly.h
index 9a9de72991705c..ab0c35b0042ad8 100644
--- a/libc/src/__support/assembly.h
+++ b/libc/src/__support/assembly.h
@@ -12,9 +12,9 @@
 #error "No not include assembly.h in non-asm sources"
 #endif
 
-#if defined(ELF) && (defined(__GNU__) || defined(__FreeBSD__) ||               \
-                     defined(__Fuchsia__) || defined(__linux__))
-#define NO_EXEC_STACK_DIRECTIVE .section.note.GNU - stack, "", % progbits
+#if defined(__ELF__) && (defined(__GNU__) || defined(__FreeBSD__) ||           \
+                         defined(__Fuchsia__) || defined(__linux__))
+#define NO_EXEC_STACK_DIRECTIVE .section.note.GNU - stack, "", @progbits
 #else
 #define NO_EXEC_STACK_DIRECTIVE
 #endif



More information about the libc-commits mailing list