Re: [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray

[Philippe Mathieu-Daudé]

On 4/14/20 12:52 PM, Peter Maydell wrote:
> On Tue, 14 Apr 2020 at 11:24, Philippe Mathieu-Daudé <address@hidden> wrote:
> > Introduce gdb_get_zeroes() to fill a GByteArray with zeroes.
> >
> > Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > ---
> >   include/exec/gdbstub.h  | 9 +++++++++
> >   target/arm/gdbstub.c    | 3 +--
> >   target/xtensa/gdbstub.c | 6 ++----
> >   3 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> > index 30b909ebd2..b52d9933ee 100644
> > --- a/include/exec/gdbstub.h
> > +++ b/include/exec/gdbstub.h
> > @@ -125,6 +125,15 @@ static inline int gdb_get_reg128(GByteArray *buf, uint64_t 
> > val_hi,
> >       return 16;
> >   }
> >
> > +static inline int gdb_get_zeroes(GByteArray *array, size_t len)
> > +{
> > +    for (size_t i = 0; i < len; i++) {
> > +        gdb_get_reg8(array, '\0');
> > +    }
> > +
> > +    return len;
> > +}
>
> The other implementation option here would be
>
>       guint oldlen = array->len;
>       g_byte_array_set_size(array, oldlen + len);
>       memset(array->data + oldlen, 0, len);

I thought about it but I'd rather not access GByteArray internals.

> For length values < 16 my guess is the perf difference is
> not going to be noticeable though.

On ARM it is called with size=12:

target/arm/gdbstub.c:50:        memset(mem_buf, 0, 12);

On Xtensa it is only used with unimplemented registers:

        qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported type %d\n",
                      __func__, n, reg->type);

If you prefer I can use a static empty buffer of 16 bytes and copy from it:

static inline int gdb_get_zeroes(GByteArray *array, size_t len)
{
    static const uint8_t zeroed_buf[16] = { };

    g_byte_array_append(array, zeroed_buf, MIN(len, sizeof(zeroed_buf)));
    for (size_t i = sizeof(zeroed_buf); i < len; i++) {
        gdb_get_reg8(array, '\0');
    }

    return len;
}

> thanks
> -- PMM