public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions
@ 2021-08-10 22:37 Lancelot SIX
  2021-08-11 15:53 ` Andrew Burgess
  0 siblings, 1 reply; 6+ messages in thread
From: Lancelot SIX @ 2021-08-10 22:37 UTC (permalink / raw)
  To: gdb-patches; +Cc: Lancelot SIX, Andrew Burgess

Noticeable changes from V1:

- Following review from Andrew Burgess, add support for LW.
- Explicitly include C.LD and C.LW in the testcase.

---

While working on the testsuite, I ended up noticing that GDB fails to
produce a full backtrace from a thread waiting in pthread_join.  When
selecting the waiting thread and using the 'bt' command, the following
result can be observed:

	(gdb) bt
	#0  0x0000003ff7fccd20 in __futex_abstimed_wait_common64 () from /lib/riscv64-linux-gnu/libpthread.so.0
	#1  0x0000003ff7fc43da in __pthread_clockjoin_ex () from /lib/riscv64-linux-gnu/libpthread.so.0
	Backtrace stopped: frame did not save the PC

On my platform, I do not have debug symbols for glibc, so I need to rely
on prologue analysis in order to unwind stack.

Here is what the function prologue looks like:

	(gdb) disassemble __pthread_clockjoin_ex
	Dump of assembler code for function __pthread_clockjoin_ex:
	   0x0000003ff7fc42de <+0>:     addi    sp,sp,-144
	   0x0000003ff7fc42e0 <+2>:     sd      s5,88(sp)
	   0x0000003ff7fc42e2 <+4>:     auipc   s5,0xd
	   0x0000003ff7fc42e6 <+8>:     ld      s5,-2(s5) # 0x3ff7fd12e0
	   0x0000003ff7fc42ea <+12>:    ld      a5,0(s5)
	   0x0000003ff7fc42ee <+16>:    sd      ra,136(sp)
	   0x0000003ff7fc42f0 <+18>:    sd      s0,128(sp)
	   0x0000003ff7fc42f2 <+20>:    sd      s1,120(sp)
	   0x0000003ff7fc42f4 <+22>:    sd      s2,112(sp)
	   0x0000003ff7fc42f6 <+24>:    sd      s3,104(sp)
	   0x0000003ff7fc42f8 <+26>:    sd      s4,96(sp)
	   0x0000003ff7fc42fa <+28>:    sd      s6,80(sp)
	   0x0000003ff7fc42fc <+30>:    sd      s7,72(sp)
	   0x0000003ff7fc42fe <+32>:    sd      s8,64(sp)
	   0x0000003ff7fc4300 <+34>:    sd      s9,56(sp)
	   0x0000003ff7fc4302 <+36>:    sd      a5,40(sp)

As far as prologue analysis is concerned, the most interesting part is
done at address 0x0000003ff7fc42ee (<+16>): 'sd ra,136(sp)'. This stores
the RA (return address) register on the stack, which is the information
we are looking for in order to identify the caller.

In the current implementation of the prologue scanner, GDB stops when
hitting 0x0000003ff7fc42e6 (<+8>) because it does not know what to do
with the 'ld' instruction.  GDB thinks it reached the end of the
prologue but have not yet reached the important part, which explain
GDB's inability to unwind past this point.

The section of the prologue starting at <+4> until <+12> is used to load
the stack canary[1], which will then be placed on the stack at <+36> at
the end of the prologue.

In order to have the prologue properly handled, this commit proposes to
add support for the ld instruction in the RISC-V prologue scanner.
I guess riscv32 would use lw in such situation so this patch also adds
support for this instruction.

With this patch applied, gdb is now able to unwind past pthread_join:

	(gdb) bt
	#0  0x0000003ff7fccd20 in __futex_abstimed_wait_common64 () from /lib/riscv64-linux-gnu/libpthread.so.0
	#1  0x0000003ff7fc43da in __pthread_clockjoin_ex () from /lib/riscv64-linux-gnu/libpthread.so.0
	#2  0x0000002aaaaaa88e in bar() ()
	#3  0x0000002aaaaaa8c4 in foo() ()
	#4  0x0000002aaaaaa8da in main ()

I have had a look to see if I could reproduce this easily, but in my
simple testcases using '-fstack-protector-all', the canary is loaded
after the RA register is saved.  I do not have a reliable way of
generating a prologue similar to the problematic one so I forged one
instead.

The testsuite have been run on riscv64 ubuntu 21.01 with no regression
observed.

[1] https://en.wikipedia.org/wiki/Buffer_overflow_protection#Canaries
---
 gdb/riscv-tdep.c                              | 33 +++++++++
 .../riscv64-unwind-prologue-with-ld-lw-foo.s  | 74 +++++++++++++++++++
 .../riscv64-unwind-prologue-with-ld-lw.exp    | 45 +++++++++++
 .../riscv64-unwind-prologue-with-ld.c         | 30 ++++++++
 4 files changed, 182 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s
 create mode 100644 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
 create mode 100644 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index b5b0d2d79de..8b55bc33ded 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1409,6 +1409,8 @@ class riscv_insn
       LUI,
       SD,
       SW,
+      LD,
+      LW,
       /* These are needed for software breakpoint support.  */
       JAL,
       JALR,
@@ -1519,6 +1521,15 @@ class riscv_insn
     m_imm.s = EXTRACT_CITYPE_IMM (ival);
   }
 
+  /* Helper for DECODE, decode 16-bit compressed CL-type instruction.  */
+  void decode_cl_type_insn (enum opcode opcode, ULONGEST ival)
+  {
+    m_opcode = opcode;
+    m_rd = decode_register_index_short (ival, OP_SH_CRS2S);
+    m_rs1 = decode_register_index_short (ival, OP_SH_CRS1S);
+    m_imm.s = EXTRACT_CLTYPE_IMM (ival);
+  }
+
   /* Helper for DECODE, decode 32-bit S-type instruction.  */
   void decode_s_type_insn (enum opcode opcode, ULONGEST ival)
   {
@@ -1715,6 +1726,10 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
 	decode_r_type_insn (SC, ival);
       else if (is_ecall_insn (ival))
 	decode_i_type_insn (ECALL, ival);
+      else if (is_ld_insn (ival))
+	decode_i_type_insn (LD, ival);
+      else if (is_lw_insn (ival))
+	decode_i_type_insn (LW, ival);
       else
 	/* None of the other fields are valid in this case.  */
 	m_opcode = OTHER;
@@ -1783,6 +1798,10 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
 	decode_cb_type_insn (BEQ, ival);
       else if (is_c_bnez_insn (ival))
 	decode_cb_type_insn (BNE, ival);
+      else if (is_c_ld_insn (ival))
+	decode_cl_type_insn (LD, ival);
+      else if (is_c_lw_insn (ival))
+	decode_cl_type_insn (LW, ival);
       else
 	/* None of the other fields of INSN are valid in this case.  */
 	m_opcode = OTHER;
@@ -1931,6 +1950,20 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
 	  gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
 	  regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]);
 	}
+      else if (insn.opcode () == riscv_insn::LD
+	       || insn.opcode () == riscv_insn::LW)
+	{
+	  /* Handle: ld reg, offset(rs1)
+	     or:     c.ld reg, offset(rs1)
+	     or:     lw reg, offset(rs1)
+	     or:     c.lw reg, offset(rs1)  */
+	  gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
+	  gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
+	  regs[insn.rd ()]
+	    = stack.fetch (pv_add_constant (regs[insn.rs1 ()],
+					    insn.imm_signed ()),
+			   (insn.opcode () == riscv_insn::LW ? 4 : 8));
+	}
       else
 	{
 	  end_prologue_addr = cur_pc;
diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s
new file mode 100644
index 00000000000..ebc27ff1e8c
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s
@@ -0,0 +1,74 @@
+/* Copyright 2021 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 testcase contains a function where the 'ld', 'c.ld', 'lw' or 'c.lw'
+   instruction is used in the prologue before the RA register have been saved
+   on the stack.
+
+   This mimics a pattern observed in the __pthread_clockjoin_ex function
+   in libpthread.so.0 (from glibc-2.33-0ubuntu5) where a canary value is
+   loaded and placed on the stack in order to detect stack smashing.
+
+   The skeleton for this file was generated using the following command:
+
+      gcc -x c -S -c -o - - <<EOT
+        static long int __canary = 42;
+        extern int bar ();
+        int foo () { return bar(); }
+      EOT
+
+   The result of this command is modified in the following way:
+     - The prologue is adapted to reserve 16 more bytes on the stack.
+     - A part that simulates the installation of a canary on the stack is
+       added.  The canary is loaded multiple times to simulate the use of
+       various instructions that could do the work (ld or c.ld for a 64 bit
+       canary, lw or c.lw for a 32 bit canary).
+     - The epilogue is adjusted to be able to return properly.  The epilogue
+       does not check the canary value since this testcase is only interested
+       in ensuring GDB can scan the prologue.  */
+
+	.option pic
+	.text
+	.data
+	.align	3
+	.type	__canary, @object
+	.size	__canary, 8
+__canary:
+	.dword	42
+	.text
+	.align	1
+	.globl	foo
+	.type	foo, @function
+foo:
+	addi	sp,sp,-32
+	lla	a5,__canary  # Load the fake canary address.
+	lw	t4,0(a5)     # Load a 32 bit canary (use t4 to force the use of
+			     # the non compressed instruction).
+	ld	t4,0(a5)     # Load a 64 bit canary (use t4 to force the use of
+			     # the non compressed instruction).
+	c.lw 	a4,0(a5)     # Load a 32 bit canary using the compressed insn.
+	c.ld 	a4,0(a5)     # Load a 64 bit canary using the compressed insn.
+	sd	a4,0(sp)     # Place the fake canary on the stack.
+	sd	ra,16(sp)
+	sd	s0,8(sp)
+	addi	s0,sp,32
+	call	bar@plt
+	mv	a5,a0
+	mv	a0,a5
+	ld	ra,16(sp)
+	ld	s0,8(sp)
+	addi	sp,sp,32
+	jr	ra
+	.size	foo, .-foo
diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
new file mode 100644
index 00000000000..c329d2f72e0
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
@@ -0,0 +1,45 @@
+# Copyright 2021 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 tests GDB's ability to use the RISC-V prologue scanner in order to
+# unwind through a function that uses the 'ld' instruction in its prologue.
+
+if {![istarget "riscv64-*-*"]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile riscv64-unwind-prologue-with-ld.c \
+		  riscv64-unwind-prologue-with-ld-lw-foo.s
+if {[prepare_for_testing "failed to prepare" $testfile \
+			 "$srcfile $srcfile2"  nodebug]} {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "can't run to main"
+    return 0
+}
+
+gdb_breakpoint "bar"
+gdb_continue_to_breakpoint "bar"
+gdb_test "bt" \
+    [multi_line \
+         "#0\[ \t\]*$hex in bar \\\(\\\)" \
+         "#1\[ \t\]*$hex in foo \\\(\\\)" \
+         "#2\[ \t\]*$hex in main \\\(\\\)"] \
+    "Backtrace to the main frame"
+gdb_test "finish" "foo \\\(\\\)" "finish bar"
+gdb_test "finish" "main \\\(\\\)" "finish foo"
diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c
new file mode 100644
index 00000000000..9ff950df273
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c
@@ -0,0 +1,30 @@
+/* Copyright 2021 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/>.  */
+
+/* See riscv64-unwind-prologue-with-ld-foo.s for implementation.  */
+extern int foo (void);
+
+int
+bar ()
+{
+  return 0;
+}
+
+int
+main ()
+{
+  return foo ();
+}
+
-- 
2.30.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions
  2021-08-10 22:37 [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions Lancelot SIX
@ 2021-08-11 15:53 ` Andrew Burgess
  2021-08-12 23:26   ` Lancelot SIX
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2021-08-11 15:53 UTC (permalink / raw)
  To: Lancelot SIX; +Cc: gdb-patches

* Lancelot SIX <lsix@lancelotsix.com> [2021-08-10 22:37:54 +0000]:

> Noticeable changes from V1:
> 
> - Following review from Andrew Burgess, add support for LW.
> - Explicitly include C.LD and C.LW in the testcase.
> 
> ---
> 
> While working on the testsuite, I ended up noticing that GDB fails to
> produce a full backtrace from a thread waiting in pthread_join.  When
> selecting the waiting thread and using the 'bt' command, the following
> result can be observed:
> 
> 	(gdb) bt
> 	#0  0x0000003ff7fccd20 in __futex_abstimed_wait_common64 () from /lib/riscv64-linux-gnu/libpthread.so.0
> 	#1  0x0000003ff7fc43da in __pthread_clockjoin_ex () from /lib/riscv64-linux-gnu/libpthread.so.0
> 	Backtrace stopped: frame did not save the PC
> 
> On my platform, I do not have debug symbols for glibc, so I need to rely
> on prologue analysis in order to unwind stack.
> 
> Here is what the function prologue looks like:
> 
> 	(gdb) disassemble __pthread_clockjoin_ex
> 	Dump of assembler code for function __pthread_clockjoin_ex:
> 	   0x0000003ff7fc42de <+0>:     addi    sp,sp,-144
> 	   0x0000003ff7fc42e0 <+2>:     sd      s5,88(sp)
> 	   0x0000003ff7fc42e2 <+4>:     auipc   s5,0xd
> 	   0x0000003ff7fc42e6 <+8>:     ld      s5,-2(s5) # 0x3ff7fd12e0
> 	   0x0000003ff7fc42ea <+12>:    ld      a5,0(s5)
> 	   0x0000003ff7fc42ee <+16>:    sd      ra,136(sp)
> 	   0x0000003ff7fc42f0 <+18>:    sd      s0,128(sp)
> 	   0x0000003ff7fc42f2 <+20>:    sd      s1,120(sp)
> 	   0x0000003ff7fc42f4 <+22>:    sd      s2,112(sp)
> 	   0x0000003ff7fc42f6 <+24>:    sd      s3,104(sp)
> 	   0x0000003ff7fc42f8 <+26>:    sd      s4,96(sp)
> 	   0x0000003ff7fc42fa <+28>:    sd      s6,80(sp)
> 	   0x0000003ff7fc42fc <+30>:    sd      s7,72(sp)
> 	   0x0000003ff7fc42fe <+32>:    sd      s8,64(sp)
> 	   0x0000003ff7fc4300 <+34>:    sd      s9,56(sp)
> 	   0x0000003ff7fc4302 <+36>:    sd      a5,40(sp)
> 
> As far as prologue analysis is concerned, the most interesting part is
> done at address 0x0000003ff7fc42ee (<+16>): 'sd ra,136(sp)'. This stores
> the RA (return address) register on the stack, which is the information
> we are looking for in order to identify the caller.
> 
> In the current implementation of the prologue scanner, GDB stops when
> hitting 0x0000003ff7fc42e6 (<+8>) because it does not know what to do
> with the 'ld' instruction.  GDB thinks it reached the end of the
> prologue but have not yet reached the important part, which explain
> GDB's inability to unwind past this point.
> 
> The section of the prologue starting at <+4> until <+12> is used to load
> the stack canary[1], which will then be placed on the stack at <+36> at
> the end of the prologue.
> 
> In order to have the prologue properly handled, this commit proposes to
> add support for the ld instruction in the RISC-V prologue scanner.
> I guess riscv32 would use lw in such situation so this patch also adds
> support for this instruction.
> 
> With this patch applied, gdb is now able to unwind past pthread_join:
> 
> 	(gdb) bt
> 	#0  0x0000003ff7fccd20 in __futex_abstimed_wait_common64 () from /lib/riscv64-linux-gnu/libpthread.so.0
> 	#1  0x0000003ff7fc43da in __pthread_clockjoin_ex () from /lib/riscv64-linux-gnu/libpthread.so.0
> 	#2  0x0000002aaaaaa88e in bar() ()
> 	#3  0x0000002aaaaaa8c4 in foo() ()
> 	#4  0x0000002aaaaaa8da in main ()
> 
> I have had a look to see if I could reproduce this easily, but in my
> simple testcases using '-fstack-protector-all', the canary is loaded
> after the RA register is saved.  I do not have a reliable way of
> generating a prologue similar to the problematic one so I forged one
> instead.
> 
> The testsuite have been run on riscv64 ubuntu 21.01 with no regression
> observed.

LGTM.

Thanks for doing this.

Andrew


> 
> [1] https://en.wikipedia.org/wiki/Buffer_overflow_protection#Canaries
> ---
>  gdb/riscv-tdep.c                              | 33 +++++++++
>  .../riscv64-unwind-prologue-with-ld-lw-foo.s  | 74 +++++++++++++++++++
>  .../riscv64-unwind-prologue-with-ld-lw.exp    | 45 +++++++++++
>  .../riscv64-unwind-prologue-with-ld.c         | 30 ++++++++
>  4 files changed, 182 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s
>  create mode 100644 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
>  create mode 100644 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index b5b0d2d79de..8b55bc33ded 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1409,6 +1409,8 @@ class riscv_insn
>        LUI,
>        SD,
>        SW,
> +      LD,
> +      LW,
>        /* These are needed for software breakpoint support.  */
>        JAL,
>        JALR,
> @@ -1519,6 +1521,15 @@ class riscv_insn
>      m_imm.s = EXTRACT_CITYPE_IMM (ival);
>    }
>  
> +  /* Helper for DECODE, decode 16-bit compressed CL-type instruction.  */
> +  void decode_cl_type_insn (enum opcode opcode, ULONGEST ival)
> +  {
> +    m_opcode = opcode;
> +    m_rd = decode_register_index_short (ival, OP_SH_CRS2S);
> +    m_rs1 = decode_register_index_short (ival, OP_SH_CRS1S);
> +    m_imm.s = EXTRACT_CLTYPE_IMM (ival);
> +  }
> +
>    /* Helper for DECODE, decode 32-bit S-type instruction.  */
>    void decode_s_type_insn (enum opcode opcode, ULONGEST ival)
>    {
> @@ -1715,6 +1726,10 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
>  	decode_r_type_insn (SC, ival);
>        else if (is_ecall_insn (ival))
>  	decode_i_type_insn (ECALL, ival);
> +      else if (is_ld_insn (ival))
> +	decode_i_type_insn (LD, ival);
> +      else if (is_lw_insn (ival))
> +	decode_i_type_insn (LW, ival);
>        else
>  	/* None of the other fields are valid in this case.  */
>  	m_opcode = OTHER;
> @@ -1783,6 +1798,10 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
>  	decode_cb_type_insn (BEQ, ival);
>        else if (is_c_bnez_insn (ival))
>  	decode_cb_type_insn (BNE, ival);
> +      else if (is_c_ld_insn (ival))
> +	decode_cl_type_insn (LD, ival);
> +      else if (is_c_lw_insn (ival))
> +	decode_cl_type_insn (LW, ival);
>        else
>  	/* None of the other fields of INSN are valid in this case.  */
>  	m_opcode = OTHER;
> @@ -1931,6 +1950,20 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>  	  gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
>  	  regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]);
>  	}
> +      else if (insn.opcode () == riscv_insn::LD
> +	       || insn.opcode () == riscv_insn::LW)
> +	{
> +	  /* Handle: ld reg, offset(rs1)
> +	     or:     c.ld reg, offset(rs1)
> +	     or:     lw reg, offset(rs1)
> +	     or:     c.lw reg, offset(rs1)  */
> +	  gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> +	  gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> +	  regs[insn.rd ()]
> +	    = stack.fetch (pv_add_constant (regs[insn.rs1 ()],
> +					    insn.imm_signed ()),
> +			   (insn.opcode () == riscv_insn::LW ? 4 : 8));
> +	}
>        else
>  	{
>  	  end_prologue_addr = cur_pc;
> diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s
> new file mode 100644
> index 00000000000..ebc27ff1e8c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw-foo.s
> @@ -0,0 +1,74 @@
> +/* Copyright 2021 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 testcase contains a function where the 'ld', 'c.ld', 'lw' or 'c.lw'
> +   instruction is used in the prologue before the RA register have been saved
> +   on the stack.
> +
> +   This mimics a pattern observed in the __pthread_clockjoin_ex function
> +   in libpthread.so.0 (from glibc-2.33-0ubuntu5) where a canary value is
> +   loaded and placed on the stack in order to detect stack smashing.
> +
> +   The skeleton for this file was generated using the following command:
> +
> +      gcc -x c -S -c -o - - <<EOT
> +        static long int __canary = 42;
> +        extern int bar ();
> +        int foo () { return bar(); }
> +      EOT
> +
> +   The result of this command is modified in the following way:
> +     - The prologue is adapted to reserve 16 more bytes on the stack.
> +     - A part that simulates the installation of a canary on the stack is
> +       added.  The canary is loaded multiple times to simulate the use of
> +       various instructions that could do the work (ld or c.ld for a 64 bit
> +       canary, lw or c.lw for a 32 bit canary).
> +     - The epilogue is adjusted to be able to return properly.  The epilogue
> +       does not check the canary value since this testcase is only interested
> +       in ensuring GDB can scan the prologue.  */
> +
> +	.option pic
> +	.text
> +	.data
> +	.align	3
> +	.type	__canary, @object
> +	.size	__canary, 8
> +__canary:
> +	.dword	42
> +	.text
> +	.align	1
> +	.globl	foo
> +	.type	foo, @function
> +foo:
> +	addi	sp,sp,-32
> +	lla	a5,__canary  # Load the fake canary address.
> +	lw	t4,0(a5)     # Load a 32 bit canary (use t4 to force the use of
> +			     # the non compressed instruction).
> +	ld	t4,0(a5)     # Load a 64 bit canary (use t4 to force the use of
> +			     # the non compressed instruction).
> +	c.lw 	a4,0(a5)     # Load a 32 bit canary using the compressed insn.
> +	c.ld 	a4,0(a5)     # Load a 64 bit canary using the compressed insn.
> +	sd	a4,0(sp)     # Place the fake canary on the stack.
> +	sd	ra,16(sp)
> +	sd	s0,8(sp)
> +	addi	s0,sp,32
> +	call	bar@plt
> +	mv	a5,a0
> +	mv	a0,a5
> +	ld	ra,16(sp)
> +	ld	s0,8(sp)
> +	addi	sp,sp,32
> +	jr	ra
> +	.size	foo, .-foo
> diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
> new file mode 100644
> index 00000000000..c329d2f72e0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
> @@ -0,0 +1,45 @@
> +# Copyright 2021 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 tests GDB's ability to use the RISC-V prologue scanner in order to
> +# unwind through a function that uses the 'ld' instruction in its prologue.
> +
> +if {![istarget "riscv64-*-*"]} {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile riscv64-unwind-prologue-with-ld.c \
> +		  riscv64-unwind-prologue-with-ld-lw-foo.s
> +if {[prepare_for_testing "failed to prepare" $testfile \
> +			 "$srcfile $srcfile2"  nodebug]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    fail "can't run to main"
> +    return 0
> +}
> +
> +gdb_breakpoint "bar"
> +gdb_continue_to_breakpoint "bar"
> +gdb_test "bt" \
> +    [multi_line \
> +         "#0\[ \t\]*$hex in bar \\\(\\\)" \
> +         "#1\[ \t\]*$hex in foo \\\(\\\)" \
> +         "#2\[ \t\]*$hex in main \\\(\\\)"] \
> +    "Backtrace to the main frame"
> +gdb_test "finish" "foo \\\(\\\)" "finish bar"
> +gdb_test "finish" "main \\\(\\\)" "finish foo"
> diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c
> new file mode 100644
> index 00000000000..9ff950df273
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c
> @@ -0,0 +1,30 @@
> +/* Copyright 2021 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/>.  */
> +
> +/* See riscv64-unwind-prologue-with-ld-foo.s for implementation.  */
> +extern int foo (void);
> +
> +int
> +bar ()
> +{
> +  return 0;
> +}
> +
> +int
> +main ()
> +{
> +  return foo ();
> +}
> +
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions
  2021-08-11 15:53 ` Andrew Burgess
