Why enforcing sw_breakpoint_from_kind() implementation in GDBserver targets

[Shahab Vahedi <shahab.vahedi@gmail.com>]

Hello,

I  believe  that  the  "sw_breakpoint_from_kind()"  method  declared  in
gdbserver/target.h should not be a pure virtual method.  Please read  on
for further details.

Recently, I have rebased our code base of ARC GDB [1] onto the master of
binutils-gdb [2]. During this process, I noticed that "linux target ops"
have become methods [3].

I noticed that the proto type of "sw_breakpoint_from_kind()" has  become
[4]:

-------------------------[ gdbserver/target.h ]-------------------------
  ...
  class process_stratum_target
  {
  public:
    ...
    virtual const gdb_byte *
      sw_breakpoint_from_kind (int kind, int *size) = 0;
    ...
  };
  ...
------------------------------------------------------------------------

This means "sw_breakpoint_from_kind()" is a  pure  virtual  method  now.
Any  class  inheriting  "process_stratum_target"  or   its   derivatives
("linux_process_target"  in  particular)  MUST  implement  this  method.
Else, the compilation will fail.

In ARC's case, the GDBserver code did not have this  method  implemented
[5]:

---------------------[ gdbserver/linux-arc-low.cc ]---------------------
  ...
  struct linux_target_ops the_low_target =
  {
    arc_arch_setup,
    arc_regs_info,
    arc_cannot_fetch_register,
    arc_cannot_store_register,
    NULL,                      /* fetch_register  */
    linux_get_pc_32bit,
    linux_set_pc_32bit,
    NULL,                      /* breakpoint_kind_from_pc  */
    NULL,                      /* sw_breakpoint_from_kind  */
    NULL,                      /* get_next_pcs  */
    0,                         /* decr_pc_after_break  */
    arc_breakpoint_at,
  };
  ...
------------------------------------------------------------------------

Now, it has to provide the implementation for "sw_breakpoint_from_kind".
That is not the whole story.  I also noticed that this  piece  of  newly
implemented code never gets executed.  This makes sense because we  have
a setup  that  looks  like  below  (both  entities  are  running  inside
GNU/Linux):

 ,------------------------------------------------.
 | ARC GDB client on x86 machine (cross debugger) |
 `------------------------------------------------'
                        /\
                        ||
                 remote debugging
                        ||
                        \/
 ,------------------------------------------------.
 |   ARC GDBserver on ARC board (native server)   |
 `------------------------------------------------'

It is the "ARC GDB client" that inserts the breakpoint.  That has always
been the case.  Else, it would be impossible to  break  while  debugging
with GDBserver in older implementation (before  rebase).   It  is  worth
mentioning the "ARC GDB client" does have the  "sw_breakpoint_from_kind"
implemented [].

Last  but  not  least,  one  nitpick:  Even  though  I  have  added  the
implementation of "sw_breakpoint_from_kind", I have never  done  so  for
"breakpoint_kind_from_pc"    or    "breakpoint_kind_from_current_state".
These last two are supposed to provide  the  "kind"  that  will  be  the
input parameter for "sw_breakpoint_from_kind".  Therefore, even  if  the
new piece of "sw_breakpoint_from_kind" would be executed, that would  be
problematic.  I'm not sure what can be done about this but I think  _if_
"sw_breakpoint_from_kind"     should     be    mandatory,     so     are
"breakpoint_kind_from_pc" and "breakpoint_kind_from_current_state".


Cheers,
Shahab


[1] ARC GDB repository
https://github.com/foss-for-synopsys-dwc-arc-processors/binutils-gdb

[2] Head of the master at the time of rebase
commit: cebd6b8ac1c5a2a847a50e3efe932ff2d0867b3e
IFUNC: Update IFUNC resolver check with DT_TEXTREL

[3] Commits for converting "linux target ops" to methods
0dd7b52ede3 gdbserver/linux-low: delete 'linux_target_ops' and 'the_low_target'
fc5ecdb6304 gdbserver/linux-low: turn 'get_ipa_tdesc_idx' into a method
9eedd27d42c gdbserver/linux-low: turn 'get_syscall_trapinfo' into a method
b31cdfa69f4 gdbserver/linux-low: turn 'supports_hardware_single_step' into a method
9cfd8715514 gdbserver/linux-low: turn 'supports_range_stepping' into a method
ab64c99982e gdbserver/linux-low: turn 'emit_ops' into a method
809a0c354b9 gdbserver/linux-low: turn fast tracepoint ops into methods
13e567af27e gdbserver/linux-low: turn 'get_thread_area' into a method
47f70aa7685 gdbserver/linux-low: turn 'supports_tracepoints' into a method
a5b5da9258d gdbserver/linux-low: turn 'process_qsupported' into a method
d7599cc0826 gdbserver/linux-low: turn 'prepare_to_resume' into a method
fd000fb3dfd gdbserver/linux-low: turn process/thread addition/deletion ops into methods
cb63de7ca80 gdbserver/linux-low: turn 'siginfo_fixup' into a method
b35db73327c gdbserver/linux-low: turn '{collect, supply}_ptrace_register' into methods
ac1bbaca106 gdbserver/linux-low: turn watchpoint ops into methods
9db9aa232ac gdbserver/linux-low: turn 'insert_point' and 'remove_point' into methods
007c9b975dc gdbserver/linux-low: turn 'supports_z_point_type' into a method
d7146cda56c gdbserver/linux-low: turn 'breakpoint_at' into a method
d4807ea231e gdbserver/linux-low: turn the 'decr_pc_after_break' field into a method
7582c77c1d2 gdbserver/linux-low: turn 'supports_software_single_step' and 'get_next_pcs' into methods
3ca4edb6617 gdbserver/linux-low: turn 'sw_breakpoint_from_kind' into a method
06250e4e67c gdbserver/linux-low: turn 'breakpoint_kind_from_{pc, current_state}' into methods
bf9ae9d8c37 gdbserver/linux-low: turn 'get_pc' and 'set_pc' into methods
df95181f00d gdbserver/linux-low: turn some more static functions into private methods
bd70b1f240b gdbserver/linux-low: turn 'fetch_register' into a method
daca57a7de5 gdbserver/linux-low: turn 'cannot_{fetch/store}_register' into methods
aa8d21c9bb4 gdbserver/linux-low: turn 'regs_info' into a method
797bcff595c gdbserver/linux-low: turn 'arch_setup' into a method
ef0478f6112 gdbserver/linux-low: start turning linux target ops into methods

[4] gdbserver: turn breakpoint kind-related target ops into methods
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=d367006fb7cf

[5] ARC's GDB server code before rebase; not providing sw_breakpoint_from_kind()
https://github.com/foss-for-synopsys-dwc-arc-processors/binutils-gdb/blob/arc-2020.03/gdbserver/linux-arc-low.cc#L311

[6] ARC's GDB client having the "sw_breakpoint_from_kind" method
https://github.com/foss-for-synopsys-dwc-arc-processors/binutils-gdb/blob/arc-2020.03/gdb/arc-linux-tdep.c#L319