[libc][setjmp][x86] implement setjmp in terms of out of line asm #88157

nickdesaulniers · 2024-04-09T17:20:09Z

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

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

llvmbot · 2024-04-09T17:20:40Z

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

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

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

5 Files Affected:

(modified) libc/src/setjmp/x86_64/CMakeLists.txt (+1-6)
(added) libc/src/setjmp/x86_64/setjmp.S (+50)
(removed) libc/src/setjmp/x86_64/setjmp.cpp (-56)
(modified) libc/test/include/CMakeLists.txt (+11)
(added) libc/test/include/setjmp_test.cpp (+29)

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

efriedma-quic · 2024-04-09T20:50:19Z

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.

nickdesaulniers · 2024-04-09T21:06:29Z

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 setjmp for aarch64 either:

unsupported for GCC, or
out of line asm, just for setjmp and just for aarch64.

nickdesaulniers · 2024-04-09T21:12:37Z

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.

jyknight · 2024-04-09T20:29:32Z

libc/src/setjmp/x86_64/setjmp.S

+//
+//===----------------------------------------------------------------------===//
+
+#define paste(ns) _ZN22 ## ns ## 6setjmpEP9__jmp_buf


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.

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?

jyknight · 2024-04-09T20:29:44Z

libc/src/setjmp/x86_64/setjmp.S

+// 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!


Move into a header file, so setjmp_test can include?

jyknight · 2024-04-09T20:52:10Z

libc/src/setjmp/x86_64/setjmp.S

+.type setjump, @function
+.type LIBC_NAMESPACE_setjump, @function
+
+setjump:


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]

Probably! Good find wrt SEPARATOR.

Orthogonal: Why does apple+aarch64 need %% as separator? 🤮

libc/src/setjmp/x86_64/setjmp.S

jyknight · 2024-04-09T21:50:21Z

The mach-o asm-portability issues are handled in compiler-rt's assembly.h file, too:

libc/src/setjmp/x86_64/setjmp.S:28:1: error: unknown directive
.type setjump, @function

SYMBOL_IS_FUNC macro

libc/src/setjmp/x86_64/setjmp.S:47:1: error: unknown directive
.size setjump, . - setjump

END_COMPILERRT_FUNCTION macro

also, macho-o expects a leading underscore prefix on identifiers.

SYMBOL_NAME macro

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

libc/src/setjmp/x86_64/setjmp.h

nickdesaulniers · 2024-04-22T21:17:04Z

libc/src/setjmp/x86_64/setjmp.h

+#ifdef __ASSEMBLER__
+#define UL(x) x
+#else
+#define UL(x) x##UL
+#endif


Will be useful again, move to assembly.h

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.

nickdesaulniers · 2024-04-22T21:25:02Z

Meta comment, the larger this PR gets, the more the naked fn attr looks attractive.
#88054

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.

libc/src/setjmp/x86_64/setjmp.S

nickdesaulniers · 2024-04-22T22:01:25Z

Orthogonal but tangentially related, #89542 points out that setjmp is supposed to be a macro!

nickdesaulniers · 2024-04-22T22:06:36Z

libc/test/include/setjmp_test.cpp

+#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


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

nickdesaulniers marked this pull request as draft April 9, 2024 17:20

llvmbot added the libc label Apr 9, 2024

This was referenced Apr 9, 2024

[libc][docs] codify Policy on Assembler Sources #88185

Merged

[libc][setjmp] fix setjmp test #87837

Draft

nickdesaulniers requested review from jyknight and efriedma-quic April 9, 2024 20:17

jyknight reviewed Apr 9, 2024

View reviewed changes

nickdesaulniers added 3 commits April 22, 2024 13:40

define NO_EXEC_STACK_DIRECTIVE

fa3e817

use shared header for offsets

913e955

fix NO_EXEC_STACK_DIRECTIVE

ea7f70c

nickdesaulniers commented Apr 22, 2024

View reviewed changes

add license block

327e64b

fix noexec stack AGAIN

8d48548

nickdesaulniers commented Apr 22, 2024

View reviewed changes

libc/src/setjmp/x86_64/setjmp.S Outdated Show resolved Hide resolved

nickdesaulniers added 4 commits April 22, 2024 14:40

SYMBOL_IS_FUNC

9182f1c

setjmp not setjump (idiot)

221ef63

SYMBOL_NAME

81a2396

END_FUNC

e39c288

nickdesaulniers commented Apr 22, 2024

View reviewed changes

nickdesaulniers mentioned this pull request May 20, 2024

[libc][setjmp] disable -ftrivial-auto-var-init=pattern for now #92796

Merged

nickdesaulniers mentioned this pull request May 23, 2024

[libc][setjmp] fix setjmp test via naked fn attr #88054

Merged

[libc][setjmp][x86] implement setjmp in terms of out of line asm #88157

Are you sure you want to change the base?

[libc][setjmp][x86] implement setjmp in terms of out of line asm #88157

Uh oh!

Conversation

nickdesaulniers commented Apr 9, 2024

Uh oh!

llvmbot commented Apr 9, 2024

Uh oh!

efriedma-quic commented Apr 9, 2024

Uh oh!

nickdesaulniers commented Apr 9, 2024

Uh oh!

nickdesaulniers commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyknight Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jyknight commented Apr 9, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers commented Apr 22, 2024

Uh oh!

Uh oh!

nickdesaulniers commented Apr 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickdesaulniers commented Apr 9, 2024 •

edited

Loading

jyknight Apr 9, 2024 •

edited

Loading