@ 2021-08-12 23:26   ` Lancelot SIX
  2021-08-12 23:44     ` Lancelot SIX
  0 siblings, 1 reply; 6+ messages in thread
From: Lancelot SIX @ 2021-08-12 23:26 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

> 
> LGTM.
> 
> Thanks for doing this.
> 
> Andrew
> 

I just pushed it.

Lancelot.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions
  2021-08-12 23:26   ` Lancelot SIX
@ 2021-08-12 23:44     ` Lancelot SIX
  2021-08-13  9:14       ` Andrew Burgess
  0 siblings, 1 reply; 6+ messages in thread
From: Lancelot SIX @ 2021-08-12 23:44 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

On Thu, Aug 12, 2021 at 11:26:38PM +0000, Lancelot SIX via Gdb-patches wrote:
> > 
> > LGTM.
> > 
> > Thanks for doing this.
> > 
> > Andrew
> > 
> 
> I just pushed it.
> 
> Lancelot.

Actually, I just realized that one of the testfile was not properly
named.  It is probably not critical, keep it tidy, I’ll rename the .c
file to match the name of the testcase.

Is the following patch OK?

---
From bda6d870e1202ceb113de369b8a109d495b8d4d7 Mon Sep 17 00:00:00 2001
From: Lancelot SIX <lsix@lancelotsix.com>
Date: Thu, 12 Aug 2021 23:29:08 +0000
Subject: [PATCH] gdb: rename
 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A previous commit added the
gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp testcase, but one of its
associated file was named after a previous version of the test.

This commit fixes this and makes sure that all the files linked to this
testcase share the same prefix in the name.

Tested on riscv64 GNU/Linux.
---
 ...-prologue-with-ld.c => riscv64-unwind-prologue-with-ld-lw.c} | 0
 gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp   | 2 +-
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename gdb/testsuite/gdb.arch/{riscv64-unwind-prologue-with-ld.c => riscv64-unwind-prologue-with-ld-lw.c} (100%)

diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.c
similarity index 100%
rename from gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c
rename to gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.c
diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
index c329d2f72e0..f559c3ff4bc 100644
--- a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
+++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
@@ -21,7 +21,7 @@ if {![istarget "riscv64-*-*"]} {
     return
 }
 
-standard_testfile riscv64-unwind-prologue-with-ld.c \
+standard_testfile riscv64-unwind-prologue-with-ld-lw.c \
 		  riscv64-unwind-prologue-with-ld-lw-foo.s
 if {[prepare_for_testing "failed to prepare" $testfile \
 			 "$srcfile $srcfile2"  nodebug]} {
-- 
2.30.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions
  2021-08-12 23:44     ` Lancelot SIX
