[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gdb-patches
Subject:    Re: [PATCH,rs6000,v3] gdb-power10-single-step
From:       Ulrich Weigand via Gdb-patches <gdb-patches () sourceware ! org>
Date:       2021-03-30 19:47:09
Message-ID: 20210330194709.GB2343 () oc3748833570 ! ibm ! com
[Download RAW message or body]

On Mon, Mar 29, 2021 at 05:43:53PM -0500, will schmidt wrote:

> YYYY-MM-DD  Will Schmidt  <will_schmidt@vnet.ibm.com>
> 
> 	gdb/ChangeLog:
> 	* rs6000-tdep.c:  Add support for single-stepping of
> 	prefixed instructions.
> 
> 	gdb/testsuite/ChangeLog:
> 	* gdb.arch/powerpc-plxv-nonrel.s:  Testcase using
> 	non-relative plxv instructions.
> 	* /gdb.arch/powerpc-plxv-nonrel.exp: Testcase harness.

Stray '/' at the beginning.

> +  if ((ssize_t) len < PPC_INSN_SIZE)
> +		memory_error (TARGET_XFER_E_IO, from);

Incorrect indentation.

> +  /* Check for PNOP and for prefixed instructions with R=0.  Those
> +     instructions are safe to displace.  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 data
> +    into a displaced location, so decline to displace those instructions.  */
> +  if ((insn & OP_MASK) == 1 << 26)
> +    {
> +      if (((insn & PNOP_MASK) != PNOP_INSN) &&
> +          ((insn & R_MASK) != R_ZERO))

GDB coding style is to have the && at the start of the second line.

> +	{
> +	  displaced_debug_printf ("Not displacing prefixed instruction %08x at %s",
> +						insn, paddress (gdbarch, from));

Incorrect indentation.

> +  /* set offset to 8 if this is an 8-byte (prefixed) instruction. */

Start with a capital 'S' and end with two spaces after the '.'.

> +    if ((opcode) == 1 << 26)
> +	offset = 2 * PPC_INSN_SIZE;
> +    else
> +	offset = PPC_INSN_SIZE;

Wrong indentation.

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

Hmm.  This reads all instruction twice now, which we should avoid for
performance reasons.  But it is not actually necessary: at the start
of this loop, "insn" is always the instruction at "loc", read either
by the code before the loop or the previous iteration of the loop.
So you can just use "insn" for the prefix check.

(Also, indentation seems wrong :-))


Except for this one issue with reading the instructions twice, this
looks now ready to commit (once the coding style issues are fixed).

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com
[prev in list] [next in list] [prev in thread] [next in thread]