Date: Wed, 24 Jun 2020 20:45:17 -0400 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: [PATCH 1/2] mipsel: Add debug information to __syscall_cp_asm On Wed, Jun 24, 2020 at 06:35:16PM -0500, Daniel Santos wrote: > This is the function called for interruptable / repeatable syscalls like > nanosleep. Without this patch, attaching a debugger to a program making > such a syscall results in the debugger being completely unable to > perform a backtrace. On other archs this is handled by the tools/add-cfi* scripts. It might be worth revisiting whether that's the right choice; for most asm we're actually trying to get rid of the asm source files and use inline asm so it's all up to the compiler, but that can't be done for syscalls because we need to be able to adjust the stack for the outgoing syscall. (Note: this is probably also an issue for inline, non-cancellable syscalls and maybe not so easy to fix there.) Anyway some comments: > Co-Authored-By: Daniele Tamino <dtamino@...bot.com> > Signed-off-by: Daniel Santos <daniel.santos@...ox.com> > --- > src/thread/mips/syscall_cp.s | 41 +++++++++++++++++++++++++++++++++++- > 1 file changed, 40 insertions(+), 1 deletion(-) > > diff --git a/src/thread/mips/syscall_cp.s b/src/thread/mips/syscall_cp.s > index d2846264..d39bff59 100644 > --- a/src/thread/mips/syscall_cp.s > +++ b/src/thread/mips/syscall_cp.s > @@ -1,4 +1,14 @@ > +.section .mdebug.abi32 > +.previous What does this do? > .set noreorder > +.cfi_sections .debug_frame > +.abicalls > +#ifdef __PIC__ > + .option pic2 > +#else > + .option pic0 > +#endif I don't think pic0 corresponds to not having -fPIC; it's a weird thing that's not used or supported. There's no reason to override any of these (or .abicalls, which is also default and equivalent to pic2 AIUI), and the change does not seem to be at all related to adding CFI. I'm not sure if .cfi_sections .debug_frame is needed -- is it? > +.text > > .global __cp_begin > .hidden __cp_begin > @@ -9,12 +19,32 @@ > .global __cp_cancel > .hidden __cp_cancel > .type __cp_cancel,@function > -.hidden __cancel > +.hidden __cancel /* long __cancel() in src/thread/pthread_cancel.c */ > .global __syscall_cp_asm > .hidden __syscall_cp_asm > .type __syscall_cp_asm,@function > + > +/* > +long __syscall_cp_asm( > + volatile int *cancel, > + syscall_arg_t nr, > + syscall_arg_t u, > + syscall_arg_t v, > + syscall_arg_t w, > + syscall_arg_t x, > + syscall_arg_t y, > + syscall_arg_t z) > +*/ These changes are also unrelated to CFI. > + > + .ent __syscall_cp_asm > + .frame $sp, 32, $ra > + .mask 0x00000000, 0 > + .fmask 0x00000000, 0 > + .cfi_startproc > + .cfi_return_column $ra And I'm not sure about all of these. > __syscall_cp_asm: > subu $sp, $sp, 32 > + .cfi_adjust_cfa_offset 32 > __cp_begin: > lw $4, 0($4) > bne $4, $0, __cp_cancel > @@ -35,14 +65,17 @@ __cp_begin: > __cp_end: > beq $7, $0, 1f > addu $sp, $sp, 32 > + .cfi_adjust_cfa_offset -32 > subu $2, $0, $2 > 1: jr $ra > nop > > __cp_cancel: > move $2, $ra > + .cfi_register $ra, $2 > bal 1f > addu $sp, $sp, 32 > + .cfi_adjust_cfa_offset -32 > .gpword . > .gpword __cancel > 1: lw $3, ($ra) > @@ -51,3 +84,9 @@ __cp_cancel: > addu $25, $25, $3 > jr $25 > move $ra, $2 I think this part looks ok, either to be generated like on the other archs or included. > + .cfi_restore $ra > +#ifdef __ELF__ > + .size __syscall_cp_asm,.-__syscall_cp_asm > +#endif This is not needed. musl is always ELF. Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.