Re: [PATCH,powerpc,v2] gdb-power10-single-step

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

On Fri, Mar 26, 2021 at 07:25:15PM -0500, will schmidt wrote:

>     gdb/ChangeLog:
> 	* gdb/rs6000-tdep;c:  Add support for single-stepping of prefixed instructions.
> 
>     gdb/testsuite/ChangeLog:
> 	* gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.s:  Testcase 
> 	
>   using non-relative plxv instructions.
> 	* gdb/testsuite/gdb.arch/powerpc-plxv-nonrel.exp: Testcase 
> 	
>   harness.

Again omit the leading "gdb/" or "gdb/testsuite/".  Names should always be
relative to the location of the ChangeLog file that contains the entry.

> @@ -897,30 +902,66 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
>    size_t len = gdbarch_max_insn_length (gdbarch);
>    std::unique_ptr<ppc_displaced_step_copy_insn_closure> closure
>      (new ppc_displaced_step_copy_insn_closure (len));
>    gdb_byte *buf = closure->buf.data ();
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -  int insn;
> +  ULONGEST insn;
>  
> -  read_memory (from, buf, len);
> +  len = target_read (current_inferior()->top_target(), TARGET_OBJECT_MEMORY, NULL,
> +			buf, from, len);
> +  if ((ssize_t) len < PPC_INSN_SIZE)
> +		memory_error (TARGET_XFER_E_IO, from);
>  
>    insn = extract_signed_integer (buf, PPC_INSN_SIZE, byte_order);
> +  displaced_debug_printf ("attempting displacement of instruction as: %16lx, length %ld, ",
> +			  insn, len);

Again, there's too many debug statements, they will flood the log and make
it not really usable.  We don't need to print every single instruction here.

> +  /* If the instruction is prefixed, handle the instruction size properly.  */
> +  if ((insn & OP_MASK) == 1 << 26)
> +    {
> +      /* Handle PNOP.  */
> +      /* Handle Prefixed instructions with R=0.  */
> +      if (((insn & PNOP_MASK) == PNOP_INSN) ||
> +          ((insn & R_MASK) == R_ZERO))
> +	 {
> +	   insn = extract_signed_integer (buf, 2 * PPC_INSN_SIZE, byte_order);
> +	   displaced_debug_printf ("reloaded instruction: %16lx, length %ld",
> +				   insn, len);

Likewise we don't need to display each instruction here.  This whole block
is probably unnecessary.

> +	 }
> +      else
> +	{
> +	/* Prefixed with R=1 */
> +	/* The prefixed instructions with R=1 will read/write data to/from
> +	   locations relative to the current PC.  We would not be able to
> +	   fixup after an instruction has written to a displaced location, so
> +	   decline to displace those instructions.  */
> +	  displaced_debug_printf ("Not displacing prefixed instruction %16lx at %s",
> +	  			  insn, paddress (gdbarch, from));

This statement is actually useful and should be kept.

> +			return NULL;
> +	}
> +    } /* prefixed.  */
> +  else
> +    /* non-prefixed.  */
> +    {
> +      /* Set the instruction length to 4 to match the actual instruction
> +	 length.  */
> +      len = 4;
> +    }

The formatting is odd throughout -- is this just an issue with the email?
Please verify it complies with the GNU coding style.

>    /* Assume all atomic sequences start with a Load and Reserve instruction.  */
>    if (IS_LOAD_AND_RESERVE_INSN (insn))
>      {
>        displaced_debug_printf ("can't displaced step atomic sequence at %s",
>  			      paddress (gdbarch, from));
> -
>        return NULL;
>      }
>  
>    write_memory (to, buf, len);
>  
>    displaced_debug_printf ("copy %s->%s: %s",
> -			  paddress (gdbarch, from), paddress (gdbarch, to),
> -			  displaced_step_dump_bytes (buf, len).c_str ());;
> +			paddress (gdbarch, from), paddress (gdbarch, to),
> +			displaced_step_dump_bytes (buf, len).c_str ());

Please omit those whitespace changes.

>    /* Check for breakpoints in the inferior.  If we've found one, place the PC
>       right at the breakpoint instruction.  */
>    else if ((insn & BP_MASK) == BP_INSN)
> -    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
> +    {
> +      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
> +    }
>    else
> +    {
>    /* Handle any other instructions that do not fit in the categories above.  */
> +  /* set offset to 8 if this is an 8-byte (prefixed) instruction. */
> +    if ((insn & OP_MASK) == 1 << 26)
> +	offset = 2 * PPC_INSN_SIZE;
> +    else
> +	offset = PPC_INSN_SIZE;
>      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
>  				    from + offset);
> +    }

Check the formatting again.   Also, I think it would be better to move
calculating the correct offset to the top of the function, just after
insn is read initially.


>    for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
>      {
> -      loc += PPC_INSN_SIZE;
> -      insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +	int cur_insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +	if ((cur_insn & OP_MASK) == 1 << 26) {
> +	  loc += 2*PPC_INSN_SIZE;
> +	  insn = read_memory_integer (loc, 2 *  PPC_INSN_SIZE, byte_order);

This seems wrong to me: subsequent code expects "insn" to be a 4-byte value
(e.g. for the if ((insn & OP_MASK) == BC_INSN) check).

Why do you have the read the full 8 byte instruction in the first place?
Nothing depends on the second half.

> +	} else {
> +	  loc += PPC_INSN_SIZE;
> +	  insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> +	}

Position of the '{' is definitely wrong.  Also check the indentation.

> +    fprintf_unfiltered(gdb_stdlog,"Warning: read_memory_integer: instruction found here. %08x \n",insn);

This seems left-over from debugging?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com