Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer.

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

On 10/16/2015 12:06 PM, Pedro Alves wrote:
> GDB calls them "placed address" and "requested address":
>
> struct bp_target_info
> {
> ...
>    /* Address at which the breakpoint was placed.  This is normally
>       the same as REQUESTED_ADDRESS, except when adjustment happens in
>       gdbarch_breakpoint_from_pc.  The most common form of adjustment
>       is stripping an alternate ISA marker from the PC which is used
>       to determine the type of breakpoint to insert.  */
>    CORE_ADDR placed_address;
>
>    /* Address at which the breakpoint was requested.  */
>    CORE_ADDR reqstd_address;

Nice I had not seen that !

I'll change pcfull for reqstd_address;

But leave pc as pc for now, we can change it in a later patch.

> I don't see why my comment conflicts with Yao's.  But I think we
> could simplify the interfaces and entry points, and get rid of the
> duplication, like this:
>
> Replace the breakpoint_from_pc method with a breakpoint_kind_from_pc
> method.  This adjusts the PC (if necessary) and returns the
> breakpoint _kind_ instead of the breakpoint opcode / data.
>
> enum arm_breakpoint_kinds
> {
>     ARM_BP_KIND_THUMB = 2,
>     ARM_BP_KIND_THUMB2 = 3,
>     ARM_BP_KIND_ARM = 4,
> };
>
> static int
> arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr, int len)
> {
>    if (IS_THUMB_ADDR (*pcptr))
>      {
>        gdb_byte buf[2];
>
>        *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
>
>        /* Check whether we are replacing a thumb2 32-bit instruction.  */
>        if ((*the_target->read_memory) (*pcptr, buf, 2) == 0)
> 	{
> 	  unsigned short inst1 = 0;
>
> 	  (*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2);
> 	  if (thumb_insn_size (inst1) == 4)
>              return ARM_BP_KIND_THUMB2;
> 	}
>
>        return ARM_BP_KIND_THUMB;
>      }
>    else
>      return ARM_BP_KIND_ARM;
> }
>
> Then the breakpoints functions and structures always work
> with the already-adjusted PC, and with a breakpoint-kind.
>
> for internal breakpoints, we have:
>
>    set_breakpoint_at (breakpoint_kind_from_pc, to find bp kind,
>                       rest the same as today)
>    set_gdb_breakpoint_1 (same as today)
>       |
>       `--> set_breakpoint (address, kind)
>               |
>               `-->set_raw_breakpoint_at (address, kind)
>                      |
>                      `--> the_target->insert_point (address, kind)
>
> Everything thinks in terms of breakpoint kind.  Then the only
> places that need to know the real breakpoint instruction opcode
> and opcode size can query the breakpoint_from_kind target method
> you already added.
>
> About:
>
> > We could but then we would have to call breakpoint_from_kind in a lot of
> > places basically everywhere bp->size is referenced like :
> >
> > check_mem_read
> > check_mem_write
> > insert_memory_breakpoint
> > remove_memory_breakpoint
> > set_raw_breakpoint_at
> > validate_inserted_breakpoint
> > delete_raw_breakpoint
> > uninsert_raw_breakpoint
> > reinsert_raw_breakpoint
> > find_raw_breakpoint_at
>
> Minimizing the patch size is less important than making sure the
> resulting code is clear

I was thinking about making the code clear.

> Sounds like that's manageable with a trivial replace of bp->size
> with a call to something like:
>
> static int
> bp_size (struct raw_breakpoint *bp)
> {
>     int size = bp->kind;
>
>     breakpoint_from_kind (&size);
>     return size;
> }
>
> Likewise for the opcode data:
>
> static const gdb_byte *
> bp_opcode (struct raw_breakpoint *bp)
> {
>     int size = bp->kind;
>
>     return breakpoint_from_kind (&size);
> }
>
> Doesn't seem to me like the end result would be any less clear.

I see what you mean more clearly now.

I like the idea to treat only in kinds but I'm not sure about the 
advantage in terms of clarity.

I would say it's debatable like you said in the end result is not any 
less clear but the current implementation doesn't seem less clear to me 
either.

I do not like the detour we need to make when we do not have a real 
reason to have a kind, adding a "fake" unique kind and having to add 
breakpoint_from_kind implementations on all architectures, not just the 
ones that support software breakpoints. (Regardless of the patch size).

Also even if we can call functions to get the size and opcode when 
needed, it still seems like since those values do not change for the 
life of the breakpoint and that they can be set only once from a 
meaningful context it's perfectly acceptable and more clear to set them 
once and avoid this level of abstraction to access these values.

At this point I could do either but to avoid redoing the patch set 
multiple times, I would like to ask for Yao's opinion and I will work on 
the consensual option.

Regards,
Antoine