[v2,36/39] gdbserver: send regs with stop reason

The gdb protocol allows sending register values of the stopped thread
with the stop-reason packet. This reduces round-trips because the client
doesn't have to get registers after stopping. This is a significant win
because getting registers requires multiple commands so it saves about
10 commands (round-trips).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 src/pdbgproxy.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

On Wed, 20 Apr 2022 at 06:51, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> The gdb protocol allows sending register values of the stopped thread
> with the stop-reason packet. This reduces round-trips because the client
> doesn't have to get registers after stopping. This is a significant win
> because getting registers requires multiple commands so it saves about
> 10 commands (round-trips).
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  src/pdbgproxy.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c
> index 6b9bdf90..d50121c0 100644
> --- a/src/pdbgproxy.c
> +++ b/src/pdbgproxy.c
> @@ -202,7 +202,11 @@ static void send_stop_for_thread(struct pdbg_target *target)
>         uint64_t pir = gdb_thread->pir;
>         char data[MAX_RESP_LEN];
>         size_t s = 0;
> +       struct thread_regs regs;
> +       uint64_t value;
> +       uint32_t value32;
>         int sig;
> +       int i;
>
>         if (gdb_thread->stop_attn)
>                 sig = 5; /* TRAP */
> @@ -213,6 +217,39 @@ static void send_stop_for_thread(struct pdbg_target *target)
>
>         s += snprintf(data + s, sizeof(data) - s, "T%02uthread:%04" PRIx64 ";%s", sig, pir, sig == 5 ? "swbreak:;" : "");

Ignore my comment about s in the earlier patch.

Reviewed-by: Joel Stanley <joel@jms.id.au>

>
> +       if (thread_getregs(target, &regs))
> +               PR_ERROR("Error reading gprs\n");

Forge ahead anyway? Makes sense I guess. Should we zero out 'regs' first?

> +
> +       for (i = 0; i < 32; i++)
> +               s += snprintf(data + s, sizeof(data) - s, "%x:%016" PRIx64 ";", i, be64toh(regs.gprs[i]));
> +
> +       thread_getnia(target, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%016" PRIx64 ";", 0x40, be64toh(value));
> +
> +       thread_getmsr(target, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%016" PRIx64 ";", 0x41, be64toh(value));
> +
> +       thread_getcr(target, &value32);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%08" PRIx32 ";", 0x42, be32toh(value32));
> +
> +       thread_getspr(target, SPR_LR, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%016" PRIx64 ";", 0x43, be64toh(value));
> +
> +       thread_getspr(target, SPR_CTR, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%016" PRIx64 ";", 0x44, be64toh(value));
> +
> +       thread_getspr(target, SPR_XER, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%08" PRIx32 ";", 0x45, be32toh(value));
> +
> +       thread_getspr(target, SPR_FPSCR, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%08" PRIx32 ";", 0x46, be32toh(value));
> +
> +       thread_getspr(target, SPR_VSCR, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%08" PRIx32 ";", 0x67, be32toh(value));
> +
> +       thread_getspr(target, SPR_VRSAVE, &value);
> +       s += snprintf(data + s, sizeof(data) - s, "%x:%08" PRIx32 ";", 0x68, be32toh(value));
> +
>         send_response(fd, data);
>  }
>
> --
> 2.35.1
>
> --
> Pdbg mailing list
> Pdbg@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/pdbg

Excerpts from Joel Stanley's message of May 3, 2022 5:42 pm:
> On Wed, 20 Apr 2022 at 06:51, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> The gdb protocol allows sending register values of the stopped thread
>> with the stop-reason packet. This reduces round-trips because the client
>> doesn't have to get registers after stopping. This is a significant win
>> because getting registers requires multiple commands so it saves about
>> 10 commands (round-trips).
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  src/pdbgproxy.c | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c
>> index 6b9bdf90..d50121c0 100644
>> --- a/src/pdbgproxy.c
>> +++ b/src/pdbgproxy.c
>> @@ -202,7 +202,11 @@ static void send_stop_for_thread(struct pdbg_target *target)
>>         uint64_t pir = gdb_thread->pir;
>>         char data[MAX_RESP_LEN];
>>         size_t s = 0;
>> +       struct thread_regs regs;
>> +       uint64_t value;
>> +       uint32_t value32;
>>         int sig;
>> +       int i;
>>
>>         if (gdb_thread->stop_attn)
>>                 sig = 5; /* TRAP */
>> @@ -213,6 +217,39 @@ static void send_stop_for_thread(struct pdbg_target *target)
>>
>>         s += snprintf(data + s, sizeof(data) - s, "T%02uthread:%04" PRIx64 ";%s", sig, pir, sig == 5 ? "swbreak:;" : "");
> 
> Ignore my comment about s in the earlier patch.

Oh yeah I see why you made the comment when this was the only line.
Probably should have written it properly for one line then changed
it here, but oh well.

> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
>>
>> +       if (thread_getregs(target, &regs))
>> +               PR_ERROR("Error reading gprs\n");
> 
> Forge ahead anyway? Makes sense I guess. Should we zero out 'regs' first?

Humph, yeah lot of error handling in this code isn't good. Most of the 
time if you get an error anywhere it's because the host has crashed
terribly.

We should try to be a bit consistent with it though.

Thanks,
Nick