gdbstub: Do not use memset() on GByteArray

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

Posted by Philippe Mathieu-Daudé 5 years, 9 months ago

Introduce gdb_get_zeroes() to fill a GByteArray with zeroes.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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;
+}
+
 /**
  * gdb_get_reg_ptr: get pointer to start of last element
  * @len: length of element
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 8efc535f2a..063551df23 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -47,8 +47,7 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         if (gdb_has_xml) {
             return 0;
         }
-        memset(mem_buf, 0, 12);
-        return 12;
+        return gdb_get_zeroes(mem_buf, 12);
     }
     switch (n) {
     case 24:
diff --git a/target/xtensa/gdbstub.c b/target/xtensa/gdbstub.c
index 0ee3feabe5..4d43f1340a 100644
--- a/target/xtensa/gdbstub.c
+++ b/target/xtensa/gdbstub.c
@@ -105,8 +105,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         default:
             qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported size %d\n",
                           __func__, n, reg->size);
-            memset(mem_buf, 0, reg->size);
-            return reg->size;
+            return gdb_get_zeroes(mem_buf, reg->size);
         }
 
     case xtRegisterTypeWindow: /*a*/
@@ -115,8 +114,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     default:
         qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported type %d\n",
                       __func__, n, reg->type);
-        memset(mem_buf, 0, reg->size);
-        return reg->size;
+        return gdb_get_zeroes(mem_buf, reg->size);
     }
 }
 
-- 
2.21.1

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

Posted by Peter Maydell 5 years, 9 months ago

On Tue, 14 Apr 2020 at 11:24, Philippe Mathieu-Daudé <philmd@redhat.com> 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é <philmd@redhat.com>
> ---
>  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);

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

thanks
-- PMM

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

Posted by Philippe Mathieu-Daudé 5 years, 9 months ago

On 4/14/20 12:52 PM, Peter Maydell wrote:
> On Tue, 14 Apr 2020 at 11:24, Philippe Mathieu-Daudé <philmd@redhat.com> 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é <philmd@redhat.com>
>> ---
>>   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
>

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

Posted by Peter Maydell 5 years, 9 months ago

On Tue, 14 Apr 2020 at 12:05, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > 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.

AIUI ->len and ->data are not internals -- they're
in the documentation as "public fields" and the code
example for GByteArray directly accesses ->data.
(Contrast GBytes, where there are no defined public
fields and instead there are functions g_bytes_get_data
and g_bytes_get_size.)

thanks
-- PMM

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

Posted by Alex Bennée 5 years, 9 months ago

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 14 Apr 2020 at 12:05, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> > 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.
>
> AIUI ->len and ->data are not internals -- they're
> in the documentation as "public fields" and the code
> example for GByteArray directly accesses ->data.

They are - and I do it in a few places in the gdbstub when it's easier
than going long form. FWIW I prefer Peter's formulation.

> (Contrast GBytes, where there are no defined public
> fields and instead there are functions g_bytes_get_data
> and g_bytes_get_size.)
>
> thanks
> -- PMM


-- 
Alex Bennée

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

Posted by Alex Bennée 5 years, 9 months ago

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Introduce gdb_get_zeroes() to fill a GByteArray with zeroes.
>
> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Queued to for-5.0/more-random-fixes with Peter's alternative.

> ---
>  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;
> +}
> +
>  /**
>   * gdb_get_reg_ptr: get pointer to start of last element
>   * @len: length of element
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 8efc535f2a..063551df23 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -47,8 +47,7 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>          if (gdb_has_xml) {
>              return 0;
>          }
> -        memset(mem_buf, 0, 12);
> -        return 12;
> +        return gdb_get_zeroes(mem_buf, 12);
>      }
>      switch (n) {
>      case 24:
> diff --git a/target/xtensa/gdbstub.c b/target/xtensa/gdbstub.c
> index 0ee3feabe5..4d43f1340a 100644
> --- a/target/xtensa/gdbstub.c
> +++ b/target/xtensa/gdbstub.c
> @@ -105,8 +105,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>          default:
>              qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported size %d\n",
>                            __func__, n, reg->size);
> -            memset(mem_buf, 0, reg->size);
> -            return reg->size;
> +            return gdb_get_zeroes(mem_buf, reg->size);
>          }
>  
>      case xtRegisterTypeWindow: /*a*/
> @@ -115,8 +114,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>      default:
>          qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported type %d\n",
>                        __func__, n, reg->type);
> -        memset(mem_buf, 0, reg->size);
> -        return reg->size;
> +        return gdb_get_zeroes(mem_buf, reg->size);
>      }
>  }


-- 
Alex Bennée