Re: [try 2nd 4/8] Displaced stepping for Thumb 16-bit insn

["Ulrich Weigand" <uweigand@de.ibm.com>]

Yao Qi wrote:

> 	Support displaced stepping for Thumb 16-bit insns.
> 	* arm-tdep.c (THUMB_NOP) Define.
> 	(thumb_copy_unmodified_16bit): New.
> 	(thumb_copy_b, thumb_copy_bx_blx_reg): New.
> 	(thumb_copy_alu_reg): New.
> 	(arm_copy_svc): Move some common code to ...
> 	(copy_svc): ... here.  New.
> 	(thumb_copy_svc): New.
> 	(install_pc_relative): New.
> 	(thumb_copy_pc_relative_16bit): New.
> 	(thumb_decode_pc_relative_16bit): New.
> 	(thumb_copy_16bit_ldr_literal): New.
> 	(thumb_copy_cbnz_cbz): New.
> 	(cleanup_pop_pc_16bit): New.
> 	(thumb_copy_pop_pc_16bit): New.
> 	(thumb_process_displaced_16bit_insn): New.
> 	(thumb_process_displaced_32bit_insn): New.
> 	(thumb_process_displaced_insn): process thumb instruction.

This is looking pretty good now, thanks.  There is still one problem
that I noticed:

> +static int
> +thumb_copy_pop_pc_16bit (struct gdbarch *gdbarch, unsigned short insn1,
> +			 struct regcache *regs,
> +			 struct displaced_step_closure *dsc)
> +{
> +  dsc->u.block.regmask = insn1 & 0x00ff;
> +
> +  /* Rewrite instruction: POP {rX, rY, ...,rZ, PC}
> +     to :
> +
> +     (1) register list is not empty,
> +     Prepare: tmp[0] <- r8,
> +
> +     POP {rX};   PC is stored in rX
> +     MOV r8, rX; finally, PC is stored in r8
> +     POP {rX, rY, ...., rZ}
> +
> +     Cleanup: PC <-r8, r8 <- tmp[0]

It seems this approach is actually incorrect.  If you do a
  POP {rX, rY, ..., rZ, PC}
the value at the SP gets restored into rX, the value at rX+4
into rY and so on, and the value at the highest address gets
restored into PC.

With your replacement sequence, it would appear that you
instead get the value at the *lowest* address (just SP)
restored into the PC ...


Apart from this, just a few minor issues:

> +/* Common copy routine for svc instruciton.  */
>  
> +static int
> +copy_svc (struct gdbarch *gdbarch, struct regcache *regs,
> +	  struct displaced_step_closure *dsc)
> +{

I guess to keep in sync with terminology in the rest of the file,
this really should be called install_svc ...

> +      err = thumb_copy_unmodified_16bit (gdbarch, insn1,"shift/add/sub/mov/cmp",

Formatting: space after the comma (here and at a couple other places).

> +      thumb_process_displaced_32bit_insn(gdbarch, insn1, insn2, regs, dsc);
> +    }
> +  else
> +    thumb_process_displaced_16bit_insn(gdbarch, insn1, regs, dsc);

Formatting: space before (


Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com