* Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding
2018-09-19 16:41 [PATCH] gdb/riscv: Improve non-dwarf stack unwinding Andrew Burgess
@ 2018-09-21 16:20 ` Palmer Dabbelt
2018-09-26 13:13 ` PING: " Andrew Burgess
2018-09-26 13:49 ` Joel Brobecker
2 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2018-09-21 16:20 UTC (permalink / raw)
To: andrew.burgess; +Cc: gdb-patches, Jim Wilson, andrew.burgess
On Wed, 19 Sep 2018 09:41:38 PDT (-0700), andrew.burgess@embecosm.com wrote:
> This commit improves the prologue scanning stack unwinder, to better
> support AUIPC, LUI, and more variants of ADD and ADDI.
>
> This allows unwinding over frames containing large local variables,
> where the frame size does not fit into a single instruction immediate,
> and is first loaded into a temporary register, before being added to
> the stack pointer.
>
> A new test is added that tests this behaviour. As there's nothing
> truely RiscV specific about this test I've added it into gdb.base, but
> as this depends on target specific code to perform the unwind it is
> possible that some targets might fail this new test.
>
> gdb/ChangeLog:
>
> * riscv-tdep.c (riscv_insn::decode): Decode c.lui.
> (riscv_scan_prologue): Split handling of AUIPC, LUI, ADD, ADDI,
> and NOP.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/large-frame-1.c: New file.
> * gdb.base/large-frame-2.c: New file.
> * gdb.base/large-frame.exp: New file.
> * gdb.base/large-frame.h: New file.
Looks good to me, thanks!
> ---
> gdb/ChangeLog | 6 ++++
> gdb/riscv-tdep.c | 59 ++++++++++++++++++++++------------
> gdb/testsuite/ChangeLog | 7 ++++
> gdb/testsuite/gdb.base/large-frame-1.c | 32 ++++++++++++++++++
> gdb/testsuite/gdb.base/large-frame-2.c | 25 ++++++++++++++
> gdb/testsuite/gdb.base/large-frame.exp | 57 ++++++++++++++++++++++++++++++++
> gdb/testsuite/gdb.base/large-frame.h | 24 ++++++++++++++
> 7 files changed, 189 insertions(+), 21 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/large-frame-1.c
> create mode 100644 gdb/testsuite/gdb.base/large-frame-2.c
> create mode 100644 gdb/testsuite/gdb.base/large-frame.exp
> create mode 100644 gdb/testsuite/gdb.base/large-frame.h
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7e..163c8ec231d 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1227,7 +1227,11 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
> m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);
> }
> else if (is_c_lui_insn (ival))
> - m_opcode = OTHER;
> + {
> + m_opcode = LUI;
> + m_rd = decode_register_index (ival, OP_SH_CRS1S);
> + m_imm.s = EXTRACT_RVC_LUI_IMM (ival);
> + }
> /* C_SD and C_FSW have the same opcode. C_SD is RV64 and RV128 only,
> and C_FSW is RV32 only. */
> else if (xlen != 4 && is_c_sd_insn (ival))
> @@ -1359,28 +1363,41 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
> gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0);
> }
> - else if ((insn.rd () == RISCV_GP_REGNUM
> - && (insn.opcode () == riscv_insn::AUIPC
> - || insn.opcode () == riscv_insn::LUI
> - || (insn.opcode () == riscv_insn::ADDI
> - && insn.rs1 () == RISCV_GP_REGNUM)
> - || (insn.opcode () == riscv_insn::ADD
> - && (insn.rs1 () == RISCV_GP_REGNUM
> - || insn.rs2 () == RISCV_GP_REGNUM))))
> - || (insn.opcode () == riscv_insn::ADDI
> - && insn.rd () == RISCV_ZERO_REGNUM
> - && insn.rs1 () == RISCV_ZERO_REGNUM
> - && insn.imm_signed () == 0))
> + else if ((insn.opcode () == riscv_insn::ADDI
> + && insn.rd () == RISCV_ZERO_REGNUM
> + && insn.rs1 () == RISCV_ZERO_REGNUM
> + && insn.imm_signed () == 0))
> {
> - /* Handle: auipc gp, n
> - or: addi gp, gp, n
> - or: add gp, gp, reg
> - or: add gp, reg, gp
> - or: lui gp, n
> - or: add x0, x0, 0 (NOP) */
> - /* These instructions are part of the prologue, but we don't need
> - to do anything special to handle them. */
> + /* Handle: add x0, x0, 0 (NOP) */
> }
> + else if (insn.opcode () == riscv_insn::AUIPC)
> + {
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_constant (cur_pc + insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::LUI)
> + {
> + /* Handle: lui REG, n
> + Where REG is not gp register. */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_constant (insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::ADDI)
> + {
> + /* Handle: addi REG1, REG2, IMM */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()]
> + = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::ADD)
> + {
> + /* Handle: addi REG1, REG2, IMM */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]);
> + }
> else
> {
> end_prologue_addr = cur_pc;
> diff --git a/gdb/testsuite/gdb.base/large-frame-1.c b/gdb/testsuite/gdb.base/large-frame-1.c
> new file mode 100644
> index 00000000000..cd9c5ccdd0a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame-1.c
> @@ -0,0 +1,32 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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 "large-frame.h"
> +
> +__attribute__ ((noinline)) void
> +blah (int *a)
> +{
> + asm ("" :: "m" (a) : "memory");
> +}
> +
> +int
> +main ()
> +{
> + func ();
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/large-frame-2.c b/gdb/testsuite/gdb.base/large-frame-2.c
> new file mode 100644
> index 00000000000..2491c347292
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame-2.c
> @@ -0,0 +1,25 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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 "large-frame.h"
> +
> +__attribute__ ((noinline)) int
> +func (void)
> +{
> + int a[4096];
> + blah (a);
> +}
> diff --git a/gdb/testsuite/gdb.base/large-frame.exp b/gdb/testsuite/gdb.base/large-frame.exp
> new file mode 100644
> index 00000000000..00090da795e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame.exp
> @@ -0,0 +1,57 @@
> +# Copyright 2018 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 file is part of the gdb testsuite.
> +
> +# This test was added to test GDB's ability to backtrace over a large
> +# stack frame for which there is no debug information. This should
> +# test the non-DWARF stack unwinder.
> +#
> +# The test was originally added for Risc-V where this case caused a
> +# problem at one point, however, there's nothing Risc-V specific in
> +# the test.
> +
> +proc run_test { opt_level } {
> + global srcfile srcfile2 binfile hex
> +
> + standard_testfile large-frame-1.c large-frame-2.c
> +
> + if {[prepare_for_testing_full "failed to prepare" \
> + [list ${binfile}-${opt_level} debug \
> + $srcfile [list debug] \
> + $srcfile2 [list nodebug optimize=-$opt_level]]]} {
> + return
> + }
> +
> + if { ![runto_main] } then {
> + unsupported "runto main"
> + return
> + }
> +
> + gdb_breakpoint "blah"
> + gdb_continue_to_breakpoint "blah"
> +
> + gdb_test "backtrace" [multi_line \
> + "#0 blah \[^\n\r\]+" \
> + "#1 $hex in func \[^\n\r\]+" \
> + "#2 $hex in main \[^\n\r\]+"]
> +}
> +
> +foreach opt { O0 O1 O2 } {
> + with_test_prefix "optimize=-$opt" {
> + run_test $opt
> + }
> +}
> +
> diff --git a/gdb/testsuite/gdb.base/large-frame.h b/gdb/testsuite/gdb.base/large-frame.h
> new file mode 100644
> index 00000000000..17058a5efd2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame.h
> @@ -0,0 +1,24 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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/>. */
> +
> +#ifndef LARGE_FRAME_H
> +#define LARGE_FRAME_H
> +
> +extern void blah (int *);
> +extern int func (void);
> +
> +#endif /* LARGE_FRAME_H */
^ permalink raw reply [flat|nested] 8+ messages in thread* PING: Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding
2018-09-19 16:41 [PATCH] gdb/riscv: Improve non-dwarf stack unwinding Andrew Burgess
2018-09-21 16:20 ` Palmer Dabbelt
@ 2018-09-26 13:13 ` Andrew Burgess
2018-09-26 13:49 ` Joel Brobecker
2 siblings, 0 replies; 8+ messages in thread
From: Andrew Burgess @ 2018-09-26 13:13 UTC (permalink / raw)
To: gdb-patches; +Cc: jimw, palmer
Ping!
Please could a global maintainer OK the testsuite changes.
Thanks,
Andrew
* Andrew Burgess <andrew.burgess@embecosm.com> [2018-09-19 17:41:38 +0100]:
> This commit improves the prologue scanning stack unwinder, to better
> support AUIPC, LUI, and more variants of ADD and ADDI.
>
> This allows unwinding over frames containing large local variables,
> where the frame size does not fit into a single instruction immediate,
> and is first loaded into a temporary register, before being added to
> the stack pointer.
>
> A new test is added that tests this behaviour. As there's nothing
> truely RiscV specific about this test I've added it into gdb.base, but
> as this depends on target specific code to perform the unwind it is
> possible that some targets might fail this new test.
>
> gdb/ChangeLog:
>
> * riscv-tdep.c (riscv_insn::decode): Decode c.lui.
> (riscv_scan_prologue): Split handling of AUIPC, LUI, ADD, ADDI,
> and NOP.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/large-frame-1.c: New file.
> * gdb.base/large-frame-2.c: New file.
> * gdb.base/large-frame.exp: New file.
> * gdb.base/large-frame.h: New file.
> ---
> gdb/ChangeLog | 6 ++++
> gdb/riscv-tdep.c | 59 ++++++++++++++++++++++------------
> gdb/testsuite/ChangeLog | 7 ++++
> gdb/testsuite/gdb.base/large-frame-1.c | 32 ++++++++++++++++++
> gdb/testsuite/gdb.base/large-frame-2.c | 25 ++++++++++++++
> gdb/testsuite/gdb.base/large-frame.exp | 57 ++++++++++++++++++++++++++++++++
> gdb/testsuite/gdb.base/large-frame.h | 24 ++++++++++++++
> 7 files changed, 189 insertions(+), 21 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/large-frame-1.c
> create mode 100644 gdb/testsuite/gdb.base/large-frame-2.c
> create mode 100644 gdb/testsuite/gdb.base/large-frame.exp
> create mode 100644 gdb/testsuite/gdb.base/large-frame.h
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7e..163c8ec231d 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1227,7 +1227,11 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
> m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);
> }
> else if (is_c_lui_insn (ival))
> - m_opcode = OTHER;
> + {
> + m_opcode = LUI;
> + m_rd = decode_register_index (ival, OP_SH_CRS1S);
> + m_imm.s = EXTRACT_RVC_LUI_IMM (ival);
> + }
> /* C_SD and C_FSW have the same opcode. C_SD is RV64 and RV128 only,
> and C_FSW is RV32 only. */
> else if (xlen != 4 && is_c_sd_insn (ival))
> @@ -1359,28 +1363,41 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
> gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0);
> }
> - else if ((insn.rd () == RISCV_GP_REGNUM
> - && (insn.opcode () == riscv_insn::AUIPC
> - || insn.opcode () == riscv_insn::LUI
> - || (insn.opcode () == riscv_insn::ADDI
> - && insn.rs1 () == RISCV_GP_REGNUM)
> - || (insn.opcode () == riscv_insn::ADD
> - && (insn.rs1 () == RISCV_GP_REGNUM
> - || insn.rs2 () == RISCV_GP_REGNUM))))
> - || (insn.opcode () == riscv_insn::ADDI
> - && insn.rd () == RISCV_ZERO_REGNUM
> - && insn.rs1 () == RISCV_ZERO_REGNUM
> - && insn.imm_signed () == 0))
> + else if ((insn.opcode () == riscv_insn::ADDI
> + && insn.rd () == RISCV_ZERO_REGNUM
> + && insn.rs1 () == RISCV_ZERO_REGNUM
> + && insn.imm_signed () == 0))
> {
> - /* Handle: auipc gp, n
> - or: addi gp, gp, n
> - or: add gp, gp, reg
> - or: add gp, reg, gp
> - or: lui gp, n
> - or: add x0, x0, 0 (NOP) */
> - /* These instructions are part of the prologue, but we don't need
> - to do anything special to handle them. */
> + /* Handle: add x0, x0, 0 (NOP) */
> }
> + else if (insn.opcode () == riscv_insn::AUIPC)
> + {
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_constant (cur_pc + insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::LUI)
> + {
> + /* Handle: lui REG, n
> + Where REG is not gp register. */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_constant (insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::ADDI)
> + {
> + /* Handle: addi REG1, REG2, IMM */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()]
> + = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::ADD)
> + {
> + /* Handle: addi REG1, REG2, IMM */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]);
> + }
> else
> {
> end_prologue_addr = cur_pc;
> diff --git a/gdb/testsuite/gdb.base/large-frame-1.c b/gdb/testsuite/gdb.base/large-frame-1.c
> new file mode 100644
> index 00000000000..cd9c5ccdd0a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame-1.c
> @@ -0,0 +1,32 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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 "large-frame.h"
> +
> +__attribute__ ((noinline)) void
> +blah (int *a)
> +{
> + asm ("" :: "m" (a) : "memory");
> +}
> +
> +int
> +main ()
> +{
> + func ();
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/large-frame-2.c b/gdb/testsuite/gdb.base/large-frame-2.c
> new file mode 100644
> index 00000000000..2491c347292
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame-2.c
> @@ -0,0 +1,25 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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 "large-frame.h"
> +
> +__attribute__ ((noinline)) int
> +func (void)
> +{
> + int a[4096];
> + blah (a);
> +}
> diff --git a/gdb/testsuite/gdb.base/large-frame.exp b/gdb/testsuite/gdb.base/large-frame.exp
> new file mode 100644
> index 00000000000..00090da795e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame.exp
> @@ -0,0 +1,57 @@
> +# Copyright 2018 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 file is part of the gdb testsuite.
> +
> +# This test was added to test GDB's ability to backtrace over a large
> +# stack frame for which there is no debug information. This should
> +# test the non-DWARF stack unwinder.
> +#
> +# The test was originally added for Risc-V where this case caused a
> +# problem at one point, however, there's nothing Risc-V specific in
> +# the test.
> +
> +proc run_test { opt_level } {
> + global srcfile srcfile2 binfile hex
> +
> + standard_testfile large-frame-1.c large-frame-2.c
> +
> + if {[prepare_for_testing_full "failed to prepare" \
> + [list ${binfile}-${opt_level} debug \
> + $srcfile [list debug] \
> + $srcfile2 [list nodebug optimize=-$opt_level]]]} {
> + return
> + }
> +
> + if { ![runto_main] } then {
> + unsupported "runto main"
> + return
> + }
> +
> + gdb_breakpoint "blah"
> + gdb_continue_to_breakpoint "blah"
> +
> + gdb_test "backtrace" [multi_line \
> + "#0 blah \[^\n\r\]+" \
> + "#1 $hex in func \[^\n\r\]+" \
> + "#2 $hex in main \[^\n\r\]+"]
> +}
> +
> +foreach opt { O0 O1 O2 } {
> + with_test_prefix "optimize=-$opt" {
> + run_test $opt
> + }
> +}
> +
> diff --git a/gdb/testsuite/gdb.base/large-frame.h b/gdb/testsuite/gdb.base/large-frame.h
> new file mode 100644
> index 00000000000..17058a5efd2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame.h
> @@ -0,0 +1,24 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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/>. */
> +
> +#ifndef LARGE_FRAME_H
> +#define LARGE_FRAME_H
> +
> +extern void blah (int *);
> +extern int func (void);
> +
> +#endif /* LARGE_FRAME_H */
> --
> 2.14.4
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding
2018-09-19 16:41 [PATCH] gdb/riscv: Improve non-dwarf stack unwinding Andrew Burgess
2018-09-21 16:20 ` Palmer Dabbelt
2018-09-26 13:13 ` PING: " Andrew Burgess
@ 2018-09-26 13:49 ` Joel Brobecker
2018-09-26 15:11 ` Andrew Burgess
2 siblings, 1 reply; 8+ messages in thread
From: Joel Brobecker @ 2018-09-26 13:49 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, jimw, palmer
Hi Andrew,
> This commit improves the prologue scanning stack unwinder, to better
> support AUIPC, LUI, and more variants of ADD and ADDI.
>
> This allows unwinding over frames containing large local variables,
> where the frame size does not fit into a single instruction immediate,
> and is first loaded into a temporary register, before being added to
> the stack pointer.
>
> A new test is added that tests this behaviour. As there's nothing
> truely RiscV specific about this test I've added it into gdb.base, but
> as this depends on target specific code to perform the unwind it is
> possible that some targets might fail this new test.
>
> gdb/ChangeLog:
>
> * riscv-tdep.c (riscv_insn::decode): Decode c.lui.
> (riscv_scan_prologue): Split handling of AUIPC, LUI, ADD, ADDI,
> and NOP.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/large-frame-1.c: New file.
> * gdb.base/large-frame-2.c: New file.
> * gdb.base/large-frame.exp: New file.
> * gdb.base/large-frame.h: New file.
I am OK with the patch.
Are you sure you want to assert on the register number? What if
the instruction was malformed and the register number ended up
being larger than RISCV_NUM_INTEGER_REGS; is that possible?
> ---
> gdb/ChangeLog | 6 ++++
> gdb/riscv-tdep.c | 59 ++++++++++++++++++++++------------
> gdb/testsuite/ChangeLog | 7 ++++
> gdb/testsuite/gdb.base/large-frame-1.c | 32 ++++++++++++++++++
> gdb/testsuite/gdb.base/large-frame-2.c | 25 ++++++++++++++
> gdb/testsuite/gdb.base/large-frame.exp | 57 ++++++++++++++++++++++++++++++++
> gdb/testsuite/gdb.base/large-frame.h | 24 ++++++++++++++
> 7 files changed, 189 insertions(+), 21 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/large-frame-1.c
> create mode 100644 gdb/testsuite/gdb.base/large-frame-2.c
> create mode 100644 gdb/testsuite/gdb.base/large-frame.exp
> create mode 100644 gdb/testsuite/gdb.base/large-frame.h
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7e..163c8ec231d 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1227,7 +1227,11 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
> m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);
> }
> else if (is_c_lui_insn (ival))
> - m_opcode = OTHER;
> + {
> + m_opcode = LUI;
> + m_rd = decode_register_index (ival, OP_SH_CRS1S);
> + m_imm.s = EXTRACT_RVC_LUI_IMM (ival);
> + }
> /* C_SD and C_FSW have the same opcode. C_SD is RV64 and RV128 only,
> and C_FSW is RV32 only. */
> else if (xlen != 4 && is_c_sd_insn (ival))
> @@ -1359,28 +1363,41 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
> gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0);
> }
> - else if ((insn.rd () == RISCV_GP_REGNUM
> - && (insn.opcode () == riscv_insn::AUIPC
> - || insn.opcode () == riscv_insn::LUI
> - || (insn.opcode () == riscv_insn::ADDI
> - && insn.rs1 () == RISCV_GP_REGNUM)
> - || (insn.opcode () == riscv_insn::ADD
> - && (insn.rs1 () == RISCV_GP_REGNUM
> - || insn.rs2 () == RISCV_GP_REGNUM))))
> - || (insn.opcode () == riscv_insn::ADDI
> - && insn.rd () == RISCV_ZERO_REGNUM
> - && insn.rs1 () == RISCV_ZERO_REGNUM
> - && insn.imm_signed () == 0))
> + else if ((insn.opcode () == riscv_insn::ADDI
> + && insn.rd () == RISCV_ZERO_REGNUM
> + && insn.rs1 () == RISCV_ZERO_REGNUM
> + && insn.imm_signed () == 0))
> {
> - /* Handle: auipc gp, n
> - or: addi gp, gp, n
> - or: add gp, gp, reg
> - or: add gp, reg, gp
> - or: lui gp, n
> - or: add x0, x0, 0 (NOP) */
> - /* These instructions are part of the prologue, but we don't need
> - to do anything special to handle them. */
> + /* Handle: add x0, x0, 0 (NOP) */
> }
> + else if (insn.opcode () == riscv_insn::AUIPC)
> + {
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_constant (cur_pc + insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::LUI)
> + {
> + /* Handle: lui REG, n
> + Where REG is not gp register. */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_constant (insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::ADDI)
> + {
> + /* Handle: addi REG1, REG2, IMM */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()]
> + = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
> + }
> + else if (insn.opcode () == riscv_insn::ADD)
> + {
> + /* Handle: addi REG1, REG2, IMM */
> + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> + gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
> + regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]);
> + }
> else
> {
> end_prologue_addr = cur_pc;
> diff --git a/gdb/testsuite/gdb.base/large-frame-1.c b/gdb/testsuite/gdb.base/large-frame-1.c
> new file mode 100644
> index 00000000000..cd9c5ccdd0a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame-1.c
> @@ -0,0 +1,32 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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 "large-frame.h"
> +
> +__attribute__ ((noinline)) void
> +blah (int *a)
> +{
> + asm ("" :: "m" (a) : "memory");
> +}
> +
> +int
> +main ()
> +{
> + func ();
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/large-frame-2.c b/gdb/testsuite/gdb.base/large-frame-2.c
> new file mode 100644
> index 00000000000..2491c347292
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame-2.c
> @@ -0,0 +1,25 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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 "large-frame.h"
> +
> +__attribute__ ((noinline)) int
> +func (void)
> +{
> + int a[4096];
> + blah (a);
> +}
> diff --git a/gdb/testsuite/gdb.base/large-frame.exp b/gdb/testsuite/gdb.base/large-frame.exp
> new file mode 100644
> index 00000000000..00090da795e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame.exp
> @@ -0,0 +1,57 @@
> +# Copyright 2018 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 file is part of the gdb testsuite.
> +
> +# This test was added to test GDB's ability to backtrace over a large
> +# stack frame for which there is no debug information. This should
> +# test the non-DWARF stack unwinder.
> +#
> +# The test was originally added for Risc-V where this case caused a
> +# problem at one point, however, there's nothing Risc-V specific in
> +# the test.
> +
> +proc run_test { opt_level } {
> + global srcfile srcfile2 binfile hex
> +
> + standard_testfile large-frame-1.c large-frame-2.c
> +
> + if {[prepare_for_testing_full "failed to prepare" \
> + [list ${binfile}-${opt_level} debug \
> + $srcfile [list debug] \
> + $srcfile2 [list nodebug optimize=-$opt_level]]]} {
> + return
> + }
> +
> + if { ![runto_main] } then {
> + unsupported "runto main"
> + return
> + }
> +
> + gdb_breakpoint "blah"
> + gdb_continue_to_breakpoint "blah"
> +
> + gdb_test "backtrace" [multi_line \
> + "#0 blah \[^\n\r\]+" \
> + "#1 $hex in func \[^\n\r\]+" \
> + "#2 $hex in main \[^\n\r\]+"]
> +}
> +
> +foreach opt { O0 O1 O2 } {
> + with_test_prefix "optimize=-$opt" {
> + run_test $opt
> + }
> +}
> +
> diff --git a/gdb/testsuite/gdb.base/large-frame.h b/gdb/testsuite/gdb.base/large-frame.h
> new file mode 100644
> index 00000000000..17058a5efd2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/large-frame.h
> @@ -0,0 +1,24 @@
> +/* This file is part of GDB, the GNU debugger.
> +
> + Copyright 2018 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/>. */
> +
> +#ifndef LARGE_FRAME_H
> +#define LARGE_FRAME_H
> +
> +extern void blah (int *);
> +extern int func (void);
> +
> +#endif /* LARGE_FRAME_H */
> --
> 2.14.4
--
Joel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding
2018-09-26 13:49 ` Joel Brobecker
@ 2018-09-26 15:11 ` Andrew Burgess
2018-09-26 16:53 ` Joel Brobecker
2018-09-26 17:13 ` Palmer Dabbelt
0 siblings, 2 replies; 8+ messages in thread
From: Andrew Burgess @ 2018-09-26 15:11 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches, jimw, palmer
* Joel Brobecker <brobecker@adacore.com> [2018-09-26 06:49:16 -0700]:
> Hi Andrew,
>
> > This commit improves the prologue scanning stack unwinder, to better
> > support AUIPC, LUI, and more variants of ADD and ADDI.
> >
> > This allows unwinding over frames containing large local variables,
> > where the frame size does not fit into a single instruction immediate,
> > and is first loaded into a temporary register, before being added to
> > the stack pointer.
> >
> > A new test is added that tests this behaviour. As there's nothing
> > truely RiscV specific about this test I've added it into gdb.base, but
> > as this depends on target specific code to perform the unwind it is
> > possible that some targets might fail this new test.
> >
> > gdb/ChangeLog:
> >
> > * riscv-tdep.c (riscv_insn::decode): Decode c.lui.
> > (riscv_scan_prologue): Split handling of AUIPC, LUI, ADD, ADDI,
> > and NOP.
> >
> > gdb/testsuite/ChangeLog:
> >
> > * gdb.base/large-frame-1.c: New file.
> > * gdb.base/large-frame-2.c: New file.
> > * gdb.base/large-frame.exp: New file.
> > * gdb.base/large-frame.h: New file.
>
> I am OK with the patch.
>
> Are you sure you want to assert on the register number? What if
> the instruction was malformed and the register number ended up
> being larger than RISCV_NUM_INTEGER_REGS; is that possible?
Well, it shouldn't happen in the sense that, for a given instruction,
if it operates on integer registers then the space in the encoding
should only allow for up to RISCV_NUM_INTEGER_REGS registers (some
encodings allow for less registers).
Right now I can't imagine a situation where one of those asserts could
fire and it's not either a bug in GDB, or a bug in the libopcode field
extraction code - hence an assert rather than an error.
Thanks,
Andrew
>
>
> > ---
> > gdb/ChangeLog | 6 ++++
> > gdb/riscv-tdep.c | 59 ++++++++++++++++++++++------------
> > gdb/testsuite/ChangeLog | 7 ++++
> > gdb/testsuite/gdb.base/large-frame-1.c | 32 ++++++++++++++++++
> > gdb/testsuite/gdb.base/large-frame-2.c | 25 ++++++++++++++
> > gdb/testsuite/gdb.base/large-frame.exp | 57 ++++++++++++++++++++++++++++++++
> > gdb/testsuite/gdb.base/large-frame.h | 24 ++++++++++++++
> > 7 files changed, 189 insertions(+), 21 deletions(-)
> > create mode 100644 gdb/testsuite/gdb.base/large-frame-1.c
> > create mode 100644 gdb/testsuite/gdb.base/large-frame-2.c
> > create mode 100644 gdb/testsuite/gdb.base/large-frame.exp
> > create mode 100644 gdb/testsuite/gdb.base/large-frame.h
> >
> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> > index 254914c9c7e..163c8ec231d 100644
> > --- a/gdb/riscv-tdep.c
> > +++ b/gdb/riscv-tdep.c
> > @@ -1227,7 +1227,11 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
> > m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);
> > }
> > else if (is_c_lui_insn (ival))
> > - m_opcode = OTHER;
> > + {
> > + m_opcode = LUI;
> > + m_rd = decode_register_index (ival, OP_SH_CRS1S);
> > + m_imm.s = EXTRACT_RVC_LUI_IMM (ival);
> > + }
> > /* C_SD and C_FSW have the same opcode. C_SD is RV64 and RV128 only,
> > and C_FSW is RV32 only. */
> > else if (xlen != 4 && is_c_sd_insn (ival))
> > @@ -1359,28 +1363,41 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
> > gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> > regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0);
> > }
> > - else if ((insn.rd () == RISCV_GP_REGNUM
> > - && (insn.opcode () == riscv_insn::AUIPC
> > - || insn.opcode () == riscv_insn::LUI
> > - || (insn.opcode () == riscv_insn::ADDI
> > - && insn.rs1 () == RISCV_GP_REGNUM)
> > - || (insn.opcode () == riscv_insn::ADD
> > - && (insn.rs1 () == RISCV_GP_REGNUM
> > - || insn.rs2 () == RISCV_GP_REGNUM))))
> > - || (insn.opcode () == riscv_insn::ADDI
> > - && insn.rd () == RISCV_ZERO_REGNUM
> > - && insn.rs1 () == RISCV_ZERO_REGNUM
> > - && insn.imm_signed () == 0))
> > + else if ((insn.opcode () == riscv_insn::ADDI
> > + && insn.rd () == RISCV_ZERO_REGNUM
> > + && insn.rs1 () == RISCV_ZERO_REGNUM
> > + && insn.imm_signed () == 0))
> > {
> > - /* Handle: auipc gp, n
> > - or: addi gp, gp, n
> > - or: add gp, gp, reg
> > - or: add gp, reg, gp
> > - or: lui gp, n
> > - or: add x0, x0, 0 (NOP) */
> > - /* These instructions are part of the prologue, but we don't need
> > - to do anything special to handle them. */
> > + /* Handle: add x0, x0, 0 (NOP) */
> > }
> > + else if (insn.opcode () == riscv_insn::AUIPC)
> > + {
> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> > + regs[insn.rd ()] = pv_constant (cur_pc + insn.imm_signed ());
> > + }
> > + else if (insn.opcode () == riscv_insn::LUI)
> > + {
> > + /* Handle: lui REG, n
> > + Where REG is not gp register. */
> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> > + regs[insn.rd ()] = pv_constant (insn.imm_signed ());
> > + }
> > + else if (insn.opcode () == riscv_insn::ADDI)
> > + {
> > + /* Handle: addi REG1, REG2, IMM */
> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> > + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> > + regs[insn.rd ()]
> > + = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
> > + }
> > + else if (insn.opcode () == riscv_insn::ADD)
> > + {
> > + /* Handle: addi REG1, REG2, IMM */
> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
> > + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
> > + gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
> > + regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]);
> > + }
> > else
> > {
> > end_prologue_addr = cur_pc;
> > diff --git a/gdb/testsuite/gdb.base/large-frame-1.c b/gdb/testsuite/gdb.base/large-frame-1.c
> > new file mode 100644
> > index 00000000000..cd9c5ccdd0a
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/large-frame-1.c
> > @@ -0,0 +1,32 @@
> > +/* This file is part of GDB, the GNU debugger.
> > +
> > + Copyright 2018 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 "large-frame.h"
> > +
> > +__attribute__ ((noinline)) void
> > +blah (int *a)
> > +{
> > + asm ("" :: "m" (a) : "memory");
> > +}
> > +
> > +int
> > +main ()
> > +{
> > + func ();
> > +
> > + return 0;
> > +}
> > diff --git a/gdb/testsuite/gdb.base/large-frame-2.c b/gdb/testsuite/gdb.base/large-frame-2.c
> > new file mode 100644
> > index 00000000000..2491c347292
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/large-frame-2.c
> > @@ -0,0 +1,25 @@
> > +/* This file is part of GDB, the GNU debugger.
> > +
> > + Copyright 2018 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 "large-frame.h"
> > +
> > +__attribute__ ((noinline)) int
> > +func (void)
> > +{
> > + int a[4096];
> > + blah (a);
> > +}
> > diff --git a/gdb/testsuite/gdb.base/large-frame.exp b/gdb/testsuite/gdb.base/large-frame.exp
> > new file mode 100644
> > index 00000000000..00090da795e
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/large-frame.exp
> > @@ -0,0 +1,57 @@
> > +# Copyright 2018 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 file is part of the gdb testsuite.
> > +
> > +# This test was added to test GDB's ability to backtrace over a large
> > +# stack frame for which there is no debug information. This should
> > +# test the non-DWARF stack unwinder.
> > +#
> > +# The test was originally added for Risc-V where this case caused a
> > +# problem at one point, however, there's nothing Risc-V specific in
> > +# the test.
> > +
> > +proc run_test { opt_level } {
> > + global srcfile srcfile2 binfile hex
> > +
> > + standard_testfile large-frame-1.c large-frame-2.c
> > +
> > + if {[prepare_for_testing_full "failed to prepare" \
> > + [list ${binfile}-${opt_level} debug \
> > + $srcfile [list debug] \
> > + $srcfile2 [list nodebug optimize=-$opt_level]]]} {
> > + return
> > + }
> > +
> > + if { ![runto_main] } then {
> > + unsupported "runto main"
> > + return
> > + }
> > +
> > + gdb_breakpoint "blah"
> > + gdb_continue_to_breakpoint "blah"
> > +
> > + gdb_test "backtrace" [multi_line \
> > + "#0 blah \[^\n\r\]+" \
> > + "#1 $hex in func \[^\n\r\]+" \
> > + "#2 $hex in main \[^\n\r\]+"]
> > +}
> > +
> > +foreach opt { O0 O1 O2 } {
> > + with_test_prefix "optimize=-$opt" {
> > + run_test $opt
> > + }
> > +}
> > +
> > diff --git a/gdb/testsuite/gdb.base/large-frame.h b/gdb/testsuite/gdb.base/large-frame.h
> > new file mode 100644
> > index 00000000000..17058a5efd2
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/large-frame.h
> > @@ -0,0 +1,24 @@
> > +/* This file is part of GDB, the GNU debugger.
> > +
> > + Copyright 2018 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/>. */
> > +
> > +#ifndef LARGE_FRAME_H
> > +#define LARGE_FRAME_H
> > +
> > +extern void blah (int *);
> > +extern int func (void);
> > +
> > +#endif /* LARGE_FRAME_H */
> > --
> > 2.14.4
>
> --
> Joel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding
2018-09-26 15:11 ` Andrew Burgess
@ 2018-09-26 16:53 ` Joel Brobecker
2018-09-26 17:13 ` Palmer Dabbelt
1 sibling, 0 replies; 8+ messages in thread
From: Joel Brobecker @ 2018-09-26 16:53 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, jimw, palmer
> > Are you sure you want to assert on the register number? What if
> > the instruction was malformed and the register number ended up
> > being larger than RISCV_NUM_INTEGER_REGS; is that possible?
>
> Well, it shouldn't happen in the sense that, for a given instruction,
> if it operates on integer registers then the space in the encoding
> should only allow for up to RISCV_NUM_INTEGER_REGS registers (some
> encodings allow for less registers).
>
> Right now I can't imagine a situation where one of those asserts could
> fire and it's not either a bug in GDB, or a bug in the libopcode field
> extraction code - hence an assert rather than an error.
OK, that's perfect. Thanks for the confirmation!
--
Joel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding
2018-09-26 15:11 ` Andrew Burgess
2018-09-26 16:53 ` Joel Brobecker
@ 2018-09-26 17:13 ` Palmer Dabbelt
2018-09-26 19:15 ` Joel Brobecker
1 sibling, 1 reply; 8+ messages in thread
From: Palmer Dabbelt @ 2018-09-26 17:13 UTC (permalink / raw)
To: andrew.burgess; +Cc: brobecker, gdb-patches, Jim Wilson
On Wed, 26 Sep 2018 08:11:12 PDT (-0700), andrew.burgess@embecosm.com wrote:
> * Joel Brobecker <brobecker@adacore.com> [2018-09-26 06:49:16 -0700]:
>
>> Hi Andrew,
>>
>> > This commit improves the prologue scanning stack unwinder, to better
>> > support AUIPC, LUI, and more variants of ADD and ADDI.
>> >
>> > This allows unwinding over frames containing large local variables,
>> > where the frame size does not fit into a single instruction immediate,
>> > and is first loaded into a temporary register, before being added to
>> > the stack pointer.
>> >
>> > A new test is added that tests this behaviour. As there's nothing
>> > truely RiscV specific about this test I've added it into gdb.base, but
>> > as this depends on target specific code to perform the unwind it is
>> > possible that some targets might fail this new test.
>> >
>> > gdb/ChangeLog:
>> >
>> > * riscv-tdep.c (riscv_insn::decode): Decode c.lui.
>> > (riscv_scan_prologue): Split handling of AUIPC, LUI, ADD, ADDI,
>> > and NOP.
>> >
>> > gdb/testsuite/ChangeLog:
>> >
>> > * gdb.base/large-frame-1.c: New file.
>> > * gdb.base/large-frame-2.c: New file.
>> > * gdb.base/large-frame.exp: New file.
>> > * gdb.base/large-frame.h: New file.
>>
>> I am OK with the patch.
>>
>> Are you sure you want to assert on the register number? What if
>> the instruction was malformed and the register number ended up
>> being larger than RISCV_NUM_INTEGER_REGS; is that possible?
>
> Well, it shouldn't happen in the sense that, for a given instruction,
> if it operates on integer registers then the space in the encoding
> should only allow for up to RISCV_NUM_INTEGER_REGS registers (some
> encodings allow for less registers).
>
> Right now I can't imagine a situation where one of those asserts could
> fire and it's not either a bug in GDB, or a bug in the libopcode field
> extraction code - hence an assert rather than an error.
While I agree this is true for I-based ISAs, I think this might be able to fire
for E-based ISAs because those can actually encode invalid register indices.
That said, these should be decoded as invalid instructions so I think we're
safe here. I'm OK either way (ie, abort or warn).
>
> Thanks,
> Andrew
>
>
>
>
>>
>>
>> > ---
>> > gdb/ChangeLog | 6 ++++
>> > gdb/riscv-tdep.c | 59 ++++++++++++++++++++++------------
>> > gdb/testsuite/ChangeLog | 7 ++++
>> > gdb/testsuite/gdb.base/large-frame-1.c | 32 ++++++++++++++++++
>> > gdb/testsuite/gdb.base/large-frame-2.c | 25 ++++++++++++++
>> > gdb/testsuite/gdb.base/large-frame.exp | 57 ++++++++++++++++++++++++++++++++
>> > gdb/testsuite/gdb.base/large-frame.h | 24 ++++++++++++++
>> > 7 files changed, 189 insertions(+), 21 deletions(-)
>> > create mode 100644 gdb/testsuite/gdb.base/large-frame-1.c
>> > create mode 100644 gdb/testsuite/gdb.base/large-frame-2.c
>> > create mode 100644 gdb/testsuite/gdb.base/large-frame.exp
>> > create mode 100644 gdb/testsuite/gdb.base/large-frame.h
>> >
>> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
>> > index 254914c9c7e..163c8ec231d 100644
>> > --- a/gdb/riscv-tdep.c
>> > +++ b/gdb/riscv-tdep.c
>> > @@ -1227,7 +1227,11 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
>> > m_imm.s = EXTRACT_RVC_ADDI4SPN_IMM (ival);
>> > }
>> > else if (is_c_lui_insn (ival))
>> > - m_opcode = OTHER;
>> > + {
>> > + m_opcode = LUI;
>> > + m_rd = decode_register_index (ival, OP_SH_CRS1S);
>> > + m_imm.s = EXTRACT_RVC_LUI_IMM (ival);
>> > + }
>> > /* C_SD and C_FSW have the same opcode. C_SD is RV64 and RV128 only,
>> > and C_FSW is RV32 only. */
>> > else if (xlen != 4 && is_c_sd_insn (ival))
>> > @@ -1359,28 +1363,41 @@ riscv_scan_prologue (struct gdbarch *gdbarch,
>> > gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
>> > regs[insn.rd ()] = pv_add_constant (regs[insn.rs1 ()], 0);
>> > }
>> > - else if ((insn.rd () == RISCV_GP_REGNUM
>> > - && (insn.opcode () == riscv_insn::AUIPC
>> > - || insn.opcode () == riscv_insn::LUI
>> > - || (insn.opcode () == riscv_insn::ADDI
>> > - && insn.rs1 () == RISCV_GP_REGNUM)
>> > - || (insn.opcode () == riscv_insn::ADD
>> > - && (insn.rs1 () == RISCV_GP_REGNUM
>> > - || insn.rs2 () == RISCV_GP_REGNUM))))
>> > - || (insn.opcode () == riscv_insn::ADDI
>> > - && insn.rd () == RISCV_ZERO_REGNUM
>> > - && insn.rs1 () == RISCV_ZERO_REGNUM
>> > - && insn.imm_signed () == 0))
>> > + else if ((insn.opcode () == riscv_insn::ADDI
>> > + && insn.rd () == RISCV_ZERO_REGNUM
>> > + && insn.rs1 () == RISCV_ZERO_REGNUM
>> > + && insn.imm_signed () == 0))
>> > {
>> > - /* Handle: auipc gp, n
>> > - or: addi gp, gp, n
>> > - or: add gp, gp, reg
>> > - or: add gp, reg, gp
>> > - or: lui gp, n
>> > - or: add x0, x0, 0 (NOP) */
>> > - /* These instructions are part of the prologue, but we don't need
>> > - to do anything special to handle them. */
>> > + /* Handle: add x0, x0, 0 (NOP) */
>> > }
>> > + else if (insn.opcode () == riscv_insn::AUIPC)
>> > + {
>> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
>> > + regs[insn.rd ()] = pv_constant (cur_pc + insn.imm_signed ());
>> > + }
>> > + else if (insn.opcode () == riscv_insn::LUI)
>> > + {
>> > + /* Handle: lui REG, n
>> > + Where REG is not gp register. */
>> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
>> > + regs[insn.rd ()] = pv_constant (insn.imm_signed ());
>> > + }
>> > + else if (insn.opcode () == riscv_insn::ADDI)
>> > + {
>> > + /* Handle: addi REG1, REG2, IMM */
>> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
>> > + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
>> > + regs[insn.rd ()]
>> > + = pv_add_constant (regs[insn.rs1 ()], insn.imm_signed ());
>> > + }
>> > + else if (insn.opcode () == riscv_insn::ADD)
>> > + {
>> > + /* Handle: addi REG1, REG2, IMM */
>> > + gdb_assert (insn.rd () < RISCV_NUM_INTEGER_REGS);
>> > + gdb_assert (insn.rs1 () < RISCV_NUM_INTEGER_REGS);
>> > + gdb_assert (insn.rs2 () < RISCV_NUM_INTEGER_REGS);
>> > + regs[insn.rd ()] = pv_add (regs[insn.rs1 ()], regs[insn.rs2 ()]);
>> > + }
>> > else
>> > {
>> > end_prologue_addr = cur_pc;
>> > diff --git a/gdb/testsuite/gdb.base/large-frame-1.c b/gdb/testsuite/gdb.base/large-frame-1.c
>> > new file mode 100644
>> > index 00000000000..cd9c5ccdd0a
>> > --- /dev/null
>> > +++ b/gdb/testsuite/gdb.base/large-frame-1.c
>> > @@ -0,0 +1,32 @@
>> > +/* This file is part of GDB, the GNU debugger.
>> > +
>> > + Copyright 2018 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 "large-frame.h"
>> > +
>> > +__attribute__ ((noinline)) void
>> > +blah (int *a)
>> > +{
>> > + asm ("" :: "m" (a) : "memory");
>> > +}
>> > +
>> > +int
>> > +main ()
>> > +{
>> > + func ();
>> > +
>> > + return 0;
>> > +}
>> > diff --git a/gdb/testsuite/gdb.base/large-frame-2.c b/gdb/testsuite/gdb.base/large-frame-2.c
>> > new file mode 100644
>> > index 00000000000..2491c347292
>> > --- /dev/null
>> > +++ b/gdb/testsuite/gdb.base/large-frame-2.c
>> > @@ -0,0 +1,25 @@
>> > +/* This file is part of GDB, the GNU debugger.
>> > +
>> > + Copyright 2018 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 "large-frame.h"
>> > +
>> > +__attribute__ ((noinline)) int
>> > +func (void)
>> > +{
>> > + int a[4096];
>> > + blah (a);
>> > +}
>> > diff --git a/gdb/testsuite/gdb.base/large-frame.exp b/gdb/testsuite/gdb.base/large-frame.exp
>> > new file mode 100644
>> > index 00000000000..00090da795e
>> > --- /dev/null
>> > +++ b/gdb/testsuite/gdb.base/large-frame.exp
>> > @@ -0,0 +1,57 @@
>> > +# Copyright 2018 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 file is part of the gdb testsuite.
>> > +
>> > +# This test was added to test GDB's ability to backtrace over a large
>> > +# stack frame for which there is no debug information. This should
>> > +# test the non-DWARF stack unwinder.
>> > +#
>> > +# The test was originally added for Risc-V where this case caused a
>> > +# problem at one point, however, there's nothing Risc-V specific in
>> > +# the test.
>> > +
>> > +proc run_test { opt_level } {
>> > + global srcfile srcfile2 binfile hex
>> > +
>> > + standard_testfile large-frame-1.c large-frame-2.c
>> > +
>> > + if {[prepare_for_testing_full "failed to prepare" \
>> > + [list ${binfile}-${opt_level} debug \
>> > + $srcfile [list debug] \
>> > + $srcfile2 [list nodebug optimize=-$opt_level]]]} {
>> > + return
>> > + }
>> > +
>> > + if { ![runto_main] } then {
>> > + unsupported "runto main"
>> > + return
>> > + }
>> > +
>> > + gdb_breakpoint "blah"
>> > + gdb_continue_to_breakpoint "blah"
>> > +
>> > + gdb_test "backtrace" [multi_line \
>> > + "#0 blah \[^\n\r\]+" \
>> > + "#1 $hex in func \[^\n\r\]+" \
>> > + "#2 $hex in main \[^\n\r\]+"]
>> > +}
>> > +
>> > +foreach opt { O0 O1 O2 } {
>> > + with_test_prefix "optimize=-$opt" {
>> > + run_test $opt
>> > + }
>> > +}
>> > +
>> > diff --git a/gdb/testsuite/gdb.base/large-frame.h b/gdb/testsuite/gdb.base/large-frame.h
>> > new file mode 100644
>> > index 00000000000..17058a5efd2
>> > --- /dev/null
>> > +++ b/gdb/testsuite/gdb.base/large-frame.h
>> > @@ -0,0 +1,24 @@
>> > +/* This file is part of GDB, the GNU debugger.
>> > +
>> > + Copyright 2018 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/>. */
>> > +
>> > +#ifndef LARGE_FRAME_H
>> > +#define LARGE_FRAME_H
>> > +
>> > +extern void blah (int *);
>> > +extern int func (void);
>> > +
>> > +#endif /* LARGE_FRAME_H */
>> > --
>> > 2.14.4
>>
>> --
>> Joel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding
2018-09-26 17:13 ` Palmer Dabbelt
@ 2018-09-26 19:15 ` Joel Brobecker
0 siblings, 0 replies; 8+ messages in thread
From: Joel Brobecker @ 2018-09-26 19:15 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: andrew.burgess, gdb-patches, Jim Wilson
> While I agree this is true for I-based ISAs, I think this might be able to
> fire for E-based ISAs because those can actually encode invalid register
> indices. That said, these should be decoded as invalid instructions so I
> think we're safe here. I'm OK either way (ie, abort or warn).
And FWIW, I agree that should the register number be invalid
in the instruction, the error should be reported during the decoding.
So the asserts here are good.
--
Joel
^ permalink raw reply [flat|nested] 8+ messages in thread