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

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

I wrote a patch as you suggested, and attach it below.  I am not very
happy on this one, but I am open to thoughts from you and others.

>
>> -# A return value of 1 means that the software_single_step breakpoints
>> -# were inserted; 0 means they were not.
>> -F:int:software_single_step:struct frame_info *frame:frame
>> +# Return a vector of addresses on which the software single step
>> +# breakpoints are inserted.  NULL means software single step is not used.
>
> s/are inserted/should be inserted/
>

I'll fix it.

>> +F:VEC (CORE_ADDR) *:software_single_step:struct frame_info *frame:frame
>> +
>> +m:void:insert_single_step_breakpoint:struct address_space *aspace, CORE_ADDR pc:aspace, pc::insert_single_step_breakpoint::0
>>   
>>   # Return non-zero if the processor is executing a delay slot and a
>>   # further single-step is needed before the instruction finishes.
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 696105d..5dbcf7a 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -2248,11 +2248,28 @@ maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
>>     int hw_step = 1;
>>   
>>     if (execution_direction == EXEC_FORWARD
>> -      && gdbarch_software_single_step_p (gdbarch)
>> -      && gdbarch_software_single_step (gdbarch, get_current_frame ()))
>> +      && gdbarch_software_single_step_p (gdbarch))
>>       {
>> -      hw_step = 0;
>> +      struct frame_info *frame = get_current_frame ();
>> +      VEC (CORE_ADDR) * next_pcs;
>> +
>> +      next_pcs = gdbarch_software_single_step (gdbarch, frame);
>> +
>> +      if (next_pcs != NULL)
>> +	{
>> +	  int i;
>> +	  CORE_ADDR pc;
>> +	  struct address_space *aspace = get_frame_address_space (frame);
>> +
>> +	  hw_step = 0;
>> +
>> +	  for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++)
>> +	    gdbarch_insert_single_step_breakpoint (gdbarch, aspace, pc);
>> +
>> +	  VEC_free (CORE_ADDR, next_pcs);
>> +	}
>
> This pattern of starting from a VEC of addresses, and a frame and
> calling gdbarch_insert_single_step_breakpoint on each address
> appears multiple times.  Can we put it in a convenience function?

Sure, I'll do.

>
> Though the other calls in record-full.c didn't go through
> gdbarch_insert_single_step_breakpoint -- why's that?

Oh, gdbarch_insert_single_step_breakpoint should be called.  I'll fix it.

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

Commit Message

Comments

Patch