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

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

Yao Qi wrote:

> Current implementation of displaced stepping in ARM assumes instruction
> size is fixed 32-bit.  Patch 2 is to rewrite some infrastructure code to
> be ready to handle non-32-bit instructions.  This patch doesn't change
> any GDB functionality either.

Thanks for working on this!

> 	* gdb/arm-tdep.h (struct displaced_step_closure): Add new field
> 	modinsns.
> 	Remove field modinsn.
> 	(RECORD_MOD_32BIT_INSN): New macro.
> 	(RECORD_MOD_16BIT_INSN): New macro.
> 	(RECORD_MOD_INSN): New macro.

> -  unsigned long modinsn[DISPLACED_MODIFIED_INSNS];
> +
> +  struct insn
> +  {
> +    union
> +    {
> +      unsigned long a;
> +      unsigned short t;
> +    }insn;
> +    unsigned short size;
> +  }modinsns[DISPLACED_MODIFIED_INSNS];
> +

I don't think this is the right way to go.  You cannot have a mixture of
ARM and Thumb instructions in a single modinsn block, and if you have
Thumb instructions, they all need to be transfered in 16-bit chunks,
even the 32-bit Thumb2 instructions, to get the endian conversion right.

So I think you should rather keep a single modinsn array of unsigned long.
When filling it in, ARM instructions are handled as today, 16-bit Thumb
instructions are likewise just filled into one modinsn slot, and 32-bit
Thumb instructions are filled into two modinsn slots.

When copying the modinsn array out to the target, each slot is transfered
as 4 bytes in ARM mode, and as 2 bytes in Thumb mode.  To know in which
mode you are, it is probably best to have a single flag in the struct
displaced_step_closure that indicated whether it is ARM or Thumb; this
flag would be set once at the start of arm_process_displaced_insn, and
used throughout the code whereever we need to know the mode.

This approach would make most of the changes in this patch obsolete.

> 	(cleanup_branch): Replace magic number by macros.

> -      ULONGEST pc = displaced_read_reg (regs, from, 15);
> -      displaced_write_reg (regs, dsc, 14, pc - 4, CANNOT_WRITE_PC);
> +      ULONGEST pc = displaced_read_reg (regs, from, ARM_PC_REGNUM);
> +      displaced_write_reg (regs, dsc, ARM_LR_REGNUM, pc - 4, CANNOT_WRITE_PC);

I'm not sure about this change -- other callers just pass in plain
register numbers as well ...  Either those should all be changed,
or none of them.  In any case, this is really an unrelated change,
and should be done -if at all- in a separate patch.

> @@ -5908,23 +5928,44 @@ 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);
>  
> +  offset = 0;
>    /* Poke modified instruction(s).  */
>    for (i = 0; i < dsc->numinsns; i++)
>      {
> +      unsigned long insn;
> +      if (dsc->modinsns[i].size == 4)
> +	insn = dsc->modinsns[i].insn.a;
> +      else if (dsc->modinsns[i].size == 2)
> +	insn = dsc->modinsns[i].insn.t;
> +      else
> +	gdb_assert (0);
> +
>        if (debug_displaced)
> -	fprintf_unfiltered (gdb_stdlog, "displaced: writing insn %.8lx at "
> -			    "%.8lx\n", (unsigned long) dsc->modinsn[i],
> -			    (unsigned long) to + i * 4);
> -      write_memory_unsigned_integer (to + i * 4, 4, byte_order_for_code,
> -				     dsc->modinsn[i]);
> +	{
> +	  fprintf_unfiltered (gdb_stdlog, "displaced: writing insn ");
> +	  if (dsc->modinsns[i].size == 4)
> +	    fprintf_unfiltered (gdb_stdlog, "%.8lx",
> +				dsc->modinsns[i].insn.a);
> +	  else if (dsc->modinsns[i].size == 2)
> +	    fprintf_unfiltered (gdb_stdlog, "%.4x",
> +				dsc->modinsns[i].insn.t);
> +
> +	  fprintf_unfiltered (gdb_stdlog, " at %.8lx\n",
> +			      (unsigned long) to + offset);
> +	}
> +      write_memory_unsigned_integer (to + offset, dsc->modinsns[i].size,
> +				     byte_order_for_code,
> +				     insn);
> +      offset += dsc->modinsns[i].size;
>      }

As indicated above, this should just copy dsc->numinsns chunks of size 4
in ARM mode, and of size 2 in Thumb mode.

>    /* Put breakpoint afterwards.  */
> -  write_memory (to + dsc->numinsns * 4, tdep->arm_breakpoint,
> -		tdep->arm_breakpoint_size);
> +  write_memory (to + arm_displaced_step_breakpoint_offset (dsc),
> +		arm_breakpoint_from_pc (gdbarch, &from, &len),
> +		len);

Calling arm_breakpoint_from_pc is not a good idea, since this calls
arm_pc_is_thumb, which may end up getting a wrong result.  Since we
already know whether we're in ARM or Thumb mode, you should just
emit either tdep->arm_breakpoint or tdep->thumb_breakpoint.  (Since
we're not *replacing* any instruction here, there is never a need
to use the Thumb-2 breakpoint.)

> @@ -5960,7 +6001,11 @@ arm_displaced_step_fixup (struct gdbarch *gdbarch,
>      dsc->cleanup (gdbarch, regs, dsc);
>  
>    if (!dsc->wrote_to_pc)
> -    regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM, dsc->insn_addr + 4);
> +    {
> +      struct frame_info *fi = get_current_frame ();
> +      regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM,
> +				      arm_get_next_pc_raw(fi, dsc->insn_addr, 0));
> +    }

Hmm, arm_get_next_pc_raw tries to follow branches etc, which is probably
not what we want here.  Again, I'd rather just check ARM vs. Thumb state
(in Thumb mode we could then check the instruction to see whether it is
a 16-bit or 32-bit instruction --- or even better, the original decoding
step could have just set a flag in dsc).

Bye,
Ulrich

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