This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/4] Fall back to a default value of 0 for the MISA register.
This look like a great improvement. I have one issue inline below...
* John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]:
> On 9/19/18 5:40 PM, John Baldwin wrote:
> > Whether or not 'C' is present is a bit more subtle as it isn't something
> > you could infer just from a register being present or not. On the other
> > hand, for breakpoints, I wonder if a more straightforward approach would
> > be to examine the opcode of the existing instruction and use that to
> > decide which breakpoint to use? That is, read the first byte of the
> > existing instruction and if the low 2 bits indicate a 16-bit instruction
> > use C.BREAK and otherwise use BREAK?
>
> This seems to work for me in my testing (I used 'stepi' to step across a
> set of instructions with a mix of regular and 'C' instructions and it
> worked correctly). You can use 'set debug riscv breakpoints 1' to see what
> it decides each time it sets a breakpoint. I haven't yet tested if this
> means I can remove the MISA patch, but I suspect it does as the only
> place that needs MISA after this is the riscv_isa_flen() function.
>
> Author: John Baldwin <jhb@FreeBSD.org>
> Date: Thu Sep 20 10:12:22 2018 -0700
>
> Use the existing instruction to determine the RISC-V breakpoint kind.
>
> RISC-V supports instructions of varying lengths. Standard existing
> instructions in the base ISA are 4 bytes in length, but the 'C'
> extension adds support for compressed, 2 byte instructions. RISC-V
> supports two different breakpoint instructions: EBREAK is a 4 byte
> instruction in the base ISA, and C.EBREAK is a 2 byte instruction only
> available on processors implementing the 'C' extension. Using EBREAK
> to set breakpoints on compressed instructions causes problems as the
> second half of EBREAK will overwrite the first 2 bytes of the
> following instruction breaking other threads in the process if their
> PC is the following instruction. Thus, breakpoints on compressed
> instructions need to use C.EBREAK instead of EBREAK.
>
> Previously, the riscv architecture checked the MISA register to
> determine if the 'C' extension was available. If so, it used C.EBREAK
> for all breakpoints. However, the MISA register is not necessarily
> available to supervisor mode operating systems. While native targets
> could provide a fake MISA register value, this patch instead examines
> the existing instruction at a breakpoint target to determine which
> breakpoint instruction to use. If the existing instruction is a
> compressed instruction, C.EBREAK is used, otherwise EBREAK is used.
>
> gdb/ChangeLog:
>
> * riscv-tdep.c (show_use_compressed_breakpoints): Remove
> 'additional_info' and related logic.
> (riscv_debug_breakpoints): New variable.
> (riscv_breakpoint_kind_from_pc): Use the length of the existing
> instruction to determine the breakpoint kind.
> (_initialize_riscv_tdep): Add 'set/show debug riscv breakpoints'
> flag. Update description of 'set/show riscv
> use-compressed-breakpoints' flag.
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 75032b7a4d..df4561d319 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,14 @@
> +2018-09-20 John Baldwin <jhb@FreeBSD.org>
> +
> + * riscv-tdep.c (show_use_compressed_breakpoints): Remove
> + 'additional_info' and related logic.
> + (riscv_debug_breakpoints): New variable.
> + (riscv_breakpoint_kind_from_pc): Use the length of the existing
> + instruction to determine the breakpoint kind.
> + (_initialize_riscv_tdep): Add 'set/show debug riscv breakpoints'
> + flag. Update description of 'set/show riscv
> + use-compressed-breakpoints' flag.
> +
> 2018-09-19 John Baldwin <jhb@FreeBSD.org>
>
> * Makefile.in (ALLDEPFILES): Add riscv-fbsd-nat.c.
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 4b385ed5da..cd8dbaf9f6 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -210,20 +210,9 @@ show_use_compressed_breakpoints (struct ui_file *file, int from_tty,
> struct cmd_list_element *c,
> const char *value)
> {
> - const char *additional_info;
> - struct gdbarch *gdbarch = target_gdbarch ();
> -
> - if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
> - if (riscv_has_feature (gdbarch, 'C'))
> - additional_info = _(" (currently on)");
> - else
> - additional_info = _(" (currently off)");
> - else
> - additional_info = "";
> -
> fprintf_filtered (file,
> _("Debugger's use of compressed breakpoints is set "
> - "to %s%s.\n"), value, additional_info);
> + "to %s.\n"), value);
> }
>
> /* The set and show lists for 'set riscv' and 'show riscv' prefixes. */
> @@ -284,6 +273,11 @@ show_riscv_debug_variable (struct ui_file *file, int from_tty,
> c->name, value);
> }
>
> +/* When this is set to non-zero debugging information about breakpoint
> + kinds will be printed. */
> +
> +static unsigned int riscv_debug_breakpoints = 0;
> +
> /* When this is set to non-zero debugging information about inferior calls
> will be printed. */
>
> @@ -426,7 +420,22 @@ riscv_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
> {
> if (use_compressed_breakpoints == AUTO_BOOLEAN_AUTO)
> {
> - if (riscv_has_feature (gdbarch, 'C'))
> + enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
byte_order is unused.
> + gdb_byte buf[1];
> + int status;
> +
> + /* Read the opcode byte to determine the instruction length. */
> + status = target_read_memory (*pcptr, buf, 1);
This should use target_read_code. I know that we already have some
(incorrect) uses of target_read_memory in riscv-tdep.c, but we can fix
those later.
However, this causes a testsuite regression for gdb.gdb/unittest.exp.
You can easily reproduce the issue with:
(gdb) maintenance selftest
We probably need to add a riscv specific case into
disasm-selftest.c:print_one_insn_test, lots of other targets already
do.
Otherwise, I think this is good.
Thanks,
Andrew
> + if (status)
> + memory_error (TARGET_XFER_E_IO, *pcptr);
> +
> + 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)
> return 2;
> else
> return 4;
> @@ -2945,6 +2954,16 @@ _initialize_riscv_tdep (void)
> &showdebugriscvcmdlist, "show debug riscv ", 0,
> &showdebuglist);
>
> + add_setshow_zuinteger_cmd ("breakpoints", class_maintenance,
> + &riscv_debug_breakpoints, _("\
> +Set riscv breakpoint debugging."), _("\
> +Show riscv breakpoint debugging."), _("\
> +When non-zero, print debugging information for the riscv specific parts\n\
> +of the breakpoint mechanism."),
> + NULL,
> + show_riscv_debug_variable,
> + &setdebugriscvcmdlist, &showdebugriscvcmdlist);
> +
> add_setshow_zuinteger_cmd ("infcall", class_maintenance,
> &riscv_debug_infcall, _("\
> Set riscv inferior call debugging."), _("\
> @@ -2982,9 +3001,9 @@ of the stack unwinding mechanism."),
> Set debugger's use of compressed breakpoints."), _(" \
> Show debugger's use of compressed breakpoints."), _("\
> Debugging compressed code requires compressed breakpoints to be used. If\n \
> -left to 'auto' then gdb will use them if $misa indicates the C extension\n \
> -is supported. If that doesn't give the correct behavior, then this option\n\
> -can be used."),
> +left to 'auto' then gdb will use them if the existing instruction is a\n \
> +compressed instruction. If that doesn't give the correct behavior, then\n \
> +this option can be used."),
> NULL,
> show_use_compressed_breakpoints,
> &setriscvcmdlist,
>
> --
> John Baldwin
>
>