* [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray
@ 2020-04-14 10:24 Philippe Mathieu-Daudé
2020-04-14 10:52 ` Peter Maydell
2020-04-14 16:09 ` Alex Bennée
0 siblings, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-14 10:24 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
qemu-arm, Alex Bennée, Max Filippov
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray
2020-04-14 10:24 [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray Philippe Mathieu-Daudé
@ 2020-04-14 10:52 ` Peter Maydell
2020-04-14 11:05 ` Philippe Mathieu-Daudé
2020-04-14 16:09 ` Alex Bennée
1 sibling, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2020-04-14 10:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Max Filippov, qemu-arm, Alex Bennée, QEMU Developers
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray
2020-04-14 10:52 ` Peter Maydell
@ 2020-04-14 11:05 ` Philippe Mathieu-Daudé
2020-04-14 11:49 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-14 11:05 UTC (permalink / raw)
To: Peter Maydell; +Cc: Max Filippov, qemu-arm, Alex Bennée, QEMU Developers
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray
2020-04-14 11:05 ` Philippe Mathieu-Daudé
@ 2020-04-14 11:49 ` Peter Maydell
2020-04-14 14:22 ` Alex Bennée
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2020-04-14 11:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Max Filippov, qemu-arm, Alex Bennée, QEMU Developers
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray
2020-04-14 11:49 ` Peter Maydell
@ 2020-04-14 14:22 ` Alex Bennée
0 siblings, 0 replies; 6+ messages in thread
From: Alex Bennée @ 2020-04-14 14:22 UTC (permalink / raw)
To: Peter Maydell
Cc: Max Filippov, qemu-arm, Philippe Mathieu-Daudé, QEMU Developers
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray
2020-04-14 10:24 [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray Philippe Mathieu-Daudé
2020-04-14 10:52 ` Peter Maydell
@ 2020-04-14 16:09 ` Alex Bennée
1 sibling, 0 replies; 6+ messages in thread
From: Alex Bennée @ 2020-04-14 16:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, qemu-arm, qemu-devel, Max Filippov
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-14 16:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-14 10:24 [PATCH-for-5.0] gdbstub: Do not use memset() on GByteArray Philippe Mathieu-Daudé
2020-04-14 10:52 ` Peter Maydell
2020-04-14 11:05 ` Philippe Mathieu-Daudé
2020-04-14 11:49 ` Peter Maydell
2020-04-14 14:22 ` Alex Bennée
2020-04-14 16:09 ` Alex Bennée
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.