Re: [PING] [PATCH] gdb/arm: Fix backtrace for pthread_cond_timedwait

[Luis Machado <luis.machado@arm.com>]

Hi Jan,

Sorry, I didn't spot the patch on the list.

On 1/16/23 16:10, Jan Kratochvil wrote:
> GDB expected PC should point right after the SVC instruction when the
> syscall is active. But some active syscalls keep PC pointing to the SVC
> instruction itself.
> 
> This leads to a broken backtrace like:
>   Backtrace stopped: previous frame identical to this frame (corrupt stack?)
>   #0  0xb6f8681c in pthread_cond_timedwait@@GLIBC_2.4 () from /lib/arm-linux-gnueabihf/libpthread.so.0
>   #1  0xb6e21f80 in ?? ()
> 
> The reason is that .ARM.exidx unwinder gives up if PC does not point
> right after the SVC (syscall) instruction. I did not investigate why but
> some syscalls will point PC to the SVC instruction itself. This happens
> for the "futex" syscall used by pthread_cond_timedwait.
> 
> That normally does not matter as ARM prologue unwinder gets called
> instead of the .ARM.exidx one. Unfortunately some glibc calls have more
> complicated prologue where the GDB unwinder fails to properly determine
> the return address (that is in fact an orthogonal GDB bug). I expect it
> is due to the "vpush" there in this case but I did not investigate it more:
> 
> Dump of assembler code for function pthread_cond_timedwait@@GLIBC_2.4:
>     0xb6f8757c <+0>:     push    {r4, r5, r6, r7, r8, r9, r10, r11, lr}
>     0xb6f87580 <+4>:     mov     r10, r2
>     0xb6f87584 <+8>:     vpush   {d8}
> 
> Regression tested on armv7l kernel 5.15.32-v7l+ (Raspbian 11).
> ---
>   gdb/arm-tdep.c                                |  42 ++++---
>   .../gdb.arch/arm-pthread_cond_timedwait-bt.c  |  62 +++++++++++
>   .../arm-pthread_cond_timedwait-bt.exp         | 105 ++++++++++++++++++
>   3 files changed, 192 insertions(+), 17 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.c
>   create mode 100644 gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.exp
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 78a2fe2ade5..328eb17f419 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3116,26 +3116,34 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self,
>   	  && get_frame_type (get_next_frame (this_frame)) == NORMAL_FRAME)
>   	exc_valid = 1;
>   
> -      /* We also assume exception information is valid if we're currently
> -	 blocked in a system call.  The system library is supposed to
> -	 ensure this, so that e.g. pthread cancellation works.  */
> -      if (arm_frame_is_thumb (this_frame))
> +      /* Some syscalls keep PC pointing to the SVC instruction itself.  */
> +      for (int shift = 0; shift <= 1 && !exc_valid; ++shift)
>   	{
> -	  ULONGEST insn;
> +	  /* We also assume exception information is valid if we're currently
> +	     blocked in a system call.  The system library is supposed to
> +	     ensure this, so that e.g. pthread cancellation works.  */
> +	  if (arm_frame_is_thumb (this_frame))
> +	    {
> +	      ULONGEST insn;
>   
> -	  if (safe_read_memory_unsigned_integer (get_frame_pc (this_frame) - 2,
> -						 2, byte_order_for_code, &insn)
> -	      && (insn & 0xff00) == 0xdf00 /* svc */)
> -	    exc_valid = 1;
> -	}
> -      else
> -	{
> -	  ULONGEST insn;
> +	      if (safe_read_memory_unsigned_integer ((get_frame_pc (this_frame)
> +						      - (shift ? 2 : 0)),
> +						     2, byte_order_for_code,
> +						     &insn)
> +		  && (insn & 0xff00) == 0xdf00 /* svc */)
> +		exc_valid = 1;
> +	    }
> +	  else
> +	    {
> +	      ULONGEST insn;
>   
> -	  if (safe_read_memory_unsigned_integer (get_frame_pc (this_frame) - 4,
> -						 4, byte_order_for_code, &insn)
> -	      && (insn & 0x0f000000) == 0x0f000000 /* svc */)
> -	    exc_valid = 1;
> +	      if (safe_read_memory_unsigned_integer ((get_frame_pc (this_frame)
> +						      - (shift ? 4 : 0)),
> +						     4, byte_order_for_code,
> +						     &insn)
> +		  && (insn & 0x0f000000) == 0x0f000000 /* svc */)
> +		exc_valid = 1;
> +	    }
>   	}

The changes and reasoning sound good to me, though the test doesn't seem to be doing much on my setup.

I see the same results with a patched and unpatched gdb:

PASS: gdb.arch/arm-pthread_cond_timedwait-bt.exp: successfully compiled posix threads test case
PASS: gdb.arch/arm-pthread_cond_timedwait-bt.exp: advance to break-line
PASS: gdb.arch/arm-pthread_cond_timedwait-bt.exp: thread 2 for svc check
UNTESTED: gdb.arch/arm-pthread_cond_timedwait-bt.exp: pc points to svc

Do you see something different?

