Re: Why enforcing sw_breakpoint_from_kind() implementation in GDBserver targets

Luis Machado
2020-06-17T22:20:12Z

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.