Why enforcing sw_breakpoint_from_kind() implementation in GDBserver targets

[Luis Machado]

On 6/17/20 6:31 PM, Shahab Vahedi wrote:
> Hi Baris,
> 
> Sorry for my late reply and thank you again for your thorough response.
> 
> On Tue, Jun 16, 2020 at 01:15:29PM +0000, Aktemur, Tankut Baris wrote:
>> 1. In GDB 9 (i.e. before C++'ification of gdbserver's target definition),
>> the base linux target implemented the "sw_breakpoint_from_kind" target op
>> to directly delegate to "sw_breakpoint_from_kind" of the linux low target.
>> It asserted that the low target's "sw_breakpoint_from_kind" function pointer
>> is non-null.
> 
> And this is how the ARC port at the time looked like:
> gdb/gdbserver/linux-arc-low.c [1]:
> ...
> 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,
> };
> ...
> This did not hit the assert in base linux implementation, since
> "linux_sw_breakpoint_from_kind" was never executed. It's because
> the GDB client was writing the breakpoint opcodes to the address
> through gdbserver while bypassing sw_breakpoint_from_kind().
> 
>> As far as I understand, there is a certain control flow and a combination
>> of (un)supported features in the ARC port with which the "sw_breakpoint_from_kind"
>> target op is not invoked.  I'm not sure what this flow or feature combination is.
> 
> I guess what I explained above should be the case. I do not know
> the exact control flow that led/leads to this though.
>> Just out of curiosity, is there a specific reason why you want to avoid implementing
>> the "sw_breakpoint_from_kind" target op?
> 
> No, there is no reason that I might be against it. I brought this whole thing
> up because I noticed ARC did not have sw_breakpoint_from_kind implemented and
> it was working fine. Even after I implemented the pure virtual method [2], it
> does not get executed (I put an assert in a temporary implementation as an
> experiment). So it seemed like an unnecessary constraint for compilation.
> If we want to deprecate how ARC inserts breakpoints (GDB client writing
> the opcode through GDBserver) then that is fine [3]. If not, then implementing
> sw_breakpoint_from_kind() shouldn't be mandatory.
> 
> I just wanted to make sure that this message is conveyed. That's all :)
> 
> For the record, I plan to change ARC's behavior to really use
> sw_breakpoint_from_kind(), but that is not the purpose of this email.
> 
> 
> Shahab
> 
> [1]
> https://github.com/foss-for-synopsys-dwc-arc-processors/binutils-gdb/blob/arc-2019.09-gdb/gdb/gdbserver/linux-arc-low.c#L300
> 
> [2]
> https://github.com/foss-for-synopsys-dwc-arc-processors/binutils-gdb/blob/arc-2020.09/gdbserver/linux-arc-low.cc#L350
> 
> [3]
> Please keep in mind that if deprecation is wanted, yet still nothing is
> stopping ARC (and the likes) from bypassing sw_breakpoint_from_kind().
> 

I think commit dd373349578df87396bc43e7ab00a1a5ceb16c8b made 
sw_breakpoint_from_kind required, but under the old C structure, so no 
compilation errors for whoever didn't implement it.  But it seems this 
doesn't always get used. In fact, it may only be useful to ARM/AArch32 
and RISCV.

The fact that multiple backends implement it the same way indicates a 
good candidate for a linux-low implementation that only returns the 
necessary default bits.

Most of the implementations completely ignore the KIND parameter anyway.