-
Notifications
You must be signed in to change notification settings - Fork 11.5k
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][x86] implement setjmp in terms of out of line asm #88157
base: main
Are you sure you want to change the base?
Conversation
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 (llvm#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: llvm#87837 Link: llvm#88054 Link: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249
@llvm/pr-subscribers-libc Author: Nick Desaulniers (nickdesaulniers) ChangesThis fixes libc_setjmp_unittests for me. This would consistently fail for me locally, to the point where I could not run Turns out that since I enabled -ftrivial-auto-var-init=pattern in Adding out of line asm to llvm-libc is opening pandora's box. This should not Link: #87837 Full diff: https://github.com/llvm/llvm-project/pull/88157.diff 5 Files Affected:
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__
+}
|
It would be helpful in future code reviews to document a policy with regards to where and when Assembler sources are appropriate. That way when reviewers point out infractions, they can point to this written policy, which may help contributors understand that it's not the solely personal preferences of reviewers but rather a previously agreed upon rule by maintainers. Link: llvm#87837 Link: llvm#88157 Link: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249/12
I don't have a strong preference on out-of-line vs. naked. I think naked is probably slightly easier to maintain for a few small functions; the current patch doesn't have all the scaffolding necessary to handle non-ELF targets. |
I definitely do like the safety provided by the offsetof macros in the naked fn impl (#88054), but I do worry that GCC not supported that function attribute will make the implementation for
|
As to what's unsupported for mach-o: $ clang libc/src/setjmp/x86_64/setjmp.S -c --target=x86_64-darwin
libc/src/setjmp/x86_64/setjmp.S:28:1: error: unknown directive
.type setjump, @function
^
libc/src/setjmp/x86_64/setjmp.S:29:1: error: unknown directive
.type _ZN22LIBC_NAMESPACE6setjmpEP9__jmp_buf, @function
^
libc/src/setjmp/x86_64/setjmp.S:47:1: error: unknown directive
.size setjump, . - setjump
^
libc/src/setjmp/x86_64/setjmp.S:48:1: error: unknown directive
.size _ZN22LIBC_NAMESPACE6setjmpEP9__jmp_buf, . - _ZN22LIBC_NAMESPACE6setjmpEP9__jmp_buf
^
libc/src/setjmp/x86_64/setjmp.S:50:19: error: unexpected token in '.section' directive
.section .note.GNU-stack, "", @progbits
^ also, macho-o expects a leading underscore prefix on identifiers. https://godbolt.org/z/nv1hazMvE for a few more darwin specific directives. |
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#define paste(ns) _ZN22 ## ns ## 6setjmpEP9__jmp_buf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest an extern C name which includes the expansion of LIBC_NAMESPACE in the name, rather than manual C++ name-mangling. If you want to do C++ name-mangling you'll need to figure out how to generate the "22" since that needs to be the length of the namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an extern C name which includes the expansion of LIBC_NAMESPACE in the name
I'm sorry I don't follow the suggestion. Can you please "break out the crayons" for me?
libc/src/setjmp/x86_64/setjmp.S
Outdated
// 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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move into a header file, so setjmp_test can include?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libc/src/setjmp/x86_64/setjmp.S
Outdated
.type setjump, @function | ||
.type LIBC_NAMESPACE_setjump, @function | ||
|
||
setjump: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this public symbol need to be conditioned on LIBC_COPT_PUBLIC_PACKAGING
?
Maybe something like:
#ifdef LIBC_COPT_PUBLIC_PACKAGING
#define LIBC_ASM_PUBLIC_ALIAS(alias, orig) \
.globl alias SEPARATOR \
.type alias,@function SEPARATOR \
.set alias, orig
#else
#define LIBC_ASM_PUBLIC_ALIAS(alias, orig)
#endif
LIBC_ASM_PUBLIC_ALIAS(setjmp, LIBC_NAMESPACE_setjmp)
(Note: in the above .size
isn't needed, as it gets copied over automatically)
[edit: added SEPARATOR
, since otherwise it'd all get mushed together on one line]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably! Good find wrt SEPARATOR.
Orthogonal: Why does apple+aarch64 need %%
as separator? 🤮
The mach-o asm-portability issues are handled in compiler-rt's assembly.h file, too:
|
It would be helpful in future code reviews to document a policy with regards to where and when Assembler sources are appropriate. That way when reviewers point out infractions, they can point to this written policy, which may help contributors understand that it's not the solely personal preferences of reviewers but rather a previously agreed upon rule by maintainers. Link: llvm#87837 Link: llvm#88157 Link: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249/12
It would be helpful in future code reviews to document a policy with regards to where and when Assembler sources are appropriate. That way when reviewers point out infractions, they can point to this written policy, which may help contributors understand that it's not solely the personal preferences of individual reviewers but instead rather a previously agreed upon rule by maintainers. Link: #87837 Link: #88157 Link: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249/12
#ifdef __ASSEMBLER__ | ||
#define UL(x) x | ||
#else | ||
#define UL(x) x##UL | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be useful again, move to assembly.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm...maybe not. I do have a:
11 #ifndef __ASSEMBLER__
12 #error "No not include assembly.h in non-asm sources" ■ "No not include assembly.h in non-asm sources"
13 #endif
in assembly.h. Maybe I should change that so that it can be included in either sources. Or create yet another new header.
Meta comment, the larger this PR gets, the more the naked fn attr looks attractive. That said, there is a fair amount of one time cost for adding some functionality here that will be reusable for other asm implementations in the future. Not sure yet how I feel yet about which is "better." But I think completing this impl correctly will give everyone a better sense for the different approaches; and I am happy to discard whichever approaches are unsuitable. Sometimes having the multiple implementations on hand better forms the discussion. |
Orthogonal but tangentially related, #89542 points out that setjmp is supposed to be a macro! |
#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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libc/src/setjmp/x86_64/setjmp.h is the updated source location
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. The implemenation should be rewritten entirely. I've proposed three different ways to do so (linked below). Until we decide which way to go, at least disable this hardening feature for this function for now so that the unit tests go back to green. Link: #87837 Link: #88054 Link: #88157 Fixes: #91164
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: #87837
Link: #88054
Link: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249