Re: [PATCH] RISC-V: Don't allow unaligned breakpoints.

[John Baldwin <jhb@FreeBSD.org>]

On 11/1/18 2:41 PM, Jim Wilson wrote:
> Some hardware doesn't support unaligned accesses, and a bare metal target
> may not have an unaligned access trap handler.  So if the PC is 2-byte
> aligned, then use a 2-byte breakpoint to avoid unaligned accesses.
> 
> Tested on native RV64GC Linux with gdb testsuite and cross on spike
> simulator and openocd with riscv-tests/debug.
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_breakpoint_kind_from_pc): New local unaligned_p.
> 	Set if pcptr if unaligned.  Return 2 if unaligned_p true.  Update
> 	debugging messages.
> ---
>  gdb/riscv-tdep.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 4b5f38a877..9c6872d021 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -415,18 +415,33 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
>  {
>    if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
>      {
> +      bool unaligned_p = false;
>        gdb_byte buf[1];
>  
> -      /* Read the opcode byte to determine the instruction length.  */
> -      read_code (*pcptr, buf, 1);
> +      /* Some targets don't support unaligned reads.  If the instruction
> +	 address is unaligned, use a compressed breakpoint.  */
> +      if (*pcptr & 0x2)
> +	unaligned_p = true;
> +      else
> +	{
> +	  /* Read the opcode byte to determine the instruction length.  */
> +	  read_code (*pcptr, buf, 1);
> +	}
>  
>        if (riscv_debug_breakpoints)
> -	fprintf_unfiltered
> -	  (gdb_stdlog,
> -	   "Using %s for breakpoint at %s (instruction length %d)\n",
> -	   riscv_insn_length (buf[0]) == 2 ? "C.EBREAK" : "EBREAK",
> -	   paddress (gdbarch, *pcptr), riscv_insn_length (buf[0]));
> -      if (riscv_insn_length (buf[0]) == 2)
> +	{
> +	  const char *bp = (unaligned_p || riscv_insn_length (buf[0]) == 2
> +			    ? "C.EBREAK" : "EBREAK");
> +
> +	  fprintf_unfiltered (gdb_stdlog, "Using %s for breakpoint at %s ",
> +			      bp, paddress (gdbarch, *pcptr));
> +	  if (unaligned_p)
> +	    fprintf_unfiltered (gdb_stdlog, "(unaligned address)\n");
> +	  else
> +	    fprintf_unfiltered (gdb_stdlog, "(instruction length %d)\n",
> +				riscv_insn_length (buf[0]));
> +	}
> +      if (unaligned_p || riscv_insn_length (buf[0]) == 2)
>  	return 2;
>        else
>  	return 4;

This looks good to me.  Not sure if you want to explicitly add something to
the comment that an unaligned PC is only valid if C is supported, so we
know C.EBREAK is safe to use with an unaligned PC?

-- 
John Baldwin