mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <dalias@libc.org>
To: musl@lists.openwall.com
Subject: Re: [musl] [PATCH 1/2] mipsel: Add debug information to __syscall_cp_asm
Date: Wed, 24 Jun 2020 20:45:17 -0400	[thread overview]
Message-ID: <20200625004516.GN6430@brightrain.aerifal.cx> (raw)
In-Reply-To: <20200624233517.4909-2-daniel.santos@pobox.com>

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@irobot.com>
> Signed-off-by: Daniel Santos <daniel.santos@pobox.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

  reply	other threads:[~2020-06-25  0:45 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-24 23:35 [musl] [PATCH 0/2] mipsel: Add debug information to syscalls Daniel Santos
2020-06-24 23:35 ` [musl] [PATCH 1/2] mipsel: Add debug information to __syscall_cp_asm Daniel Santos
2020-06-25  0:45   ` Rich Felker [this message]
2020-07-05  1:34     ` Daniel Santos
2020-06-24 23:35 ` [musl] [PATCH 2/2] mipsel: preprocess syscall_cp.s Daniel Santos

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200625004516.GN6430@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).