Re: [PATCH v4 3/6] Refactor arm_software_single_step to use regcache.

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

On 12/03/2015 05:28 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> Hi Antoine,
>
> > 	* common/common-regcache.h (register_status) New enum.
> > 	(regcache_raw_read_unsigned): New declaration.
> > 	* regcache.h (enum register_status): Move to common-regcache.h.
> > 	(regcache_raw_read_unsigned): Likewise.
> >
> > gdb/gdbserver/ChangeLog:
> >
> > 	* regcache.c (regcache_raw_read_unsigned): New function.
> > 	* regcache.h (REG_UNAVAILABLE, REG_VALID): Replaced by shared
> > 	register_status enum.
> > 	(init_register_cache): Initialize cache to REG_UNAVAILABLE.
>
> Adding regcache_raw_read_unsigned in GDBserver looks unrelated to this
> patch.  I know patch #4 will use regcache_raw_read_unsigned in
> GDBserver, so this change can be patch 3.5.

Ok. I can split that.

> >      another breakpoint by our caller.  */
> >
> >   static CORE_ADDR
> > -thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
> > +thumb_get_next_pc_raw (struct regcache *regcache, struct frame_info *frame,
> > +		       CORE_ADDR pc)
>
> Do we still need frame?  it should be removed.

Unfortunately we do for the syscall_next_pc target dependent call.

The reason I did not change syscall_next_pc for regcache is that this 
would require the implementation of a syscall_next_pc that is not using 
the frame unwinders and directly inspects the stack for multiple targets.

This would need to be done for arm,  mips, nios2, and tix6x.

software_single_step would also have to be adapted for mips, nios2 and 
tix6x for this new api.

While I do understand the intention to get rid of frame completely in 
software_single_step and I accepted to do some of this refactoring as 
part of this series like we discussed in option #1 in a previous email,
I think the syscall_next_pc refactoring should be left for option #2 as 
after all it does not have much to do with supporting software single 
step on GDBServer which this path series is about.

Also I do not have mips,nios2 or tix6x which makes me uneasy about doing 
this without testing. (My request for the gcc buildfarm is still pending.)

So for now, I ask that we keep frame for syscall_next_pc and allow 
support for software single step on ARM on GDBserver to make progress.

It will not be more difficult to refactor after this patch series the 
syscall_next_pc call along with all the other apis that would need to be 
changed for gdbarch software_single_step to use regcache globally.

> > @@ -4746,20 +4775,22 @@ thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
> >      address.  */
> >
> >   static CORE_ADDR
> > -arm_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
> > +arm_get_next_pc_raw (struct regcache *regcache, struct frame_info *frame,
> > +		     CORE_ADDR pc)
>
> Likewise.
>
> > @@ -5019,14 +5057,15 @@ arm_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
> >      loop is detected.  */
> >
> >   CORE_ADDR
> > -arm_get_next_pc (struct frame_info *frame, CORE_ADDR pc)
> > +arm_get_next_pc (struct regcache *regcache, struct frame_info *frame,
> > +		 CORE_ADDR pc)
> >   {
>
> Likewise.
>
> > diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
> > index b9311fe..c608bf3 100644
> > --- a/gdb/gdbserver/regcache.c
> > +++ b/gdb/gdbserver/regcache.c
> > @@ -145,8 +145,9 @@ init_register_cache (struct regcache *regcache,
> >   	= (unsigned char *) xcalloc (1, tdesc->registers_size);
> >         regcache->registers_owned = 1;
> >         regcache->register_status
> > -	= (unsigned char *) xcalloc (1, tdesc->num_registers);
> > -      gdb_assert (REG_UNAVAILABLE == 0);
> > +	= (unsigned char *) xmalloc (tdesc->num_registers);
> > +      memset ((void *) regcache->register_status, REG_UNAVAILABLE,
> > +	      tdesc->num_registers);
>
> Odd indentation.

Humm what is odd exactly ?

> >   #else
> >         gdb_assert_not_reached ("can't allocate memory from the heap");
> >   #endif
> > @@ -435,6 +436,27 @@ collect_register (struct regcache *regcache, int n, void *buf)
> >   	  register_size (regcache->tdesc, n));
> >   }
> >
> > +enum register_status
> > +regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
> > +			    ULONGEST *val)
>
> Odd indentation.

Likewise ?