[libc][setjmp] fix setjmp test via naked fn attr #88054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: Nick Desaulniers (nickdesaulniers) ChangesThis 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 Use Full diff: https://github.com/llvm/llvm-project/pull/88054.diff 2 Files Affected:
diff --git a/libc/src/setjmp/x86_64/CMakeLists.txt b/libc/src/setjmp/x86_64/CMakeLists.txt
index 9899c00e7c4a65..d247dc7bda8ded 100644
--- a/libc/src/setjmp/x86_64/CMakeLists.txt
+++ b/libc/src/setjmp/x86_64/CMakeLists.txt
@@ -8,7 +8,8 @@ add_entrypoint_object(
libc.include.setjmp
COMPILE_OPTIONS
-O3
- -fno-omit-frame-pointer
+ -fomit-frame-pointer
+ -momit-leaf-frame-pointer
)
add_entrypoint_object(
diff --git a/libc/src/setjmp/x86_64/setjmp.cpp b/libc/src/setjmp/x86_64/setjmp.cpp
index 8b6981d4cc34a2..1380777b8e81af 100644
--- a/libc/src/setjmp/x86_64/setjmp.cpp
+++ b/libc/src/setjmp/x86_64/setjmp.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "include/llvm-libc-macros/offsetof-macro.h"
#include "src/__support/common.h"
#include "src/setjmp/setjmp_impl.h"
@@ -15,42 +16,29 @@
namespace LIBC_NAMESPACE {
+__attribute__((naked))
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;
+ asm("mov %%rbx, %c[rbx](%%rdi)\n\t"
+ "mov %%rbp, %c[rbp](%%rdi)\n\t"
+ "mov %%r12, %c[r12](%%rdi)\n\t"
+ "mov %%r13, %c[r13](%%rdi)\n\t"
+ "mov %%r14, %c[r14](%%rdi)\n\t"
+ "mov %%r15, %c[r15](%%rdi)\n\t"
+
+ "lea 8(%%rsp), %%rax\n\t"
+ "mov %%rax, %c[rsp](%%rdi)\n\t"
+
+ "mov %[read_rip], %c[rip](%%rdi)\n\t"
+
+ "xorl %%eax, %%eax\n\t"
+ "retq"
+ ::
+ [rbx] "i"(offsetof(__jmp_buf, rbx)), [r12] "i"(offsetof(__jmp_buf, r12)),
+ [r13] "i"(offsetof(__jmp_buf, r13)), [r14] "i"(offsetof(__jmp_buf, r14)),
+ [r15] "i"(offsetof(__jmp_buf, r15)), [rbp] "i"(offsetof(__jmp_buf, rbp)),
+ [rsp] "i"(offsetof(__jmp_buf, rsp)), [rip] "i"(offsetof(__jmp_buf, rip)),
+ [read_rip] "r" (reinterpret_cast<__UINTPTR_TYPE__>(__builtin_return_address(0)))
+ : "rax");
}
} // namespace LIBC_NAMESPACE
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
nickdesaulniers
added a commit
to nickdesaulniers/llvm-project
that referenced
this pull request
Apr 9, 2024
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
nickdesaulniers
changed the title
nickdesaulniers
added a commit
that referenced
this pull request
May 20, 2024
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
requested review from
frobtech,
lntue,
michaelrj-google and
jyknight
|
The more I work on the out of line asm version (#88157), the more I hate it and prefer this approach. |
nickdesaulniers
force-pushed
the
setjmp_naked
branch
from
b60a733 to
b480e91
Compare
|
rebased; removed cmake workaround; used [[]] attribute syntax. Ready for re-review. |
jyknight
reviewed
Oct 3, 2024
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.
nickdesaulniers
force-pushed
the
setjmp_naked
branch
from
b480e91 to
734e044
Compare
jyknight
approved these changes
Oct 10, 2024
There was a problem hiding this comment.
Just noting that GCC docs say "Only basic asm statements can safely be included in naked functions (see Basic Asm — Assembler Instructions Without Operands).". Since we're depending on extended asm with immediate operands working, it would be nice to get a specification guarantee that states that does works.
LGTM for this change in any case.
| @@ -8,12 +8,6 @@ add_entrypoint_object( | |||
| libc.hdr.types.jmp_buf | |||
| COMPILE_OPTIONS | |||
| -O3 | |||
There was a problem hiding this comment.
It seems weird to hardcode the optimization level, but that seems to be llvm-libc style so OK.
There was a problem hiding this comment.
Yes, I've discussed this with the team a bit. I'd prefer to "just do what libc++ does in terms of configuring the build for size vs performance." Looking at their docs it seems that CMAKE_BUILD_TYPE is perhaps the only way to signal that you'd like a release build, vs MinSizeRel.
Either way, we should file a new issue and discuss this there.
|
According to my previous experience, when using none asm statement, clang will effectively reject the code at compile time. |
SchrodingerZhu
approved these changes
Oct 11, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/131/builds/8445 Here is the relevant piece of the build log for the reference |
|
re: buildbot failures: ack, still digging into this. |
|
follow up fix: #112415 |
nickdesaulniers
added a commit
to nickdesaulniers/llvm-project
that referenced
this pull request
Oct 15, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
observed in the GCC build by manually expanding LLVM_LIBC_FUNCTION to add
`gnu::nothrow` to the alias.
We probably need to revisit adding nothrow throughout our declarations, so
there is probably a better way to clean this up in the future.
Link: llvm#88054
nickdesaulniers
added a commit
to nickdesaulniers/llvm-project
that referenced
this pull request
Oct 15, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
Marking functions as 'naked' implies 'nothrow', so the function declaration
should be marked nothrow. Only do this conditionally for GCC for now, otherwise
clang with diagnose -Wmissing-exception-spec on the __ ## name ## _impl__
alias.
We probably need to revisit adding nothrow throughout our definitions, so
there is probably a better way to clean this up in the future.
Link: llvm#88054
nickdesaulniers
added a commit
to nickdesaulniers/llvm-project
that referenced
this pull request
Oct 15, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
Marking functions as 'naked' implies 'nothrow', so the function declaration
should be marked nothrow. Only do this conditionally for GCC for now, otherwise
clang with diagnose -Wmissing-exception-spec on the __ ## name ## _impl__
alias.
We probably need to revisit adding nothrow throughout our definitions, so
there is probably a better way to clean this up in the future.
Link: llvm#88054
nickdesaulniers
added a commit
that referenced
this pull request
Oct 15, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
observed in the GCC build by manually expanding LLVM_LIBC_FUNCTION to add
`gnu::nothrow` to the alias.
We probably need to revisit adding nothrow throughout our declarations, so
there is probably a better way to clean this up in the future.
Link: #88054
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 16, 2024
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. Use `naked` function attribute to avoid function prolog/epilog.
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 16, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
observed in the GCC build by manually expanding LLVM_LIBC_FUNCTION to add
`gnu::nothrow` to the alias.
We probably need to revisit adding nothrow throughout our declarations, so
there is probably a better way to clean this up in the future.
Link: llvm#88054
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
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. Use `naked` function attribute to avoid function prolog/epilog.
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
observed in the GCC build by manually expanding LLVM_LIBC_FUNCTION to add
`gnu::nothrow` to the alias.
We probably need to revisit adding nothrow throughout our declarations, so
there is probably a better way to clean this up in the future.
Link: llvm#88054
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 17, 2024
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. Use `naked` function attribute to avoid function prolog/epilog.
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 17, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
observed in the GCC build by manually expanding LLVM_LIBC_FUNCTION to add
`gnu::nothrow` to the alias.
We probably need to revisit adding nothrow throughout our declarations, so
there is probably a better way to clean this up in the future.
Link: llvm#88054
EricWF
pushed a commit
to efcs/llvm-project
that referenced
this pull request
Oct 22, 2024
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. Use `naked` function attribute to avoid function prolog/epilog.
EricWF
pushed a commit
to efcs/llvm-project
that referenced
this pull request
Oct 22, 2024
Fixes:
llvm-project/libc/src/setjmp/x86_64/setjmp.cpp:21:25: error: ‘int
__llvm_libc_19_0_0_git::setjmp(__jmp_buf*)’ specifies less restrictive
attribute than its target ‘int
__llvm_libc_19_0_0_git::__setjmp_impl__(__jmp_buf*)’: ‘nothrow’
[-Werror=missing-attributes]
21 | LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
| ^~~~~~
observed in the GCC build by manually expanding LLVM_LIBC_FUNCTION to add
`gnu::nothrow` to the alias.
We probably need to revisit adding nothrow throughout our declarations, so
there is probably a better way to clean this up in the future.
Link: llvm#88054
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Use
nakedfunction attribute to avoid function prolog/epilog.