>   	
>         /* Bail out if we don't know that exception information is valid.  */
> diff --git a/gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.c b/gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.c
> new file mode 100644
> index 00000000000..c382a100afd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.c
> @@ -0,0 +1,62 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <sys/time.h>
> +#include <unistd.h>
> +#include <assert.h>
> +
> +static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> +
> +static void *fun(void *arg)
> +{
> +  struct timeval now;
> +  struct timespec until;
> +  int err;
> +
> +  err = gettimeofday(&now, NULL);
> +  assert(!err);
> +
> +  until.tv_sec = now.tv_sec + 60;
> +  until.tv_nsec = now.tv_usec * 1000UL;
> +
> +  pthread_cond_timedwait(&cond, &mutex, &until);
> +  assert(0);
> +  err = pthread_mutex_unlock(&mutex);
> +  assert(!err);
> +
> +  return arg;
> +}
> +
> +int main()
> +{
> +  pthread_t thread;
> +  void *ret;
> +  int err;
> +
> +  err = pthread_mutex_lock(&mutex);
> +  assert(!err);
> +  err = pthread_create(&thread, NULL, fun, NULL);
> +  assert(!err);
> +  err = pthread_mutex_lock(&mutex);
> +  assert(!err);
> +  err = pthread_join(thread, &ret); // break-line
> +  assert(0);
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.exp b/gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.exp
> new file mode 100644
> index 00000000000..d5e594c73d5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.exp
> @@ -0,0 +1,105 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This serves as a template for writing new test cases.  Replace this with
> +# a description of what this test case tests.
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +			  {debug pthreads}] } {
> +    return
> +}
> +
> +if { ![runto_main] } {
> +    return
> +}
> +
> +gdb_test "advance [gdb_get_line_number "break-line"]" "break-line.*" \
> +	 "advance to break-line"
> +
> +gdb_test "thread 2" "Switching to thread 2 .*" "thread 2 for svc check"
> +
> +# GDB expected PC should point right after the SVC instruction when the syscall is active.
> +# But some active syscalls keep PC pointing to the SVC instruction itself.
> +set test "pc points to svc"
> +gdb_test_multiple {x/i $pc} $test {
> +    -re ":\tsvc\t0x00000000\r\n$gdb_prompt $" {
> +	pass $test
> +    }
> +    -re "\r\n$gdb_prompt $" {
> +	untested $test
> +	return
> +    }
> +}
> +
> +gdb_test "thread 1" "Switching to thread 1 .*"
> +gdb_test_no_output "set debug frame 1"
> +
> +# PASS:
> +#  [frame] frame_unwind_try_unwinder: trying unwinder "arm exidx"
> +#  [frame] frame_unwind_register_value: enter
> +#...
> +#  [frame] frame_unwind_register_value: exit
> +#  [frame] frame_unwind_try_unwinder: yes
> +#...
> +#  [frame] get_prev_frame_always_1:   -> {level=0,type=NORMAL_FRAME,unwinder="arm exidx",pc=0xb6f8681c,id=<not computed>,func=<unknown>} // cached
> +
> +# FAIL:
> +#  [frame] frame_unwind_try_unwinder: trying unwinder "arm exidx"
> +#  [frame] frame_unwind_register_value: enter
> +#...
> +#  [frame] frame_unwind_register_value: exit
> +#  [frame] frame_unwind_try_unwinder: no
> +#  [frame] frame_unwind_try_unwinder: trying unwinder "arm epilogue"
> +#  [frame] frame_unwind_register_value: enter
> +#...
> +#  [frame] frame_unwind_register_value: exit
> +#  [frame] frame_unwind_try_unwinder: no
> +#  [frame] frame_unwind_try_unwinder: trying unwinder "arm prologue"
> +#  [frame] frame_unwind_try_unwinder: yes
> +#...
> +#  [frame] get_prev_frame_always_1:   -> {level=0,type=NORMAL_FRAME,unwinder="arm prologue",pc=0xb6f8681c,id=<not computed>,func=<unknown>} // cached
> +
> +set test "unwinder is arm exidx"
> +# Switch the threads to reset frame cache.
> +gdb_test_multiple {thread 2} $test {
> +    -re "\{level=0,type=NORMAL_FRAME,unwinder=\"arm exidx\",pc=.*\r\n$gdb_prompt $" {
> +	pass $test
> +    }
> +    -re "\{level=0,type=NORMAL_FRAME,unwinder=\"arm prologue\",pc=.*\r\n$gdb_prompt $" {
> +	fail $test
> +    }
> +    -re "\r\n$gdb_prompt $" {
> +	untested $test
> +	return
> +    }
> +}
> +
> +gdb_test "thread 2" "Switching to thread 2 .*" "thread 2 for debug frame check"
> +
> +gdb_test_no_output "set debug frame 0"
> +
> +# PASS:
> +# #0  0xb6f8681c in pthread_cond_timedwait@@GLIBC_2.4 () from /lib/arm-linux-gnueabihf/libpthread.so.0
> +# #1  0x00010648 in fun (arg=0x0) at .../gdb/testsuite/gdb.arch/arm-pthread_cond_timedwait-bt.c:38
> +# ...
> +
> +# FAIL:
> +# #0  0xb6f8681c in pthread_cond_timedwait@@GLIBC_2.4 () from /lib/arm-linux-gnueabihf/libpthread.so.0
> +# #1  0xb6e21f80 in ?? ()
> +# Backtrace stopped: previous frame identical to this frame (corrupt stack?)
> +
> +gdb_test "bt" { in fun \(arg=.*} "unwind of pthread_cond_timedwait"