Re: [PATCH] RISC-V: Fix elfutils testsuite unwind failures.

[Mark Wielaard <mark at klomp dot org>]

Hi,

On Sun, 2019-01-13 at 15:48 -0800, Jim Wilson wrote:
> The clone.S patch fixes 2 elfutils testsuite unwind failures, where the
> backtrace gets stuck repeating __thread_start until we hit the backtrace
> limit.  This was confirmed by building and installing a patched glibc and
> then building elfutils and running its testsuite.

Obviously I would really like this patch to go in because it makes the
riscv64 elfutils testsuite zero fail. Unfortunately my riscv64 setup is
fairly slow (qemu/libvirt based) and so I haven't actually rebuild
glibc myself with your patch. So the following feedback is mostly
theoretical.

> Unfortunately, the testcase isn't working as expected and I don't know why.
> The testcase passes even when my clone.S patch is not installed.  The testcase
> looks logically similarly to the elfutils testcases that are failing.  Maybe
> there is a subtle difference in how the glibc unwinding works versus the
> elfutils unwinding?  I don't have good gdb pthread support yet, so I haven't
> found a way to debug this.  Anyways, I don't know if the testcase is useful or
> not.  If the testcase isn't useful then maybe the clone.S patch is OK without
> a testcase?

For what it is worth on my setup with glibc-2.28.9000-24.fc30.riscv64
your testcase goes into an infinite loop with -DUSE_PTHREADS=1, but
ends quickly without. So it does seem useful.

There might indeed be subtle differences between the elfutils unwinder,
which is out of process and might take a bit more "risks" to keep
unwinding, compared to the libgcc unwinder which is in-process and so
might be very conservative and stop on any frame that looks suspicious.

You might be able to see how it exactly unwinds by simply adding
something like printf ("%p\n", (void *) _Unwind_GetIP (ctx));
in the callback () function.

> diff --git a/sysdeps/unix/sysv/linux/riscv/clone.S b/sysdeps/unix/sysv/linux/riscv/clone.S
> index c079c1fb9f..0ff9ab3fd9 100644
> --- a/sysdeps/unix/sysv/linux/riscv/clone.S
> +++ b/sysdeps/unix/sysv/linux/riscv/clone.S
> @@ -69,6 +69,11 @@ L (error):
>  
>  ENTRY (__thread_start)
>  L (thread_start):
> +	/* Terminate call stack by noting ra is undefined.  Use a dummy
> +	   .cfi_label to force starting the FDE.  */
> +	.cfi_label .Ldummy
> +	cfi_undefined (ra)
> +
>  	/* Restore the arg for user's function.  */
>  	REG_L		a1,0(sp)	/* Function pointer.  */
>  	REG_L		a0,SZREG(sp)	/* Argument pointer.  */

Why not use cfi_startproc/cfi_endproc instead of the cfi_label?

Cheers,

Mark