Re: [PATCH] gdb-power10-single-step

Re: [PATCH] gdb-power10-single-step

[Ulrich Weigand]

On Thu, Mar 25, 2021 at 12:21:42PM -0500, will schmidt wrote:
> On Wed, 2021-03-10 at 18:50 +0100, Ulrich Weigand wrote:
> > Will Schmidt wrote:
> > 
> > >   This is a patch written by Alan Modra.  With his permission
> > > I'm submitting this for review and helping get this upstream.
> > > 
> > > Powerpc / Power10 ISA 3.1 adds prefixed instructions, which
> > > are 8 bytes in length.  This is in contrast to powerpc previously
> > > always having 4 byte instruction length.  This patch implements
> > > changes to allow GDB to better detect prefixed instructions, and
> > > handle single stepping across the 8 byte instructions.
> > 
> > There's a few issues I see here:
> 
> I've dug in a bit more,.. have a few questions related to the patch and
> the comments here.  I've got a refreshed version of this patch in my
> queue, with a nit or two that i'm  still trying to understand and
> squash before I post it.
> 
> > 
> > - The patch now *forces* software single-stepping for all 8-byte
> >   instructions.  I'm not sure why this is necessary; I thought
> >   that hardware single-stepping was supported for 8-byte instructions
> >   as well?  That would certainly be preferable.
> 
> 
> Does software single-stepping go hand-in-hand with executing the
> instructions from a displaced location?

Yes.  Hardware single-step executes the instruction where it is.
Software single-step needs to replace the subsequent instruction
with a breakpoint, and in order to be able to do that without
unduly affecting simultaneous execution of that code in other
threads, this is not done in place, but in a copy in a displaced
location.

> I only see one clear return-if-prefixed change in the patch, so I am
> assuming the above refers to the patch chunk seen as :
> > @@ -1081,6 +1090,10 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
> >    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
> >    int bc_insn_count = 0; /* Conditional branch instruction count.  */
> > 
> > +  /* Power10 prefix instructions are two words in length.  */
> > +  if ((insn & OP_MASK) == 1 << 26)
> > +    return { pc + 8 };

Yes, this is what I was refering to.  By returning a PC value here,
common code is instructed to always perform a software single-step.
This should not be necessary.

> I've got a local change to eliminate that return.   Per the poking I've
> done so far, none of the prefix instructions I've run against so far
> allow us past the is_load_and_reserve instruction check. 
> >   if (!IS_LOAD_AND_RESERVE_INSN (insn))
> >     return {};
> 
> statement, so not a significant code flow behavior change.

Yes, if you just remove the three lines above, code will fall
through to here and return an empty sequence, causing the
common code to use hardware single-step.

> > - However, the inner loop of ppc_deal_with_atomic_sequence should
> >   probably be updated to correctly skip 8-byte instructions; e.g.
> >   to avoid mistakenly recognizing the second word of an 8-byte
> >   instructions for a branch or store conditional.  (Also, the
> >   count of up to "16 instructions" is wrong if 8-byte instructions
> >   are not handled specifically.)
> 
> I've got a local change to inspect the instruction and determine if it
> is prefixed, so I think i've got this handled.  I'm generally assuming
> we will never start halfway through an 8-byte prefixed instruction.

Yes, you can assume the incoming PC value is a valid PC at the start
of the current instruction.

Bye,
Ulrich

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