@ 2021-08-13  9:14       ` Andrew Burgess
  2021-09-06 20:25         ` Lancelot SIX
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2021-08-13  9:14 UTC (permalink / raw)
  To: Lancelot SIX; +Cc: gdb-patches

* Lancelot SIX <lsix@lancelotsix.com> [2021-08-12 23:44:19 +0000]:

> On Thu, Aug 12, 2021 at 11:26:38PM +0000, Lancelot SIX via Gdb-patches wrote:
> > > 
> > > LGTM.
> > > 
> > > Thanks for doing this.
> > > 
> > > Andrew
> > > 
> > 
> > I just pushed it.
> > 
> > Lancelot.
> 
> Actually, I just realized that one of the testfile was not properly
> named.  It is probably not critical, keep it tidy, I’ll rename the .c
> file to match the name of the testcase.
> 
> Is the following patch OK?

LGTM.  Sorry for not catching this.

Thanks,
Andrew

> 
> ---
> From bda6d870e1202ceb113de369b8a109d495b8d4d7 Mon Sep 17 00:00:00 2001
> From: Lancelot SIX <lsix@lancelotsix.com>
> Date: Thu, 12 Aug 2021 23:29:08 +0000
> Subject: [PATCH] gdb: rename
>  gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.c
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> A previous commit added the
> gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp testcase, but one of its
> associated file was named after a previous version of the test.
> 
> This commit fixes this and makes sure that all the files linked to this
> testcase share the same prefix in the name.
> 
> Tested on riscv64 GNU/Linux.
> ---
>  ...-prologue-with-ld.c => riscv64-unwind-prologue-with-ld-lw.c} | 0
>  gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp   | 2 +-
>  2 files changed, 1 insertion(+), 1 deletion(-)
>  rename gdb/testsuite/gdb.arch/{riscv64-unwind-prologue-with-ld.c => riscv64-unwind-prologue-with-ld-lw.c} (100%)
> 
> diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.c
> similarity index 100%
> rename from gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld.c
> rename to gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.c
> diff --git a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
> index c329d2f72e0..f559c3ff4bc 100644
> --- a/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
> +++ b/gdb/testsuite/gdb.arch/riscv64-unwind-prologue-with-ld-lw.exp
> @@ -21,7 +21,7 @@ if {![istarget "riscv64-*-*"]} {
>      return
>  }
>  
> -standard_testfile riscv64-unwind-prologue-with-ld.c \
> +standard_testfile riscv64-unwind-prologue-with-ld-lw.c \
>  		  riscv64-unwind-prologue-with-ld-lw-foo.s
>  if {[prepare_for_testing "failed to prepare" $testfile \
>  			 "$srcfile $srcfile2"  nodebug]} {
> -- 
> 2.30.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions
  2021-08-13  9:14       ` Andrew Burgess
@ 2021-09-06 20:25         ` Lancelot SIX
  0 siblings, 0 replies; 6+ messages in thread
From: Lancelot SIX @ 2021-09-06 20:25 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

> > Is the following patch OK?
> 
> LGTM.  Sorry for not catching this.

I just pushed the fix.

Sorry for the initial error and the delay.

Lancelot.

> 
> Thanks,
> Andrew
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-06 20:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 22:37 [PATCH v2] gdb: riscv_scan_prologue: handle LD and LW instructions Lancelot SIX
2021-08-11 15:53 ` Andrew Burgess
2021-08-12 23:26   ` Lancelot SIX
2021-08-12 23:44     ` Lancelot SIX
2021-08-13  9:14       ` Andrew Burgess
2021-09-06 20:25         ` Lancelot SIX

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 read-only IMAP folder(s) and NNTP newsgroup(s).