Re: [PATCH] gdb/riscv: Improve non-dwarf stack unwinding

[Palmer Dabbelt <palmer@sifive.com>]

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