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

[John Baldwin]

Post by 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;

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?

[Jim Wilson]

Post by John Baldwin
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

[Andrew Burgess]

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

Looks good to me.

Thanks,
Andrew

Post by Jim Wilson
---
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

[Jim Wilson]

On Thu, Nov 1, 2018 at 4:46 PM Andrew Burgess

Post by Andrew Burgess
Looks good to me.

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

Jim