[1/4] Remove arm_override_mode

  This patch removes global variable arm_override_mode.  The rationale is
that if the address is the next address of current pc, we can get the
thumb/arm mode from dest address computed by software single step code.

gdb:

2016-05-12  Yao Qi  <yao.qi@linaro.org>

	* arm-tdep.c: Include "gdbthread.h".
	(arm_override_mode): Remove.
	(arm_pc_is_thumb): Remove arm_override_mode.  If MEMADDR
	is the dest address of current pc, get thumb mode by dest address.
	(arm_insert_single_step_breakpoint): Remove arm_override_mode
	and cleanup.
---
 gdb/arm-tdep.c | 57 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 16 deletions(-)
  

On 05/12/2016 05:18 PM, Yao Qi wrote:
> This patch removes global variable arm_override_mode.  The rationale is
> that if the address is the next address of current pc, we can get the
> thumb/arm mode from dest address computed by software single step code.
> 

If we're still doing something special inside arm_pc_is_thumb
for a particular caller, how's this better than the global?
It looks way more complicated, and fragile.  It looks like the sort
of thing that could break easily, since the function now
behaves differently depending on the currently select thread.  :-/

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?

Thanks,
Pedro Alves
  

On Thu, Jun 23, 2016 at 2:20 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/12/2016 05:18 PM, Yao Qi wrote:
>> This patch removes global variable arm_override_mode.  The rationale is
>> that if the address is the next address of current pc, we can get the
>> thumb/arm mode from dest address computed by software single step code.
>>
>
> If we're still doing something special inside arm_pc_is_thumb
> for a particular caller, how's this better than the global?
> It looks way more complicated, and fragile.  It looks like the sort
> of thing that could break easily, since the function now
> behaves differently depending on the currently select thread.  :-/

The reason that I choose this approach is that the alternative (see below)
is quite expensive.

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

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

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
  

On 07/22/2016 11:15 AM, Pedro Alves wrote:

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

Happened to re-read this back now and noticed a typo that changes
meaning...  FAOD, I meant:

 NOT clear to me that if ia64 wanted to support Z/z packets, that it'd
 need different breakpoint kinds.

Thanks,
Pedro Alves