Re: [PATCH 1/4] Remove arm_override_mode

[Pedro Alves <palves at redhat dot com>]

On 07/20/2016 11:17 AM, Yao Qi wrote:
> On Thu, Jun 23, 2016 at 2:20 PM, Pedro Alves <palves@redhat.com> wrote:

>>
>> I liked the plan of going the gdbserver direction of storing the
>> breakpoint's "len/kind" in the breakpoint location, as a
>> separate field, instead of encoding it in the address.  Did you
>> find a problem with that approach?
>>
> 
> If we store the breakpoint's "len/kind" in bp_target_info, we need to compute
> "kind" and store it before calling target_insert_breakpoint.  The
> "kind" of single
> step breakpoint is computed by pc and current state, while the "kind" of other
> breakpoints is computed by pc.  It is difficult to get "kind" of
> breakpoint for ia64,
> but we can get store length of breakpoint to "kind" for ia64,
> 

Now clear to me that if ia64 wanted to support Z/z packets, that it'd
need different breakpoint kinds.  Seems like bp_tgt->shadow_len has the
needed info, and bp_tgt->placed_size (the kind) could be
constant (meaning only one kind).


> - call gdbarch_breakpoint_kind_from_current_state or
> gdbarch_breakpoint_kind_from_pc to get bl->target_info.placed_size,
> - call target_insert_breakpoint
>                  |
>                 +---> remote_insert_breakpoint [1]
>                 +---> memory_insert_breakpoint
>                                  |
>                                   '---> gdbarch_memory_insert_breakpoint
>                                                  |
>                                                 +--->
> ia64_memory_insert_breakpoint
>                                                  |
>                                                  +--->
> default_memory_insert_breakpoint [2]
>                                                               |
>                                                               '-->
> gdbarch_sw_breakpoint_from_kind
> 
> in this way, three gdbarch methods, breakpoint_kind_from_pc,
> sw_breakpoint_from_kind, breakpoint_kind_from_current_state, and one
> gdbarch method remote_breakpoint_from_pc is removed.  Note that
> target_info.placed_size ("kind") is used in [1] and [2].
> 
> Further, we can simplify gdbarch method breakpoint_from_pc for most archs,
> which can be the default one, while ia64 still uses its own implementation.
> 
> const unsigned char *
> default_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
> int *lenptr)
> {
>   int kind = gdbarch_breakpoint_kind_from_pc (gdbarch, *pcptr);
> 
>   return gdbarch_sw_breakpoint_from_kind (gdbarch, kind, lenptr);
> }
> 
> Of course, we need to define breakpoint_kind_from_pc and
> sw_breakpoint_from_kind for many archs, and switch breakpoint_from_pc to
> the default one.
> 
> What do you think?  I've already got arm working in this way, but still need
> to convert other archs.
> 

That sounds good.  If it makes it easier, feel free to post a patch with
only ARM, ia64 and couple other archs, so we can see/discuss the core parts,
before doing the rest of the (seemingly) mechanical arch conversions.

Thanks,
Pedro Alves