[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