[musl] [PATCH 0/2] mipsel: Add debug information to syscalls

[musl] [PATCH 0/2] mipsel: Add debug information to syscalls

[Daniel Santos]

If you currently attach a debugger to a process making an interruptable
/ repeatable system call, you get no backtrace as there is no debug
information available.  This patch set adds appropriate debug
information to make debugging such programs possible.

To ease reviewing, I've split this into two patches.  We need the
preprocessor to determine rather or not we're generating ELF files and
PIC code -- so the second patch renames it to *.S.

[musl] [PATCH 1/2] mipsel: Add debug information to __syscall_cp_asm

[Daniel Santos]

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.

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
 .set    noreorder
+.cfi_sections	.debug_frame
+.abicalls
+#ifdef __PIC__
+	.option	pic2
+#else
+	.option	pic0
+#endif
+.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)
+*/
+
+	.ent	__syscall_cp_asm
+	.frame	$sp, 32, $ra
+	.mask	0x00000000, 0
+	.fmask	0x00000000, 0
+	.cfi_startproc
+	.cfi_return_column $ra
 __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
+	.cfi_restore $ra
+#ifdef __ELF__
+	.size __syscall_cp_asm,.-__syscall_cp_asm
+#endif
+	.end	__syscall_cp_asm
+	.cfi_endproc
-- 
2.24.1

Re: [musl] [PATCH 1/2] mipsel: Add debug information to __syscall_cp_asm

[Rich Felker]

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

Re: [musl] [PATCH 1/2] mipsel: Add debug information to __syscall_cp_asm

[Daniel Santos]

Sorry for my late response.  I didn't realize musl was "reply-to-list
only" and my filters didn't flag your reply without the CC.  (Filters
updated now!)

On 6/24/20 7:45 PM, Rich Felker wrote:
> 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.)

Yes, there are many things that just cannot be done with inline asm.  I
thought we had __attribute__((naked)) for all archs in GCC now, but I
guess that doesn't include MIPS (currently looks like ARM, AVR, C-SKY,
MCORE, MSP430, NDS32, RISC-V, RL78, RX, SPU, and x86).  I'll take a look
at the scripts, but I'm not strong with awk.

> 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?

It's a little bit trashy.  I guess that the ".previous" is  useless
since it's followed by a ".text" later.  But the empty .mdebug.<abi>
section is there to help gdb determine the ABI.  iiuc, newer versions of
binutils generate this automatically.

I'm not an expert in this area, or even MIPS yet; I mostly modeled what
GCC generated on this arch and then looked up why it did.  According to
GCC, it's there for compatibility with older versions of binutils (and
thus, gdb). 
https://github.com/gcc-mirror/gcc/blob/98fcd2513add205dcdd134eb29a2505ea9f81495/gcc/config/mips/mips.c#L9889
.  Seeing how this was added to gcc in 2007, it would probably be safe
to omit it, but if you want to keep it then we should remove either the
".previous" or ".text".

>>  .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.

Good point -- keep the patch only about debug info.

> 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.

Ah yes.  Just comments when I was analyzing the function.

>> +
>> +	.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.

To my knowledge, .ent, .frame, .mask and .fmask are ignored by gas, but
used by MIPSpro for generating debug information -- the exception being
that gas expects .ent if you use .end.  If you're only interested in
gas, then we should probably omit these.  GCC emits them by default and
finding the documentation for them was a fking pain.

http://csweb.cs.wfu.edu/~torgerse/Kokua/Irix_6.5.21_doc_cd/usr/share/Insight/library/SGI_bookshelves/SGI_Developer/books/MProAsLg_PG/sgi_html/ch08.html

".cfi_startproc" is necessary, but ".cfi_return_column $ra" may be
superfluous since it's probably the default after a
jump/branch-and-link.  I'll test without it and see.

>>  __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.

I'm not certain the ".cfi_register $ra, $2" can be safely added via a
script and it could probably even be moved to after the bal delay slot,
along with the ".cfi_adjust_cfa_offset -32".  In fact, I seem to recall
that "stepi" in gdb used to step "over" branch instructions and then
branch after using "stepi" on the delay slot -- I've noticed recently
that it now executes both at once.

>> +	.cfi_restore $ra
>> +#ifdef __ELF__
>> +	.size __syscall_cp_asm,.-__syscall_cp_asm
>> +#endif
> This is not needed. musl is always ELF.

Fantastic!  So if we also don't need __PIC__ then we can omit the rename
to *.S.
> Rich

I would also like to do the same thing for clone and dlsym.  I'll fiddle
with the awk scripts and see what I can get them to do when I get some
more time in the next week.

Thanks,
Daniel

[musl] [PATCH 2/2] mipsel: preprocess syscall_cp.s

[Daniel Santos]

Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
---
 src/thread/mips/{syscall_cp.s => syscall_cp.S} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename src/thread/mips/{syscall_cp.s => syscall_cp.S} (100%)

diff --git a/src/thread/mips/syscall_cp.s b/src/thread/mips/syscall_cp.S
similarity index 100%
rename from src/thread/mips/syscall_cp.s
rename to src/thread/mips/syscall_cp.S
-- 
2.24.1