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

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

[Jim Wilson]

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;
-- 
2.17.1

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

[John Baldwin]

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

                                                                            

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

[Jim Wilson]

On Thu, Nov 1, 2018 at 3:05 PM John Baldwin <jhb@freebsd.org> wrote:
> 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?

That seemed obvious to me, but yes it is probably better to spell it
out in a comment.  I'll add that to the patch.

Jim

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

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2018-11-01 14:41:39 -0700]:

> 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.

Looks good to me.

Thanks,
Andrew


> ---
>  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;
> -- 
> 2.17.1
> 

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

[Jim Wilson]

On Thu, Nov 1, 2018 at 4:46 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> Looks good to me.

Committed, with the improved comment that John Baldwin asked for.

Jim