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

Fix .cfi_undefined metadata with LLVM 16 #2202

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
+83 −50
Open

Fix .cfi_undefined metadata with LLVM 16 #2202

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
047c38e
[RISC-V] Add .cfi_undefined to the rtld entry point
arichardson Aug 20, 2024
666cdb8
[CHERI] Correctly add .cfi_undefined to _start
arichardson Aug 20, 2024
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
22 changes: 19 additions & 3 deletions lib/csu/aarch64c/crt1_c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,11 @@
* This restriction only applies to statically linked binaries since the dynamic
* linker takes care of initialization otherwise.
*/
void
_start(void *auxv,
__dead2 static void

Check failure on line 71 in lib/csu/aarch64c/crt1_c.c

View workflow job for this annotation

GitHub Actions / Style Checker 

storage class should be at the beginning of the declaration
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you don't need the explicit __dead2 as __libc_start1 is already tagged with that so the compiler will infer it automatically. That said, if you do keep it, I would spell this as eitherstatic void __dead2 or static __dead2 void. Those are the predominate styles in the tree (a few more of the former).

__start(void *auxv,
void (*cleanup)(void), /* from shared loader */
struct Struct_Obj_Entry *obj) /* from shared loader */
{
__asm__ volatile(".cfi_undefined c30");
int argc = 0;
char **argv = NULL;
char **env = NULL;
Expand Down Expand Up @@ -131,3 +130,20 @@

__libc_start1(argc, argv, env, cleanup, main, data_cap, code_cap);
}

/*
* The real entry point _start just sets the unwind info for CRA to undefined
* which tells libunwind to stop unwinding and then calls the real __start.
* This is needed because ".cfi_undefined" inline assembly in a non-naked
* function could be overridden by the default unwinding information.
*/
__attribute__((naked, used)) void
_start(void *auxv,
Copy link
Member

Choose a reason for hiding this comment

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

What's the benefit of doing this in C rather than just writing assembly? Though I continue to feel that, longer-term, we should push for some GNU attribute that allows you to eschew the assembly entirely, and can make things less stupid upstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

It means I don't need to add another file to the build system. Also means we don't need to be careful about adding all the required extra directives to the .S file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm planning to upstream this to FreeBSD as well

void (*cleanup)(void), /* from shared loader */
struct Struct_Obj_Entry *obj) /* from shared loader */
{
__asm__(
".cfi_undefined c30\n"
"bl %0"
:: "i"(__start));
}
1 change: 0 additions & 1 deletion lib/csu/riscv/crt1_c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ void __start(int argc, char **argv, char **env, void (*cleanup)(void)) __dead2;
void
__start(int argc, char **argv, char **env, void (*cleanup)(void))
{
__asm__ volatile(".cfi_undefined ra");
#ifdef SHOULD_PROCESS_CAP_RELOCS
/*
* Initialize __cap_relocs for static executables. The run-time linker
Expand Down
1 change: 1 addition & 0 deletions lib/csu/riscv/crt1_s.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

#include <machine/asm.h>
ENTRY(_start)
.cfi_undefined ra # Stop unwinding here
mv a3, a2 # cleanup
addi a1, a0, 8 # get argv
ld a0, 0(a0) # load argc
Expand Down
22 changes: 19 additions & 3 deletions lib/csu/riscv64c/crt1_c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,11 @@
* This restriction only applies to statically linked binaries since the dynamic
* linker takes care of initialization otherwise.
*/
void
_start(void *auxv,
__dead2 static void

Check failure on line 71 in lib/csu/riscv64c/crt1_c.c

View workflow job for this annotation

GitHub Actions / Style Checker 

storage class should be at the beginning of the declaration
__start(void *auxv,
void (*cleanup)(void), /* from shared loader */
struct Struct_Obj_Entry *obj) /* from shared loader */
{
__asm__ volatile(".cfi_undefined cra");
int argc = 0;
char **argv = NULL;
char **env = NULL;
Expand Down Expand Up @@ -128,3 +127,20 @@

__libc_start1(argc, argv, env, cleanup, main, NULL, NULL);
}

/*
* The real entry point _start just sets the unwind info for CRA to undefined
* which tells libunwind to stop unwinding and then calls the real __start.
* This is needed because ".cfi_undefined" inline assembly in a non-naked
* function could be overridden by the default unwinding information.
*/
__attribute__((naked, used)) void
_start(void *auxv,
void (*cleanup)(void), /* from shared loader */
struct Struct_Obj_Entry *obj) /* from shared loader */
{
__asm__(
".cfi_undefined cra\n"
"ccall %0"
:: "i"(__start));
}
Loading