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

[Pedro Alves <palves@redhat.com>]

On 10/16/2015 07:08 PM, Antoine Tremblay wrote:

> 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.

The main advantage is that it eliminates the redundant info
in the breakpoint structures fields.  I really see no reason for every
breakpoint to have to carry around a pointer to the opcode to use,
since that's a property that can be inferred from other fields.

Less redundancy means fewer chances of things getting out of sync.

> 
> 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).
> 

The default implementation of breakpoint_from_pc can simply be:

int
default_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
{
  /* Most architectures have only one kind of software breakpoint.  */
  return 0;
}

and then most architectures would just implement the
breakpoint_from_kind method simply as:

static gdb_byte *
arch_breakpoint_from_kind (int kind /* ignored, there's only one */)
{
  return arch_breakpoint;
}

which is quite like what you already wrote.

That is, most architectures only have one 'kind' of breakpoint,
so they can ignore the parameter.  Just like in your breakpoint_from_pc
hook, most archs would ignore the length.

Note that there's always a need to implement _one_ hook on all
architecture.  In your version, it's the breakpoint_from_pc hook.
In my suggestion, it's breakpoint_from_kind.  But it's the same
number of "hooks x architectures implementations".

> 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.

I don't buy that.  The opcode to use for a particular mode can be
determined from that info alone ("which mode"), and as such doesn't
need to be stored on every breakpoint.  If you look at it from the
other angle, you can see it as factorization.

> 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.

Fair enough.

Thanks,
Pedro Alves