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.

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][RISCV] Add naked attribute to setjmp/longjmp #100036

Merged

Conversation

ilovepi
Copy link
Contributor

@ilovepi ilovepi commented Jul 23, 2024

We want to avoid any possibility that the compiler will insert a
prologue/epilogue violating the calling contracts for these special
functions, potentially clobbering registers that must be preserved. To
do that they should be marked naked, as is already the case on ARM.

Created using spr 1.3.4
@llvmbot llvmbot added the libc label Jul 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 23, 2024

@llvm/pr-subscribers-libc

Author: Paul Kirth (ilovepi)

Changes

We want to avoid any possibility that the compiler will insert a
prologue/epilogue violating the calling contracts for these special
functions, potentially clobbering registers that must be preserved. To
do that they should be marked naked, as is already the case on ARM.


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

2 Files Affected:

  • (modified) libc/src/setjmp/riscv/longjmp.cpp (+1)
  • (modified) libc/src/setjmp/riscv/setjmp.cpp (+1)
diff --git a/libc/src/setjmp/riscv/longjmp.cpp b/libc/src/setjmp/riscv/longjmp.cpp
index 0f9537ccc4151..b14f636659ac3 100644
--- a/libc/src/setjmp/riscv/longjmp.cpp
+++ b/libc/src/setjmp/riscv/longjmp.cpp
@@ -30,6 +30,7 @@
 
 namespace LIBC_NAMESPACE_DECL {
 
+[[gnu::naked]]
 LLVM_LIBC_FUNCTION(void, longjmp, (__jmp_buf * buf, int val)) {
   LOAD(ra, buf->__pc);
   LOAD(s0, buf->__regs[0]);
diff --git a/libc/src/setjmp/riscv/setjmp.cpp b/libc/src/setjmp/riscv/setjmp.cpp
index 12def578b56f3..92982cc9d74d4 100644
--- a/libc/src/setjmp/riscv/setjmp.cpp
+++ b/libc/src/setjmp/riscv/setjmp.cpp
@@ -29,6 +29,7 @@
 
 namespace LIBC_NAMESPACE_DECL {
 
+[[gnu::naked]]
 LLVM_LIBC_FUNCTION(int, setjmp, (__jmp_buf * buf)) {
   STORE(ra, buf->__pc);
   STORE(s0, buf->__regs[0]);

@mikhailramalho
Copy link
Member

LGTM but please wait for @michaelrj-google @lntue or @petrhosek to give the go-ahead.

There were some discussions about these functions in the past so better to double-check with them first.

@mikhailramalho mikhailramalho requested a review from lntue July 23, 2024 14:42
@ilovepi
Copy link
Contributor Author

ilovepi commented Jul 23, 2024

LGTM but please wait for @michaelrj-google @lntue or @petrhosek to give the go-ahead.

There were some discussions about these functions in the past so better to double-check with them first.

Happy to wait, but naked is already on the ARM implementation, and AFAICT is required on any of these functions. TBH, I'd prefer these to be implemented in assembly files, so that we don't have to worry about it doing the right thing.

@lntue
Copy link
Contributor

lntue commented Jul 23, 2024

LGTM but please wait for @michaelrj-google @lntue or @petrhosek to give the go-ahead.
There were some discussions about these functions in the past so better to double-check with them first.

Happy to wait, but naked is already on the ARM implementation, and AFAICT is required on any of these functions. TBH, I'd prefer these to be implemented in assembly files, so that we don't have to worry about it doing the right thing.

From the previous discussion and experiment by @nickdesaulniers , naked attribute + inline asm seems to be the cleanest: #87837 (comment)

@ilovepi ilovepi merged commit 05b586b into main Jul 23, 2024
8 checks passed
@ilovepi ilovepi deleted the users/ilovepi/spr/libcriscv-add-naked-attribute-to-setjmplongjmp branch July 23, 2024 17:42
ilovepi added a commit that referenced this pull request Jul 23, 2024
ilovepi added a commit that referenced this pull request Jul 23, 2024
Reverts #100036

This caused a failure on bots:
https://lab.llvm.org/buildbot/#/builders/183/builds/1799

We likely need to discuss the particulars here a bit more deeply before
either relanding or choosing an alternate solution.
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
We want to avoid any possibility that the compiler will insert a
prologue/epilogue violating the calling contracts for these special
functions, potentially clobbering registers that must be preserved. To
do that they should be marked naked, as is already the case on ARM.
See #87837 for further context.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251407
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
Reverts #100036

This caused a failure on bots:
https://lab.llvm.org/buildbot/#/builders/183/builds/1799

We likely need to discuss the particulars here a bit more deeply before
either relanding or choosing an alternate solution.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants