Re: [PATCH 1/2] gdbarch software_single_step returns VEC (CORE_ADDR) *

[Pedro Alves <palves at redhat dot com>]

On 03/30/2016 03:14 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> I think the problem is the ambiguity in arm_breakpoint_from_pc,
>> and to the fact that we don't "normalize" breakpoint addresses
>> before passing them down to target_insert_breakpoint. 
> 
> We don't pass address to target_insert_breakpoint, instead, we pass
> bp_target_info. 

Yeah, well, we pass the address _in_ a bp_target_info, but we
pass the address.

> If we do what you suggested, we need to set
> "normalized" address into bl->target_info.reqstd_address.  This is the
> only way to pass normalized address down to target_insert_breakpoint.

Right.

> This means we propagate the ISA specific bits of address to breakpoint
> system, I don't see anything harmful so far, but I feel risky to do so.
> 
>>
>> Some callers start with an address coming from the user and want
>> to consult symbol tables / mapping symbols.  Other caller really
>> want to trust the thumb bit as set in the address.
> 
> Right.  If we want to fully trust on the bit of address, in some cases,
> we need to consult symbols to normalize the address.

Yeah.

> The question is
> "in what cases do we do so?".  In my experimental patch, address is
> normalized for breakpoints except single step breakpoint.
> 
>>
>> Note that arm_breakpoint_from_pc uses arm_pc_is_thumb, which is
>> what consults symbol tables / mapping symbols.
>>
>> I think the fix would to make arm_breakpoint_from_pc always trust
>> that the address bit is already encoded correctly, and trust
>> IS_THUMB_ADDR, similarly to how the gdbserver version does, in
>> arm_breakpoint_kind_from_pc.
> 
> If so, we need to normalize addresses before calling
> gdbarch_breakpoint_from_pc.  Moreover, we need to normalize address
> before calling insert_single_step_breakpoint out side of
> gdbarch_software_single_step.
> 
>>
>> Then, we'd still need to consult the mapping symbols
>> consultation, or IOW, do something based on arm_pc_is_thumb _before_
>> target_insert_breakpoint is reached.  That is, call something like
>> arm_pc_is_thumb and use the result to encode the thumb bit correctly in
>> the address passed to target_insert_breakpoint.  IOW, "normalize" the
>> target address, using some gdbarch method, _before_ that address is passed
>> to the target routines in the first place.
>>
>> Along the way, several other functions would stop using arm_pc_is_thumb,
>> but use IS_THUMB_ADDR directly.  E.g., arm_remote_breakpoint_from_pc.
>>
>> WDYT?

Another similar/related idea would be to go 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:

  /* Return the breakpoint kind for this target based on PC.  The PCPTR is
     adjusted to the real memory location in case a flag (e.g., the Thumb bit on
     ARM) was present in the PC.  */
  int (*breakpoint_kind_from_pc) (CORE_ADDR *pcptr);

  /* Return the software breakpoint from KIND.  KIND can have target
     specific meaning like the Z0 kind parameter.
     SIZE is set to the software breakpoint's length in memory.  */
  const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);


struct bp_target_info already has the "placed_size" field,
maybe we could reuse it, just like we started from the "len"
field on gdbserver, in 271652949894 (Support breakpoint kinds for 
software breakpoints in GDBServer).

In effect, this would move the gdbarch_remote_breakpoint_from_pc
calls to common code, just like it happened in gdbserver.

So when setting a single-step breakpoint, we'd get the "kind"
from the current mode, and when setting breakpoints from
user-input, we'd get it from the symbols tables / mapping symbols.

This would probably be the cleanest, and would expose the most
sharing opportunities with gdbserver.

I think it'd avoid the encoding <-> decoding juggling to get
at the "kind" that we see in your patch.

> @@ -809,6 +810,7 @@ void
>  default_remote_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
>  				   int *kindptr)
>  {
> +  *pcptr = gdbarch_addr_bits_encode  (gdbarch, *pcptr);
>    gdbarch_breakpoint_from_pc (gdbarch, pcptr, kindptr);
>  }

Would we need this one?  This is called from remote.c:remote_insert_breakpoint,
which already has the addr bit encoded in the PC.

Thanks,
Pedro Alves