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.

Sign up for GitHub

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] fix setjmp test #87837

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[libc][setjmp] fix setjmp test #87837

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 7 additions & 16 deletions libc/src/setjmp/x86_64/setjmp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,13 @@
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
asm("mov %%rbx, %[rbx]\n\t"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be asm volatile?

Copy link
Member Author

@nickdesaulniers nickdesaulniers Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No; volatile just keeps the inline asm blob around in case the outputs are DCE'd. In this case, because we write through the function parameter, we can't DCE the inline asm blob. You can verify this by compiling this change and disassembling this function and observing that the inline asm is not discarded.

Theoretically, if were to inline setjmp (perhaps due to LTO) and someone accidentally called setjmp but then did not use the jmp_buf parameter ever then we could DCE this inline asm blob. Which we would want.

But because this inline asm would not be safe to inline under LTO, and because this function also uses dangerous constructs like __builtin_frame_address, we should add [[noinline]] to this function definition.

"mov %%r12, %[r12]\n\t"
"mov %%r13, %[r13]\n\t"
"mov %%r14, %[r14]\n\t"
"mov %%r15, %[r15]"
: [rbx] "=m"(buf->rbx), [r12] "=m"(buf->r12), [r13] "=m"(buf->r13),
[r14] "=m"(buf->r14), [r15] "=m"(buf->r15));

// We want the rbp of the caller, which is what __builtin_frame_address(1)
// should return. But, compilers generate a warning that calling
Expand Down
Loading