Re: [patch 2/3] Displaced stepping for 16-bit Thumb instructions

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

Yao Qi wrote:

> +/* Record an ARM mode instruction in one slot.  */
> +#define RECORD_ARM_MODE_INSN(INDEX, INSN) do \
> +{\
> +  dsc->modinsn[INDEX] = INSN;\
> + } while (0)
> +
> +#define RECORD_THUMB_MODE_INSN(INDEX, INSN) do \
> +{\
> +  dsc->modinsn[INDEX] = INSN;\
> + } while (0)
> +
> +/* Record the two parts of 32-bit Thumb-2 instruction. Each part occupies
> +   one array element.  */
> +#define RECORD_THUMB2_MODE_INSN(INDEX, INSN1, INSN2) do \
> +{ \
> +  dsc->modinsn[INDEX] = INSN1;\
> +  dsc->modinsn[INDEX + 1] = INSN2;\
> +} while (0)

OK, so at this point I think it's really not necessary to have those
as macros in the first place.  Instead, code should just continue to
fill in dsc->modinsn, which will shorten this patch significantly :-)

> @@ -5117,10 +5119,21 @@ displaced_read_reg (struct regcache *regs, CORE_ADDR from, int regno)
>  
>    if (regno == 15)
>      {
> +      /* Compute pipeline offset:
> +	 - When executing an ARM instruction, PC reads as the address of the
> +	 current instruction plus 8.
> +	 - When executing a Thumb instruction, PC reads as the address of the
> +	 current instruction plus 4.  */
> +
> +      if (displaced_in_arm_mode (regs))

It would be somewhat nicer here to use dsc->is_thumb instead of re-computing
its value.  However, the displaced_read_reg function doesn't have the dsc
argument, which is annoying (and asymmetrical to displaced_write_reg ...).

So if you want to make the effort to change all call sites to pass in dsc,
this would be nice, but I guess I'm also OK with doing it as above.

> @@ -6904,23 +6919,49 @@ arm_displaced_init_closure (struct gdbarch *gdbarch, CORE_ADDR from,
>  			    CORE_ADDR to, struct displaced_step_closure *dsc)
>  {
>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> -  unsigned int i;
> +  unsigned int i, len, offset;
>    enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> +  int size = dsc->insn_size;

Ah, this is wrong: it needs to be "dsc->is_thumb? 2 : 4".  Note that if the
original instruction was 32-bit Thumb2, insn_size will be 4, but we still
need to copy 2-byte chunks here.

Otherwise, this looks OK to me now.  Thanks for your continued effort to work
on this feature!

Bye,
Ulrich

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