RE: [PATCH] Hexagon: lldb read/write predicate registers p0/p1/p2/p3

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Ted Woodward <tedwood@quicinc.com>
> Sent: Thursday, June 13, 2024 9:03 AM
> To: ltaylorsimpson@gmail.com; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>
> Cc: qemu-devel@nongnu.org; Brian Cain <bcain@quicinc.com>;
> alex.bennee@linaro.org; Sid Manning <sidneym@quicinc.com>; Marco
> Liebel (QUIC) <quic_mliebel@quicinc.com>; richard.henderson@linaro.org;
> philmd@linaro.org; ale@rev.ng; anjo@rev.ng
> Subject: RE: [PATCH] Hexagon: lldb read/write predicate registers
> p0/p1/p2/p3
> 
> 
> 
> > -----Original Message-----
> > From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> > Sent: Wednesday, June 12, 2024 9:49 PM
> > To: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>
> > Cc: qemu-devel@nongnu.org; Brian Cain <bcain@quicinc.com>; Ted
> > Woodward <tedwood@quicinc.com>; alex.bennee@linaro.org; Sid
> Manning
> > <sidneym@quicinc.com>; Marco Liebel (QUIC)
> <quic_mliebel@quicinc.com>;
> > richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng;
> > anjo@rev.ng
> > Subject: RE: [PATCH] Hexagon: lldb read/write predicate registers
> > p0/p1/p2/p3
> >
> > > -----Original Message-----
> > > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> > > Sent: Wednesday, June 12, 2024 12:30 PM
> > > To: ltaylorsimpson@gmail.com
> > > Cc: qemu-devel@nongnu.org; bcain@quicinc.com;
> tedwood@quicinc.com;
> > > alex.bennee@linaro.org; quic_mathbern@quicinc.com;
> > > sidneym@quicinc.com; quic_mliebel@quicinc.com;
> > > richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng;
> > > anjo@rev.ng
> > > Subject: Re: [PATCH] Hexagon: lldb read/write predicate registers
> > > p0/p1/p2/p3
> > >
> > > On Wed, 12 Jun 2024 10:42:39 -0600 Taylor Simpson
> > > <ltaylorsimpson@gmail.com> wrote:
> > > >
> > > > diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
> > > > index
> > > > 502c6987f0..e67e627fc9 100644
> > > > --- a/target/hexagon/gdbstub.c
> > > > +++ b/target/hexagon/gdbstub.c
> > > > @@ -56,6 +64,15 @@ int hexagon_gdb_write_register(CPUState *cs,
> > > uint8_t *mem_buf, int n)
> > > >          return sizeof(target_ulong);
> > > >      }
> > > >
> > > > +    n -= TOTAL_PER_THREAD_REGS;
> > > > +
> > > > +    if (n < NUM_PREGS) {
> > > > +        env->pred[n] = ldtul_p(mem_buf);
> > > > +        return sizeof(uint8_t);
> > >
> > > I wonder, shouldn't this be sizeof(target_ulong) since we wrote a
> > > target_ulong?
> >
> > Good question.
> >
> > From the architecture point of view, predicates are 8 bits (Section
> > 2.2.5 of the
> > v73 Hexagon PRM).  However, we model them in QEMU as target_ulong
> > because TCG variables must be either 32 bits or 64 bits.  There isn't
> > an option for 8 bits.  Whenever we write to a predicate, do "and" with
> > 0xff first to ensure there are only 8 bits written (see
> > gen_log_pred_write in target/hexagon/genptr.c).
> >
> > I did some more digging and here is what I found:
> > - Since we have bitsize="8" in hexagon-core.xml, lldb will reject any
> > attempt to write something larger.
> >   (lldb) reg write p1 0x1ff
> >   error: Failed to write register 'p1' with value '0x1ff': value 0x1ff
> > is too large to fit in a 1 byte unsigned integer value
> > - For the lldb "reg write" command, the return value from
> > hexagon_gdb_write_register isn't used.
> > - The only place the return value is used is in handle_write_all_regs.
> > This function is called in response to a "G" packet from the debugger.
> > I don't know if/when lldb uses this packet, but it seems like it would
> > count on it being 8 bits since that's what is in hexagon-core.xml.
> >
> > Ted <tedwood@quicinc.com>, when would lldb generate a "G" packet, and
> > what assumptions will it make about the size of predicate registers?
> 
> When you use the expression parser to call a function, lldb will save the
> current state, set up the function call, set a breakpoint on a return (by
> changing the lr register and setting a breakpoint on the new address), set
the
> PC to the function address, and resume. After the breakpoint is hit, lldb
will
> restore the saved state.
> 
> Since QEMU doesn't support the lldb RSP extension
> QSaveRegisterState/QRestoreRegisterState,
> lldb will use G/g packets to save and restore the register state.
> 
> lldb doesn't interpret the values from the G/g packets. It just saves and
> restores them, so I don't think the new predicate definitions will matter
for
> that. You can test this out by changing the predicate registers, then
calling a
> function with the expression parser. Not a varargs function, since the IR
> interpreter doesn't handle those.
> 
> Ted

Thanks Ted!  We do indeed execute handle_write_all_regs when we print the
result of a function call in lldb.

So, the answer to Metheus' question is "no".  We should return sizeof
uint8_t.  However, we should also mask off the high bits from the value
returned from ldtul_p before assigning to the predicate register.  This
avoids putting bits from subsequent items in the buffer into the register.
    env->pred[n] = ldtul_p(mem_buf) & 0xff;

I'll send v2 of the patch with this change shortly.

Taylor