RISC-V: Allow setting breakpoints at invalid addresses.

  Some testsuite testcases construct dwarf2 debug info for fake functions to
test that this debug info is handled correctly.  We get an error trying to
read from an invalid address when setting a breakpoint on these fake functions.
Other targets allow setting breakpoints on invalid addresses, and only error
when a command forces us to write the breakpoint to memory.  This matches that
behavior by wrapping the read in a try/catch.

Tested with a riscv64-linux native testsuite run.  I get 55 fewer unexpected
failures with the patch, and there are no regressions.

	gdb/
	* riscv-tdep.c (riscv_breakpoint_kind_from_pc): Wrap read_code call
	in try/catch.  Set buf[0] to 0 in catch clause.
---
 gdb/riscv-tdep.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
  

* Jim Wilson <jimw@sifive.com> [2019-04-13 13:42:42 -0700]:

> Some testsuite testcases construct dwarf2 debug info for fake functions to
> test that this debug info is handled correctly.  We get an error trying to
> read from an invalid address when setting a breakpoint on these fake functions.
> Other targets allow setting breakpoints on invalid addresses, and only error
> when a command forces us to write the breakpoint to memory.  This matches that
> behavior by wrapping the read in a try/catch.
> 
> Tested with a riscv64-linux native testsuite run.  I get 55 fewer unexpected
> failures with the patch, and there are no regressions.
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_breakpoint_kind_from_pc): Wrap read_code call
> 	in try/catch.  Set buf[0] to 0 in catch clause.
> ---
>  gdb/riscv-tdep.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 6370bc268f..38dc578add 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -430,8 +430,19 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
>  	unaligned_p = true;
>        else
>  	{
> -	  /* Read the opcode byte to determine the instruction length.  */
> -	  read_code (*pcptr, buf, 1);
> +	  try
> +	    {
> +	      /* Read the opcode byte to determine the instruction length.  */
> +	      read_code (*pcptr, buf, 1);
> +	    }
> +	  catch (const gdb_exception_error &ex)
> +	    {
> +	      /* We may have tried to set a breakpoint at a invalid address.
> +		 Defer the error until we try to write the breakpoint to
> +		 memory to match how other gdb targets work.  Also, testsuite
> +		 testcases like gdb.cp/nsalias.exp require this behavior.  */
> +	      buf[0] = 0;
> +	    }

I think that you can just use something like:

	  /* Read the opcode byte to determine the instruction length.  If
	     the read fails this may be because we tried to set the
	     breakpoint at an invalid address.  We provide a fake result
	     which will give a breakpoint length of 4, hopefully when we
	     try to actually insert the breakpoint we will see a failure
	     then too.  */
	  if (target_read_code (*pcptr, buf, 1) == -1)
	    buf[0] = 0;

and avoid the try/catch.

Annoyingly my QEMU/RISC-V/Linux setup got broken so I'm unable to test
the above right now ... hopefully I'll get some time next week to fix
my infrastructure issue :-/

Thanks,
Andrew

>  	}
>  
>        if (riscv_debug_breakpoints)
> -- 
> 2.17.1
>
  

On Sat, Apr 13, 2019 at 4:16 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> I think that you can just use something like:
>
>           /* Read the opcode byte to determine the instruction length.  If
>              the read fails this may be because we tried to set the
>              breakpoint at an invalid address.  We provide a fake result
>              which will give a breakpoint length of 4, hopefully when we
>              try to actually insert the breakpoint we will see a failure
>              then too.  */
>           if (target_read_code (*pcptr, buf, 1) == -1)
>             buf[0] = 0;
>
> and avoid the try/catch.

Yes, thanks, this looks a little simpler than what I suggested.  I
confirmed that this works with a riscv64-linux gdb build and make
check.

Jim
  

* Jim Wilson <jimw@sifive.com> [2019-04-14 16:23:40 -0700]:

> On Sat, Apr 13, 2019 at 4:16 PM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > I think that you can just use something like:
> >
> >           /* Read the opcode byte to determine the instruction length.  If
> >              the read fails this may be because we tried to set the
> >              breakpoint at an invalid address.  We provide a fake result
> >              which will give a breakpoint length of 4, hopefully when we
> >              try to actually insert the breakpoint we will see a failure
> >              then too.  */
> >           if (target_read_code (*pcptr, buf, 1) == -1)
> >             buf[0] = 0;
> >
> > and avoid the try/catch.
> 
> Yes, thanks, this looks a little simpler than what I suggested.  I
> confirmed that this works with a riscv64-linux gdb build and make
> check.

Thanks for testing this.

I pushed a patch with the above fix in.

Thanks,
Andrew