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

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

[Jim Wilson]

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.

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?

Jim

	[BZ #24040] 
	* elf/Makefile (CFLAGS-tst-unwind-main.c): Add -DUSE_PTHREADS=0.
	* elf/tst-unwind-main.c: If USE_PTHEADS, include pthread.h and error.h
	(func): New.
	(main): If USE_PTHREADS, call pthread_create to run func.  Otherwise
	call func directly.
	* nptl/Makefile (tests): Add tst-unwind-thread.
	(CFLAGS-tst-unwind-thread.c): Define.
	* nptl/tst-unwind-thread.c: New file.
	* sysdeps/unix/sysv/linux/riscv/clone.S (__thread_start): Mark ra
	as undefined.
---
 elf/Makefile                          |  2 +-
 elf/tst-unwind-main.c                 | 28 ++++++++++++++++++++++++---
 nptl/Makefile                         |  4 +++-
 nptl/tst-unwind-thread.c              |  2 ++
 sysdeps/unix/sysv/linux/riscv/clone.S |  5 +++++
 5 files changed, 36 insertions(+), 5 deletions(-)
 create mode 100644 nptl/tst-unwind-thread.c

diff --git a/elf/Makefile b/elf/Makefile
index 9cf5cd8dfd..815bbd2041 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1497,4 +1497,4 @@ $(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so
 
 $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
 
-CFLAGS-tst-unwind-main.c += -funwind-tables
+CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
diff --git a/elf/tst-unwind-main.c b/elf/tst-unwind-main.c
index 4089e7e907..0c345dc31f 100644
--- a/elf/tst-unwind-main.c
+++ b/elf/tst-unwind-main.c
@@ -20,19 +20,41 @@
 #include <unistd.h>
 #include <support/test-driver.h>
 
+#if USE_PTHREADS
+# include <pthread.h>
+# include <error.h>
+#endif
+
 static _Unwind_Reason_Code
 callback (struct _Unwind_Context *ctx, void *arg)
 {
   return _URC_NO_REASON;
 }
 
-int
-main (void)
+static void *
+func (void *a)
 {
   /* Arrange for this test to be killed if _Unwind_Backtrace runs into an
      endless loop.  We cannot use the test driver because the complete
      call chain needs to be compiled with -funwind-tables so that
-     _Unwind_Backtrace is able to reach _start.  */
+     _Unwind_Backtrace is able to reach the start routine.  */
   alarm (DEFAULT_TIMEOUT);
   _Unwind_Backtrace (callback, 0);
+  return a;
+}
+
+int
+main (void)
+{
+#if USE_PTHREADS
+  pthread_t thr;
+  int rc = pthread_create (&thr, NULL, &func, NULL);
+  if (rc)
+    error (1, rc, "pthread_create");
+  rc = pthread_join (thr, NULL);
+  if (rc)
+    error (1, rc, "pthread_join");
+#else
+  func (NULL);
+#endif
 }
diff --git a/nptl/Makefile b/nptl/Makefile
index 340282c6cb..07c2f49235 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -319,7 +319,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 	tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	tst-cnd-timedwait tst-thrd-detach tst-mtx-basic tst-thrd-sleep \
 	tst-mtx-recursive tst-tss-basic tst-call-once tst-mtx-timedlock \
-	tst-rwlock-pwn
+	tst-rwlock-pwn tst-unwind-thread
 
 tests-internal := tst-rwlock19 tst-rwlock20 \
 		  tst-sem11 tst-sem12 tst-sem13 \
@@ -709,6 +709,8 @@ $(objpfx)tst-audit-threads: $(objpfx)tst-audit-threads-mod2.so
 $(objpfx)tst-audit-threads.out: $(objpfx)tst-audit-threads-mod1.so
 tst-audit-threads-ENV = LD_AUDIT=$(objpfx)tst-audit-threads-mod1.so
 
+CFLAGS-tst-unwind-thread.c += -funwind-tables
+
 # The tests here better do not run in parallel
 ifneq ($(filter %tests,$(MAKECMDGOALS)),)
 .NOTPARALLEL:
diff --git a/nptl/tst-unwind-thread.c b/nptl/tst-unwind-thread.c
new file mode 100644
index 0000000000..d5c38e3709
--- /dev/null
+++ b/nptl/tst-unwind-thread.c
@@ -0,0 +1,2 @@
+#define USE_PTHREADS 1
+#include "../elf/tst-unwind-main.c"
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.  */
-- 
2.19.2

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

[Florian Weimer]

* Jim Wilson:

> -int
> -main (void)
> +static void *
> +func (void *a)
>  {
>    /* Arrange for this test to be killed if _Unwind_Backtrace runs into an
>       endless loop.  We cannot use the test driver because the complete
>       call chain needs to be compiled with -funwind-tables so that
> -     _Unwind_Backtrace is able to reach _start.  */
> +     _Unwind_Backtrace is able to reach the start routine.  */
>    alarm (DEFAULT_TIMEOUT);
>    _Unwind_Backtrace (callback, 0);
> +  return a;
> +}
> +
> +int
> +main (void)
> +{
> +#if USE_PTHREADS
> +  pthread_t thr;
> +  int rc = pthread_create (&thr, NULL, &func, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_create");
> +  rc = pthread_join (thr, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_join");
> +#else
> +  func (NULL);
> +#endif

Do you expect func to be inlined, to preserve the pattern of the
original test?

I wonder if it is preferable to have separate, new file for this.

Thanks,
Florian

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

[Jim Wilson]

On Mon, Jan 14, 2019 at 5:38 AM Florian Weimer <fweimer@redhat.com> wrote:
> Do you expect func to be inlined, to preserve the pattern of the
> original test?

It shouldn't be necessary for func to be inlined to preserve the
intent of the original test.  We are just unwinding to reach the end
of the stack, and if func isn't inlined it is just one more function
to unwind through.

Jim

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

[Mark Wielaard]

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

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

[Jim Wilson]

On Thu, Jan 17, 2019 at 9:05 AM Mark Wielaard <mark@klomp.org> wrote:
> 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.

Experimenting a bit, I've figured out that if I use the system
libpthread.so, then the testcase behaves as I expect.  It fails
without the libc.so patch, it works with my libc.so patch.  If I use
the libpthread.so I just built as part of the glibc build, then the
testcase always passes.

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

Trying this, I see that with the system libpthread.so, the backtrace
reaches __thread_start, and then loops infinitely without my patch or
exits with a 0 with my patch.  With the just built libpthread.so, the
backtrace only gets to start_thread, and then exits.  start_thread has
unwind info and a valid return address pointing back to
__thread_start, so it isn't obvious why the unwind stops there.
libpthread does have its own personality routine, maybe something
changed there.  Or maybe one of the missing binutils/gcc/glibc/linux
kernel/etc features we have implemented since the system libpthread.so
was built is causing things to work slightly differently now.  Or
maybe my glibc is configured slightly differently than the system
glibc and that is effecting the result.

I do think the __thead_start change and the new testcase are correct.
Other ports have similar changes to __thread_start, and verifying that
unwind to top of stack in a thread works if a good test to do.

> Why not use cfi_startproc/cfi_endproc instead of the cfi_label?

I used the exact same solution that is already used in the RISC-V
_start function.  I don't care what solution is used here, but I do
think that _start and __thread_start should use the exact same
solution.

Jim

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

[Mark Wielaard]

On Sun, 2019-01-20 at 21:55 -0800, Jim Wilson wrote:
> Or maybe my glibc is configured slightly differently than the
> system glibc and that is effecting the result.

The distro might build with -fasynchronous-unwind-tables which would
make sure that only the CFI unwinder is used, while without it, it
might be that there is a frame that might have to be handled by the
fallback unwinder.

> I do think the __thead_start change and the new testcase are correct.
> Other ports have similar changes to __thread_start, and verifying
> that unwind to top of stack in a thread works is a good test to do.

Agreed,

Mark

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

[Jim Wilson]

On Tue, Jan 22, 2019 at 5:28 AM Mark Wielaard <mark@klomp.org> wrote:
> The distro might build with -fasynchronous-unwind-tables which would
> make sure that only the CFI unwinder is used, while without it, it
> might be that there is a frame that might have to be handled by the
> fallback unwinder.

Thanks.  That indeed turns out to be the problem here.  I see in the
fedora glibc rpm that it is using CFLAGS="-O2 -g
-fasynchronous-unwind-tables".  When I configure and build glibc that
way, the testcase behaves the way I expect.  It fails without my
__thread_start patch and works with my patch.

Jim

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

[Palmer Dabbelt]

On Sun, 13 Jan 2019 15:48:09 PST (-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.
>
> 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?
>
> Jim
>
> 	[BZ #24040]
> 	* elf/Makefile (CFLAGS-tst-unwind-main.c): Add -DUSE_PTHREADS=0.
> 	* elf/tst-unwind-main.c: If USE_PTHEADS, include pthread.h and error.h
> 	(func): New.
> 	(main): If USE_PTHREADS, call pthread_create to run func.  Otherwise
> 	call func directly.
> 	* nptl/Makefile (tests): Add tst-unwind-thread.
> 	(CFLAGS-tst-unwind-thread.c): Define.
> 	* nptl/tst-unwind-thread.c: New file.
> 	* sysdeps/unix/sysv/linux/riscv/clone.S (__thread_start): Mark ra
> 	as undefined.

I'm happy with the fix and can merge that.  I'm not sure if I can actually 
merge the test suite changes, though.

Does anyone oppose me merging the whole patch?

> ---
>  elf/Makefile                          |  2 +-
>  elf/tst-unwind-main.c                 | 28 ++++++++++++++++++++++++---
>  nptl/Makefile                         |  4 +++-
>  nptl/tst-unwind-thread.c              |  2 ++
>  sysdeps/unix/sysv/linux/riscv/clone.S |  5 +++++
>  5 files changed, 36 insertions(+), 5 deletions(-)
>  create mode 100644 nptl/tst-unwind-thread.c
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 9cf5cd8dfd..815bbd2041 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1497,4 +1497,4 @@ $(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so
>
>  $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
>
> -CFLAGS-tst-unwind-main.c += -funwind-tables
> +CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
> diff --git a/elf/tst-unwind-main.c b/elf/tst-unwind-main.c
> index 4089e7e907..0c345dc31f 100644
> --- a/elf/tst-unwind-main.c
> +++ b/elf/tst-unwind-main.c
> @@ -20,19 +20,41 @@
>  #include <unistd.h>
>  #include <support/test-driver.h>
>
> +#if USE_PTHREADS
> +# include <pthread.h>
> +# include <error.h>
> +#endif
> +
>  static _Unwind_Reason_Code
>  callback (struct _Unwind_Context *ctx, void *arg)
>  {
>    return _URC_NO_REASON;
>  }
>
> -int
> -main (void)
> +static void *
> +func (void *a)
>  {
>    /* Arrange for this test to be killed if _Unwind_Backtrace runs into an
>       endless loop.  We cannot use the test driver because the complete
>       call chain needs to be compiled with -funwind-tables so that
> -     _Unwind_Backtrace is able to reach _start.  */
> +     _Unwind_Backtrace is able to reach the start routine.  */
>    alarm (DEFAULT_TIMEOUT);
>    _Unwind_Backtrace (callback, 0);
> +  return a;
> +}
> +
> +int
> +main (void)
> +{
> +#if USE_PTHREADS
> +  pthread_t thr;
> +  int rc = pthread_create (&thr, NULL, &func, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_create");
> +  rc = pthread_join (thr, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_join");
> +#else
> +  func (NULL);
> +#endif
>  }
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 340282c6cb..07c2f49235 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -319,7 +319,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
>  	tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
>  	tst-cnd-timedwait tst-thrd-detach tst-mtx-basic tst-thrd-sleep \
>  	tst-mtx-recursive tst-tss-basic tst-call-once tst-mtx-timedlock \
> -	tst-rwlock-pwn
> +	tst-rwlock-pwn tst-unwind-thread
>
>  tests-internal := tst-rwlock19 tst-rwlock20 \
>  		  tst-sem11 tst-sem12 tst-sem13 \
> @@ -709,6 +709,8 @@ $(objpfx)tst-audit-threads: $(objpfx)tst-audit-threads-mod2.so
>  $(objpfx)tst-audit-threads.out: $(objpfx)tst-audit-threads-mod1.so
>  tst-audit-threads-ENV = LD_AUDIT=$(objpfx)tst-audit-threads-mod1.so
>
> +CFLAGS-tst-unwind-thread.c += -funwind-tables
> +
>  # The tests here better do not run in parallel
>  ifneq ($(filter %tests,$(MAKECMDGOALS)),)
>  .NOTPARALLEL:
> diff --git a/nptl/tst-unwind-thread.c b/nptl/tst-unwind-thread.c
> new file mode 100644
> index 0000000000..d5c38e3709
> --- /dev/null
> +++ b/nptl/tst-unwind-thread.c
> @@ -0,0 +1,2 @@
> +#define USE_PTHREADS 1
> +#include "../elf/tst-unwind-main.c"
> 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.  */

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

[Jim Wilson]

On Wed, Jan 23, 2019 at 6:51 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> I'm happy with the fix and can merge that.  I'm not sure if I can actually
> merge the test suite changes, though.
>
> Does anyone oppose me merging the whole patch?

ping

I need a global glibc maintainer review for the testcase.  Andreas, is
this something you can help with?

Original message:
https://sourceware.org/ml/libc-alpha/2019-01/msg00278.html

Palmer, if no one reviews the testcase, I would suggest just checking
in the RISC-V specific fix, and then I can just put the testcase into
its own bug report and maybe someone will look at it there.  Or maybe
propose me as a glibc RISC-V maintainer and then I will check in the
RISC-V part of the fix myself.  David Abdurachmanov found another
glibc problem, so I probably need to write another glibc patch to fix
it.

Jim

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

[DJ Delorie]

Jim Wilson <jimw@sifive.com> writes:
> ping
>
> I need a global glibc maintainer review for the testcase.  Andreas, is
> this something you can help with?

I was planning on pushing at least the risc-v portion of this after the
freeze.  I can worry about the testcase consensus if you've got other
bugs to debug still, but this week is going to be a difficult week to
get anyone's attention as we're all doing release-related stuff.

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

[Jim Wilson]

On Mon, Jan 28, 2019 at 2:44 PM DJ Delorie <dj@redhat.com> wrote:
>
> Jim Wilson <jimw@sifive.com> writes:
> > ping
> >
> > I need a global glibc maintainer review for the testcase.  Andreas, is
> > this something you can help with?
>
> I was planning on pushing at least the risc-v portion of this after the
> freeze.  I can worry about the testcase consensus if you've got other
> bugs to debug still, but this week is going to be a difficult week to
> get anyone's attention as we're all doing release-related stuff.

OK, thanks, that helps.  I can wait for that.

Jim

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

[DJ Delorie]

I'm OK with this patch plus or minus the return from main comment I
added below.  If someone "in the know" can comment on that comment, I
can commit the patch once we have consensus on it.

Jim Wilson <jimw@sifive.com> writes:
> -CFLAGS-tst-unwind-main.c += -funwind-tables
> +CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0

This allows for the test to run the "old" way, ok.

>  static _Unwind_Reason_Code
>  callback (struct _Unwind_Context *ctx, void *arg)
>  {
>    return _URC_NO_REASON;
>  }
>  
> -int
> -main (void)
> +static void *
> +func (void *a)
>  {
>    /* Arrange for this test to be killed if _Unwind_Backtrace runs into an
>       endless loop.  We cannot use the test driver because the complete
>       call chain needs to be compiled with -funwind-tables so that
> -     _Unwind_Backtrace is able to reach _start.  */
> +     _Unwind_Backtrace is able to reach the start routine.  */
>    alarm (DEFAULT_TIMEOUT);
>    _Unwind_Backtrace (callback, 0);
> +  return a;
> +}
> +
> +int
> +main (void)
> +{
> +#if USE_PTHREADS
> +  pthread_t thr;
> +  int rc = pthread_create (&thr, NULL, &func, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_create");
> +  rc = pthread_join (thr, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_join");
> +#else
> +  func (NULL);
> +#endif
>  }

main() should return something in the fall-through case, be it zero
(main should exit) or 1 (code shouldn't be reached), now that main()
doesn't end with _Unwind_Backtrace() itself, if for no reason than to
avoid future warnings.

> -	tst-rwlock-pwn
> +	tst-rwlock-pwn tst-unwind-thread

ok

> +CFLAGS-tst-unwind-thread.c += -funwind-tables

ok, same as original test.

> +#define USE_PTHREADS 1
> +#include "../elf/tst-unwind-main.c"

Ok.

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

As noted, this matches start.S, so ok.

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

[Jim Wilson]

On Mon, Feb 4, 2019 at 2:36 PM DJ Delorie <dj@redhat.com> wrote:
> main() should return something in the fall-through case, be it zero
> (main should exit) or 1 (code shouldn't be reached), now that main()
> doesn't end with _Unwind_Backtrace() itself, if for no reason than to
> avoid future warnings.

The original testcase doesn't end with _Unwind_Backtrace().
_Unwind_Backtrace returns, and then it falls through the bottom of
main.  Just like my testcase.  I was just following the same style.  I
don't know if that was intentional or not.  By the way, the ISO C
standard allows programs to fall through the bottom of main.  This is
defined as exactly equivalent to returning 0.  Looking at the ISO C
2011 standard, section 5.1.2.2.3 Program termination, "reaching the }
that terminates the main function returns a value of 0".  The testcase
fails if _Unwind_Backtrace() does not return, in which case the alarm
triggers, and the alarm signal handler exits with an error.  If
_Unwind_Backtrace() returns then the testcase passed.  FYI
elf/tst-initorder.c has the same style, a main function without an
explicit return.

Anyways, adding a return 0 at the bottom of main is fine with me, I
write my own code that way, and it doesn't change the effect of the
testcase.

I don't have write access to glibc.  Do you want me to send an updated
patch?  Or just fix it yourself and commit it?

Jim

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

[DJ Delorie]

Jim Wilson <jimw@sifive.com> writes:
> By the way, the ISO C standard allows programs to fall through the
> bottom of main.  This is defined as exactly equivalent to returning 0.

Hmm... that's new in gcc 5, so color me surprised.  As that is glibc's
minimum build version, I retract my comment.

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

[Joseph Myers]

On Mon, 4 Feb 2019, DJ Delorie wrote:

> Jim Wilson <jimw@sifive.com> writes:
> > By the way, the ISO C standard allows programs to fall through the
> > bottom of main.  This is defined as exactly equivalent to returning 0.
> 
> Hmm... that's new in gcc 5, so color me surprised.  As that is glibc's
> minimum build version, I retract my comment.

It's a C99 feature that I implemented for GCC 3.0.

2000-07-27  Joseph S. Myers  <jsm28@cam.ac.uk>

        * c-decl.c (finish_function): Don't treat 'main' specially unless
        flag_hosted.  In C99 mode, return 0 from 'main' unless
        DEFAULT_MAIN_RETURN is otherwise defined.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[DJ Delorie]

Joseph Myers <joseph@codesourcery.com> writes:
>> Hmm... that's new in gcc 5, so color me surprised.  As that is glibc's
>> minimum build version, I retract my comment.
>
> It's a C99 feature that I implemented for GCC 3.0.

Perhaps having it as the default is new in gcc 5 then.  GCC 4 compiles
without that feature by default:

gcc version 4.8.5 20150623 (Red Hat 4.8.5-36) (GCC) 

#include <stdio.h>
#include <stdlib.h>

char * const str;

int
main()
{
  free (str);
}

$ gcc -S test.c

main:
        pushq   %rbp
        movq    %rsp, %rbp
        movq    str(%rip), %rax
        movq    %rax, %rdi
        call    free
        popq    %rbp
        ret

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

[Joseph Myers]

On Mon, 4 Feb 2019, DJ Delorie wrote:

> Joseph Myers <joseph@codesourcery.com> writes:
> >> Hmm... that's new in gcc 5, so color me surprised.  As that is glibc's
> >> minimum build version, I retract my comment.
> >
> > It's a C99 feature that I implemented for GCC 3.0.
> 
> Perhaps having it as the default is new in gcc 5 then.  GCC 4 compiles
> without that feature by default:

Yes, the default standard version changed from -std=gnu89 to -std=gnu11 in 
GCC 5.  (glibc has used explicit -std= options to build for a long time:

2002-10-05  Roland McGrath  <roland@redhat.com>

	[...]
        * Makeconfig (CFLAGS): Prepend -std=gnu99.

(with later changes to add -fgnu89-inline, and to move to -std=gnu11).)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Jim Wilson]

Original message:
https://sourceware.org/ml/libc-alpha/2019-01/msg00278.html

On Sun, Jan 13, 2019 at 3:49 PM Jim Wilson <jimw@sifive.com> 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.
>
> 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?
>
> Jim
>
>         [BZ #24040]
>         * elf/Makefile (CFLAGS-tst-unwind-main.c): Add -DUSE_PTHREADS=0.
>         * elf/tst-unwind-main.c: If USE_PTHEADS, include pthread.h and error.h
>         (func): New.
>         (main): If USE_PTHREADS, call pthread_create to run func.  Otherwise
>         call func directly.
>         * nptl/Makefile (tests): Add tst-unwind-thread.
>         (CFLAGS-tst-unwind-thread.c): Define.
>         * nptl/tst-unwind-thread.c: New file.
>         * sysdeps/unix/sysv/linux/riscv/clone.S (__thread_start): Mark ra
>         as undefined.
> ---
>  elf/Makefile                          |  2 +-
>  elf/tst-unwind-main.c                 | 28 ++++++++++++++++++++++++---
>  nptl/Makefile                         |  4 +++-
>  nptl/tst-unwind-thread.c              |  2 ++
>  sysdeps/unix/sysv/linux/riscv/clone.S |  5 +++++
>  5 files changed, 36 insertions(+), 5 deletions(-)
>  create mode 100644 nptl/tst-unwind-thread.c
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 9cf5cd8dfd..815bbd2041 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1497,4 +1497,4 @@ $(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so
>
>  $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
>
> -CFLAGS-tst-unwind-main.c += -funwind-tables
> +CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
> diff --git a/elf/tst-unwind-main.c b/elf/tst-unwind-main.c
> index 4089e7e907..0c345dc31f 100644
> --- a/elf/tst-unwind-main.c
> +++ b/elf/tst-unwind-main.c
> @@ -20,19 +20,41 @@
>  #include <unistd.h>
>  #include <support/test-driver.h>
>
> +#if USE_PTHREADS
> +# include <pthread.h>
> +# include <error.h>
> +#endif
> +
>  static _Unwind_Reason_Code
>  callback (struct _Unwind_Context *ctx, void *arg)
>  {
>    return _URC_NO_REASON;
>  }
>
> -int
> -main (void)
> +static void *
> +func (void *a)
>  {
>    /* Arrange for this test to be killed if _Unwind_Backtrace runs into an
>       endless loop.  We cannot use the test driver because the complete
>       call chain needs to be compiled with -funwind-tables so that
> -     _Unwind_Backtrace is able to reach _start.  */
> +     _Unwind_Backtrace is able to reach the start routine.  */
>    alarm (DEFAULT_TIMEOUT);
>    _Unwind_Backtrace (callback, 0);
> +  return a;
> +}
> +
> +int
> +main (void)
> +{
> +#if USE_PTHREADS
> +  pthread_t thr;
> +  int rc = pthread_create (&thr, NULL, &func, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_create");
> +  rc = pthread_join (thr, NULL);
> +  if (rc)
> +    error (1, rc, "pthread_join");
> +#else
> +  func (NULL);
> +#endif
>  }
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 340282c6cb..07c2f49235 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -319,7 +319,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
>         tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
>         tst-cnd-timedwait tst-thrd-detach tst-mtx-basic tst-thrd-sleep \
>         tst-mtx-recursive tst-tss-basic tst-call-once tst-mtx-timedlock \
> -       tst-rwlock-pwn
> +       tst-rwlock-pwn tst-unwind-thread
>
>  tests-internal := tst-rwlock19 tst-rwlock20 \
>                   tst-sem11 tst-sem12 tst-sem13 \
> @@ -709,6 +709,8 @@ $(objpfx)tst-audit-threads: $(objpfx)tst-audit-threads-mod2.so
>  $(objpfx)tst-audit-threads.out: $(objpfx)tst-audit-threads-mod1.so
>  tst-audit-threads-ENV = LD_AUDIT=$(objpfx)tst-audit-threads-mod1.so
>
> +CFLAGS-tst-unwind-thread.c += -funwind-tables
> +
>  # The tests here better do not run in parallel
>  ifneq ($(filter %tests,$(MAKECMDGOALS)),)
>  .NOTPARALLEL:
> diff --git a/nptl/tst-unwind-thread.c b/nptl/tst-unwind-thread.c
> new file mode 100644
> index 0000000000..d5c38e3709
> --- /dev/null
> +++ b/nptl/tst-unwind-thread.c
> @@ -0,0 +1,2 @@
> +#define USE_PTHREADS 1
> +#include "../elf/tst-unwind-main.c"
> 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.  */
> --
> 2.19.2
>

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

[Andreas Schwab]

On Jan 13 2019, Jim Wilson <jimw@sifive.com> wrote:

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

You need to point libthread-db-search-path to the nptl_db directory so
that gdb is able to find the correct libthread_db.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Andreas Schwab]

On Jan 13 2019, Jim Wilson <jimw@sifive.com> wrote:

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

I don't see that here, with the current tools from openSUSE Factory.
There many be some (bad?) luck involved, since it's undefined territory.

> 	[BZ #24040] 
> 	* elf/Makefile (CFLAGS-tst-unwind-main.c): Add -DUSE_PTHREADS=0.
> 	* elf/tst-unwind-main.c: If USE_PTHEADS, include pthread.h and error.h
> 	(func): New.
> 	(main): If USE_PTHREADS, call pthread_create to run func.  Otherwise
> 	call func directly.
> 	* nptl/Makefile (tests): Add tst-unwind-thread.
> 	(CFLAGS-tst-unwind-thread.c): Define.
> 	* nptl/tst-unwind-thread.c: New file.
> 	* sysdeps/unix/sysv/linux/riscv/clone.S (__thread_start): Mark ra
> 	as undefined.

Ok.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Palmer Dabbelt]

On Wed, 13 Feb 2019 08:33:34 PST (-0800), schwab@suse.de wrote:
> On Jan 13 2019, Jim Wilson <jimw@sifive.com> wrote:
>
>> 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.
>
> I don't see that here, with the current tools from openSUSE Factory.
> There many be some (bad?) luck involved, since it's undefined territory.
>
>> 	[BZ #24040]
>> 	* elf/Makefile (CFLAGS-tst-unwind-main.c): Add -DUSE_PTHREADS=0.
>> 	* elf/tst-unwind-main.c: If USE_PTHEADS, include pthread.h and error.h
>> 	(func): New.
>> 	(main): If USE_PTHREADS, call pthread_create to run func.  Otherwise
>> 	call func directly.
>> 	* nptl/Makefile (tests): Add tst-unwind-thread.
>> 	(CFLAGS-tst-unwind-thread.c): Define.
>> 	* nptl/tst-unwind-thread.c: New file.
>> 	* sysdeps/unix/sysv/linux/riscv/clone.S (__thread_start): Mark ra
>> 	as undefined.
>
> Ok.

Thanks.  I committed this.

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

[Andreas Schwab]

Please don't forget to update the bug page.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."