| Message ID | e6a0ee4f-1230-d48f-d085-a3bfb46527ab@FreeBSD.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Sep 20, 2018 at 1:31 PM John Baldwin <jhb@freebsd.org> wrote:
> Use the existing instruction to determine the RISC-V breakpoint kind.
Yes, this looks like a nice solution to the problem.
In riscv_breakpoint_kind_from_pc, you are setting a byte_order local
variable but not obviously using it. You presumably don't need byte
order here as you are only reading one byte.
Jim
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 > >
On 9/20/18 1:56 PM, Jim Wilson wrote: > On Thu, Sep 20, 2018 at 1:31 PM John Baldwin <jhb@freebsd.org> wrote: >> Use the existing instruction to determine the RISC-V breakpoint kind. > > Yes, this looks like a nice solution to the problem. > > In riscv_breakpoint_kind_from_pc, you are setting a byte_order local > variable but not obviously using it. You presumably don't need byte > order here as you are only reading one byte. Oh, very true. I had just copied it from riscv_insn::fetch_instruction. I'll fix and post a V2.
On 9/20/18 2:51 PM, Andrew Burgess wrote: > * John Baldwin <jhb@FreeBSD.org> [2018-09-20 13:31:46 -0700]: >> @@ -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. Will fix. >> + 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. Ok. > 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. Ok. I'll reproduce that and figure out the fix and include it in a V2 patch. One other question is if I drop the change to default MISA to 0, we should perhaps fix the comment above riscv_read_misa? The comment implies that it falls back to zero if it can't read the register and it does that for the !target_has_registers case already. It's not clear from the comment that targets are required to provide MISA.
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); + gdb_byte buf[1]; + int status; + + /* Read the opcode byte to determine the instruction length. */ + status = target_read_memory (*pcptr, buf, 1); + 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,