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

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

[Taylor Simpson]

hexagon-core.xml only exposes register p3_0 which is an alias that
aggregates the predicate registers.  It is more convenient for users
to interact directly with the predicate registers.

Tested with lldb downloaded from this location
https://github.com/llvm/llvm-project/releases/download/llvmorg-18.1.4/clang+llvm-18.1.4-x86_64-linux-gnu-ubuntu-18.04.tar.xz

BEFORE:
(lldb) reg read p3_0
    p3_0 = 0x00000000
(lldb) reg read p0
error: Invalid register name 'p0'.
(lldb) reg write p1 0xf
error: Register not found for 'p1'.

AFTER:
(lldb) reg read p3_0
    p3_0 = 0x00000000
(lldb) reg read p0
      p0 = 0x00
(lldb) reg read -s 1
Predicate Registers:
        p0 = 0x00
        p1 = 0x00
        p2 = 0x00
        p3 = 0x00

(lldb) reg write p1 0xf
(lldb) reg read p3_0
    p3_0 = 0x00000f00
(lldb) reg write p3_0 0xff00ff00
(lldb) reg read -s 1
Predicate Registers:
        p0 = 0x00
        p1 = 0xff
        p2 = 0x00
        p3 = 0xff

Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
---
 target/hexagon/gdbstub.c | 19 ++++++++++++++++++-
 gdb-xml/hexagon-core.xml |  6 +++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

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
@@ -1,5 +1,5 @@
 /*
- *  Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *  Copyright(c) 2019-2024 Qualcomm Innovation Center, Inc. All Rights Reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -36,6 +36,14 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         return gdb_get_regl(mem_buf, env->gpr[n]);
     }
 
+    n -= TOTAL_PER_THREAD_REGS;
+
+    if (n < NUM_PREGS) {
+        return gdb_get_reg8(mem_buf, env->pred[n]);
+    }
+
+    n -= NUM_PREGS;
+
     g_assert_not_reached();
 }
 
@@ -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);
+    }
+
+    n -= NUM_PREGS;
+
     g_assert_not_reached();
 }
 
diff --git a/gdb-xml/hexagon-core.xml b/gdb-xml/hexagon-core.xml
index e181163cff..b94378112a 100644
--- a/gdb-xml/hexagon-core.xml
+++ b/gdb-xml/hexagon-core.xml
@@ -1,6 +1,6 @@
 <?xml version="1.0"?>
 <!--
-  Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+  Copyright(c) 2023-2024 Qualcomm Innovation Center, Inc. All Rights Reserved.
 
   This work is licensed under the terms of the GNU GPL, version 2 or
   (at your option) any later version. See the COPYING file in the
@@ -80,5 +80,9 @@
   <reg name="c29"              bitsize="32" offset="244" encoding="uint" format="hex" group="Thread Registers" dwarf_regnum="61"/>
   <reg name="utimerlo"         bitsize="32" offset="248" encoding="uint" format="hex" group="Thread Registers" dwarf_regnum="62"/>
   <reg name="utimerhi"         bitsize="32" offset="252" encoding="uint" format="hex" group="Thread Registers" dwarf_regnum="63"/>
+  <reg name="p0"               bitsize="8"  offset="256" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="64"/>
+  <reg name="p1"               bitsize="8"  offset="257" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="65"/>
+  <reg name="p2"               bitsize="8"  offset="258" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="66"/>
+  <reg name="p3"               bitsize="8"  offset="259" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="67"/>
 
 </feature>
-- 
2.34.1

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

[Matheus Tavares Bernardino]

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?

> +    }
> +
> +    n -= NUM_PREGS;
> +
>      g_assert_not_reached();
>  }

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

[ltaylorsimpson]

> -----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?

Thanks,
Taylor

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

[Ted Woodward]

> -----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,
> Taylor
> 
> 

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

[ltaylorsimpson]

> -----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

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

[Brian Cain]

On 6/12/2024 11:42 AM, Taylor Simpson wrote:
> hexagon-core.xml only exposes register p3_0 which is an alias that
> aggregates the predicate registers.  It is more convenient for users
> to interact directly with the predicate registers.
>
> Tested with lldb downloaded from this location
> https://github.com/llvm/llvm-project/releases/download/llvmorg-18.1.4/clang+llvm-18.1.4-x86_64-linux-gnu-ubuntu-18.04.tar.xz
>
> BEFORE:
> (lldb) reg read p3_0
>      p3_0 = 0x00000000
> (lldb) reg read p0
> error: Invalid register name 'p0'.
> (lldb) reg write p1 0xf
> error: Register not found for 'p1'.
>
> AFTER:
> (lldb) reg read p3_0
>      p3_0 = 0x00000000
> (lldb) reg read p0
>        p0 = 0x00
> (lldb) reg read -s 1
> Predicate Registers:
>          p0 = 0x00
>          p1 = 0x00
>          p2 = 0x00
>          p3 = 0x00
>
> (lldb) reg write p1 0xf
> (lldb) reg read p3_0
>      p3_0 = 0x00000f00
> (lldb) reg write p3_0 0xff00ff00
> (lldb) reg read -s 1
> Predicate Registers:
>          p0 = 0x00
>          p1 = 0xff
>          p2 = 0x00
>          p3 = 0xff
>
> Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> ---


Queued.  Thanks!


>   target/hexagon/gdbstub.c | 19 ++++++++++++++++++-
>   gdb-xml/hexagon-core.xml |  6 +++++-
>   2 files changed, 23 insertions(+), 2 deletions(-)
>
> 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
> @@ -1,5 +1,5 @@
>   /*
> - *  Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved.
> + *  Copyright(c) 2019-2024 Qualcomm Innovation Center, Inc. All Rights Reserved.
>    *
>    *  This program is free software; you can redistribute it and/or modify
>    *  it under the terms of the GNU General Public License as published by
> @@ -36,6 +36,14 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>           return gdb_get_regl(mem_buf, env->gpr[n]);
>       }
>   
> +    n -= TOTAL_PER_THREAD_REGS;
> +
> +    if (n < NUM_PREGS) {
> +        return gdb_get_reg8(mem_buf, env->pred[n]);
> +    }
> +
> +    n -= NUM_PREGS;
> +
>       g_assert_not_reached();
>   }
>   
> @@ -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);
> +    }
> +
> +    n -= NUM_PREGS;
> +
>       g_assert_not_reached();
>   }
>   
> diff --git a/gdb-xml/hexagon-core.xml b/gdb-xml/hexagon-core.xml
> index e181163cff..b94378112a 100644
> --- a/gdb-xml/hexagon-core.xml
> +++ b/gdb-xml/hexagon-core.xml
> @@ -1,6 +1,6 @@
>   <?xml version="1.0"?>
>   <!--
> -  Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
> +  Copyright(c) 2023-2024 Qualcomm Innovation Center, Inc. All Rights Reserved.
>   
>     This work is licensed under the terms of the GNU GPL, version 2 or
>     (at your option) any later version. See the COPYING file in the
> @@ -80,5 +80,9 @@
>     <reg name="c29"              bitsize="32" offset="244" encoding="uint" format="hex" group="Thread Registers" dwarf_regnum="61"/>
>     <reg name="utimerlo"         bitsize="32" offset="248" encoding="uint" format="hex" group="Thread Registers" dwarf_regnum="62"/>
>     <reg name="utimerhi"         bitsize="32" offset="252" encoding="uint" format="hex" group="Thread Registers" dwarf_regnum="63"/>
> +  <reg name="p0"               bitsize="8"  offset="256" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="64"/>
> +  <reg name="p1"               bitsize="8"  offset="257" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="65"/>
> +  <reg name="p2"               bitsize="8"  offset="258" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="66"/>
> +  <reg name="p3"               bitsize="8"  offset="259" encoding="uint" format="hex" group="Predicate Registers" dwarf_regnum="67"/>
>   
>   </feature>