Re: [PATCH] Only use 32-bit thumb-2 breakpoint instruction if necessary

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>>> We can't keep GDB and GDBserver in sync because we may have old gdb
>>> talks with new gdbserver or new gdb talks with old gdbserver.  If an old
>>> gdb talks with a new gdbserver, gdb checks program_breakpoint_here_p by
>>> reading an instruction and see if it is a breakpoint instruction.  If
>>> the original instruction is a 32-bit thumb instruction, gdbserver uses
>>> 16-bit thumb breakpoint, but gdb expects to match 32-bit thumb
>>> breakpoint.  IOW, gdb and gdbserver may see/use different kinds of
>>> breakpoint on the same address (instruction), but gdb and gdbserver
>>> can't do that.
>>
>> Right. Do you see any possible solution to this ?
>>
>
> I don't have any solutions off hand.  Nowadays, GDB chooses the
> breakpoint kind by address and the instruction on that address
> in arm_breakpoint_kind_from_pc,
>
> 	  if (target_read_memory (*pcptr, buf, 2) == 0)
> 	    {
> 	      unsigned short inst1;
>
> 	      inst1 = extract_unsigned_integer (buf, 2, byte_order_for_code);
> 	      if (thumb_insn_size (inst1) == 4)
> 		return ARM_BP_KIND_THUMB2;
> 	    }
>
> we can change it to show breakpoints in target_read_memory,
>
>   /* Make sure we see the memory breakpoints.  */
>   cleanup = make_show_memory_breakpoints_cleanup (1);
>   if (target_read_memory (*pcptr, buf, 2) == 0)
>     {
>        do_cleanups (cleanup);
>
>        /* The remote stub may choose 16-bit thumb breakpoint for
>           32-bit thumb instruction if the instruction is out side
>           of IT block.   */
>        if (memcmp (buf, tdep->thumb_breakpoint) == 0)
>           return ARM_BP_KIND_THUMB;
>
>        unsigned short inst1;
>
>        inst1 = extract_unsigned_integer (buf, 2, byte_order_for_code);
>        // Suppose thumb_insn_size returns 4 for the first half of 32-bit
>        // thumb-2 breakpoint.
>        if (thumb_insn_size (inst1) == 4)
> 	 return ARM_BP_KIND_THUMB2;
>     }
>    do_cleanups (cleanup);
>

Yes that would help...

>> The only thing I would see is to accept inconsistent breakpoint
>> sizes GDBServer.
>
> In GDBserver, all breakpoints (gdb breakpoints, single-step breakpoints)
> are organized via raw_breakpoint, is it possible that 32-bit thumb
> breakpoint is use for gdb breakpoint and 16-bit thumb breakpoint is used
> for single-step breakpoint for the same address?

Yes, tests reveal that this is an issue and fail with the breakpoint
removed because of its inconsistent kind.

> It makes troubles to
> raw_breakpoint management, as we find a raw_breakpoint for a given kind
> on a given address.I don't know what GDBserver should do, so I can't
> give much help here, sorry.

Yes at first I though what's the harm in allowing different breakpoint
kinds on an address ?

But looking at the code that introduced the check there seems to be complex
reasons for that.

>I would like to change GDBserver code, and fix the regressions caused by the change.

I'm not sure what you mean ? You want to take over this issue ? (not a
problem with me)

I think we may forget this change for 8.0 however... ?