[Qemu-devel] [PATCH for 4.1 0/5] tcg tests and gdbstub fixes

[Qemu-devel] [PATCH for 4.1 0/5] tcg tests and gdbstub fixes

[Alex Bennée]

Hi,

A small selection of clean-up patches covering failing tests and
gdbstub.

The following patches need review
 patch 0002/tests tcg fix up test i386 fprem.ref generation.patch
 patch 0003/tests tcg fix diff out pass to properly report fa.patch
 patch 0004/gdbstub add some notes to the header comment.patch

Alex Bennée (4):
  tests/tcg: fix up test-i386-fprem.ref generation
  tests/tcg: fix diff-out pass to properly report failure
  gdbstub: add some notes to the header comment
  gdbstub: revert to previous set_reg behaviour

Philippe Mathieu-Daudé (1):
  target/arm/vfp_helper: Call set_fpscr_to_host before updating FPSCR
    reg

 gdbstub.c                      | 24 ++++++++++++++++++------
 target/arm/vfp_helper.c        |  4 ++--
 tests/tcg/Makefile             |  6 +++++-
 tests/tcg/i386/Makefile.target |  4 ++--
 4 files changed, 27 insertions(+), 11 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v1 1/5] target/arm/vfp_helper: Call set_fpscr_to_host before updating FPSCR reg

[Alex Bennée]

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

In commit e9d652824b0 we extracted the vfp_set_fpscr_to_host()
function but failed at calling it in the correct place, we call
it after xregs[ARM_VFP_FPSCR] is modified.

Fix by calling this function before we update FPSCR.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Tested-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Message-Id: <20190705124318.1075-1-philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/vfp_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/vfp_helper.c b/target/arm/vfp_helper.c
index 46041e3294..9710ef1c3e 100644
--- a/target/arm/vfp_helper.c
+++ b/target/arm/vfp_helper.c
@@ -197,6 +197,8 @@ void HELPER(vfp_set_fpscr)(CPUARMState *env, uint32_t val)
         val &= 0xf7c0009f;
     }
 
+    vfp_set_fpscr_to_host(env, val);
+
     /*
      * We don't implement trapped exception handling, so the
      * trap enable bits, IDE|IXE|UFE|OFE|DZE|IOE are all RAZ/WI (not RES0!)
@@ -217,8 +219,6 @@ void HELPER(vfp_set_fpscr)(CPUARMState *env, uint32_t val)
     env->vfp.qc[1] = 0;
     env->vfp.qc[2] = 0;
     env->vfp.qc[3] = 0;
-
-    vfp_set_fpscr_to_host(env, val);
 }
 
 void vfp_set_fpscr(CPUARMState *env, uint32_t val)
-- 
2.20.1

[Qemu-devel] [PATCH v1 2/5] tests/tcg: fix up test-i386-fprem.ref generation

[Alex Bennée]

We never shipped the reference data in the source tree because it was
quite big (64M). As a result the only option is to generate it
locally. Although we have a rule to generate the reference file we
missed the dependency and location changes, probably because it is
only run for SLOW test runs.

The test still fails with mostly incorrect flags and different than
expected NaNs. I'll leave that for the x86 experts to look at.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Jan Bobek <jan.bobek@gmail.com>
---
 tests/tcg/i386/Makefile.target | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/tcg/i386/Makefile.target b/tests/tcg/i386/Makefile.target
index b4033ba3d1..d0eb7023e5 100644
--- a/tests/tcg/i386/Makefile.target
+++ b/tests/tcg/i386/Makefile.target
@@ -35,9 +35,9 @@ test-i386-fprem.ref: test-i386-fprem
 	$(call quiet-command, ./$< > $@,"GENREF","generating $@")
 
 run-test-i386-fprem: TIMEOUT=60
-run-test-i386-fprem: test-i386-fprem
+run-test-i386-fprem: test-i386-fprem test-i386-fprem.ref
 	$(call run-test,test-i386-fprem, $(QEMU) $<,"$< on $(TARGET_NAME)")
-	$(call diff-out,test-i386-fprem, $(I386_SRC)/$<.ref)
+	$(call diff-out,test-i386-fprem, test-i386-fprem.ref)
 else
 run-test-i386-fprem: test-i386-fprem
 	$(call skip-test, $<, "SLOW")
-- 
2.20.1

Re: [Qemu-devel] [PATCH v1 2/5] tests/tcg: fix up test-i386-fprem.ref generation

[Richard Henderson]

On 7/5/19 6:04 PM, Alex Bennée wrote:
> We never shipped the reference data in the source tree because it was
> quite big (64M). As a result the only option is to generate it
> locally. Although we have a rule to generate the reference file we
> missed the dependency and location changes, probably because it is
> only run for SLOW test runs.
> 
> The test still fails with mostly incorrect flags and different than
> expected NaNs. I'll leave that for the x86 experts to look at.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Jan Bobek <jan.bobek@gmail.com>
> ---
>  tests/tcg/i386/Makefile.target | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

There's nothing here to guess that test-i386-fprem is in any way natively
runnable.  Otherwise you might just end up testing against the installed qemu.

On the other hand, at the moment this simply won't run at all, since the input
file is missing.  So I suppose this is good enough for SLOW.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v1 2/5] tests/tcg: fix up test-i386-fprem.ref generation

[Philippe Mathieu-Daudé]

On 7/5/19 6:04 PM, Alex Bennée wrote:
> We never shipped the reference data in the source tree because it was
> quite big (64M). As a result the only option is to generate it

Can we fetch it (with hash verification) or store it compressed?

$ du -ch pc-bios/edk2-*bz2
1.2M    pc-bios/edk2-aarch64-code.fd.bz2
1.2M    pc-bios/edk2-arm-code.fd.bz2
4.0K    pc-bios/edk2-arm-vars.fd.bz2
1.7M    pc-bios/edk2-i386-code.fd.bz2
1.9M    pc-bios/edk2-i386-secure-code.fd.bz2
4.0K    pc-bios/edk2-i386-vars.fd.bz2
1.7M    pc-bios/edk2-x86_64-code.fd.bz2
1.9M    pc-bios/edk2-x86_64-secure-code.fd.bz2
9.3M    total

> locally. Although we have a rule to generate the reference file we
> missed the dependency and location changes, probably because it is
> only run for SLOW test runs.
> 
> The test still fails with mostly incorrect flags and different than
> expected NaNs. I'll leave that for the x86 experts to look at.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Jan Bobek <jan.bobek@gmail.com>
> ---
>  tests/tcg/i386/Makefile.target | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/tcg/i386/Makefile.target b/tests/tcg/i386/Makefile.target
> index b4033ba3d1..d0eb7023e5 100644
> --- a/tests/tcg/i386/Makefile.target
> +++ b/tests/tcg/i386/Makefile.target
> @@ -35,9 +35,9 @@ test-i386-fprem.ref: test-i386-fprem
>  	$(call quiet-command, ./$< > $@,"GENREF","generating $@")
>  
>  run-test-i386-fprem: TIMEOUT=60
> -run-test-i386-fprem: test-i386-fprem
> +run-test-i386-fprem: test-i386-fprem test-i386-fprem.ref
>  	$(call run-test,test-i386-fprem, $(QEMU) $<,"$< on $(TARGET_NAME)")
> -	$(call diff-out,test-i386-fprem, $(I386_SRC)/$<.ref)
> +	$(call diff-out,test-i386-fprem, test-i386-fprem.ref)
>  else
>  run-test-i386-fprem: test-i386-fprem
>  	$(call skip-test, $<, "SLOW")
> 

Re: [Qemu-devel] [PATCH v1 2/5] tests/tcg: fix up test-i386-fprem.ref generation

[Alex Bennée]

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

> On 7/5/19 6:04 PM, Alex Bennée wrote:
>> We never shipped the reference data in the source tree because it was
>> quite big (64M). As a result the only option is to generate it
>
> Can we fetch it (with hash verification) or store it compressed?

Seems like a lot of hoops to jump through. The limitation is that you
need to be on an x86 to generate the reference data but that isn't very
hard these days. If you do happen to be on non-x86 and are not paying
attention it becomes a simple regression test against the system
(binfmt_misc) run QEMU. But as it is currently broken and needing some
love it's just an incremental improvement for where we are at the
moment.

>
> $ du -ch pc-bios/edk2-*bz2
> 1.2M    pc-bios/edk2-aarch64-code.fd.bz2
> 1.2M    pc-bios/edk2-arm-code.fd.bz2
> 4.0K    pc-bios/edk2-arm-vars.fd.bz2
> 1.7M    pc-bios/edk2-i386-code.fd.bz2
> 1.9M    pc-bios/edk2-i386-secure-code.fd.bz2
> 4.0K    pc-bios/edk2-i386-vars.fd.bz2
> 1.7M    pc-bios/edk2-x86_64-code.fd.bz2
> 1.9M    pc-bios/edk2-x86_64-secure-code.fd.bz2
> 9.3M    total
>
>> locally. Although we have a rule to generate the reference file we
>> missed the dependency and location changes, probably because it is
>> only run for SLOW test runs.
>>
>> The test still fails with mostly incorrect flags and different than
>> expected NaNs. I'll leave that for the x86 experts to look at.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Jan Bobek <jan.bobek@gmail.com>
>> ---
>>  tests/tcg/i386/Makefile.target | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/tcg/i386/Makefile.target b/tests/tcg/i386/Makefile.target
>> index b4033ba3d1..d0eb7023e5 100644
>> --- a/tests/tcg/i386/Makefile.target
>> +++ b/tests/tcg/i386/Makefile.target
>> @@ -35,9 +35,9 @@ test-i386-fprem.ref: test-i386-fprem
>>  	$(call quiet-command, ./$< > $@,"GENREF","generating $@")
>>
>>  run-test-i386-fprem: TIMEOUT=60
>> -run-test-i386-fprem: test-i386-fprem
>> +run-test-i386-fprem: test-i386-fprem test-i386-fprem.ref
>>  	$(call run-test,test-i386-fprem, $(QEMU) $<,"$< on $(TARGET_NAME)")
>> -	$(call diff-out,test-i386-fprem, $(I386_SRC)/$<.ref)
>> +	$(call diff-out,test-i386-fprem, test-i386-fprem.ref)
>>  else
>>  run-test-i386-fprem: test-i386-fprem
>>  	$(call skip-test, $<, "SLOW")
>>


--
Alex Bennée

[Qemu-devel] [PATCH v1 3/5] tests/tcg: fix diff-out pass to properly report failure

[Alex Bennée]

A side effect of piping the output to head is squash the exit status
of the diff command. Fix this by only doing the pipe if the diff
failed and then ensuring the status is non-zero.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/tcg/Makefile b/tests/tcg/Makefile
index 6fa63cc8d5..7973cd1ba2 100644
--- a/tests/tcg/Makefile
+++ b/tests/tcg/Makefile
@@ -45,7 +45,11 @@ run-test = $(call quiet-command, timeout $(TIMEOUT) $2,"TEST",$3)
 endif
 
 # $1 = test name, $2 = reference
-diff-out = $(call quiet-command, diff -u $1.out $2 | head -n 10,"DIFF","$1.out with $2")
+# to work around the pipe squashing the status we only pipe the result if
+# we know it failed and then force failure at the end.
+diff-out = $(call quiet-command, diff -q $1.out $2 || \
+				 (diff -u $1.out $2 | head -n 10 && false), \
+				 "DIFF","$1.out with $2")
 
 # $1 = test name, $2 = reason
 skip-test = @printf "  SKIPPED %s on $(TARGET_NAME) because %s\n" $1 $2
-- 
2.20.1

Re: [Qemu-devel] [PATCH v1 3/5] tests/tcg: fix diff-out pass to properly report failure

[Richard Henderson]

On 7/5/19 6:04 PM, Alex Bennée wrote:
> +diff-out = $(call quiet-command, diff -q $1.out $2 || \
> +				 (diff -u $1.out $2 | head -n 10 && false), \

What about (set -o pipefail; diff ... | head) ?
I think we already rely on bash, right?


r~

Re: [Qemu-devel] [PATCH v1 3/5] tests/tcg: fix diff-out pass to properly report failure

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 7/5/19 6:04 PM, Alex Bennée wrote:
>> +diff-out = $(call quiet-command, diff -q $1.out $2 || \
>> +				 (diff -u $1.out $2 | head -n 10 && false), \
>
> What about (set -o pipefail; diff ... | head) ?
> I think we already rely on bash, right?

I don't think so - we assume POSIX shell for configure and AFAICT we
don't do anything special in the make system to set the shell type to
bash.

--
Alex Bennée

[Qemu-devel] [PATCH v1 4/5] gdbstub: add some notes to the header comment

[Alex Bennée]

Add a link to the remote protocol spec and an SPDX tag.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 8618e34311..ea3349d1aa 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1,6 +1,10 @@
 /*
  * gdb server stub
  *
+ * This implements a subset of the remote protocol as described in:
+ *
+ *   https://sourceware.org/gdb/onlinedocs/gdb/Remote-Protocol.html
+ *
  * Copyright (c) 2003-2005 Fabrice Bellard
  *
  * This library is free software; you can redistribute it and/or
@@ -15,6 +19,8 @@
  *
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
 #include "qemu/osdep.h"
-- 
2.20.1

Re: [Qemu-devel] [PATCH v1 4/5] gdbstub: add some notes to the header comment

[Philippe Mathieu-Daudé]

On 7/5/19 6:04 PM, Alex Bennée wrote:
> Add a link to the remote protocol spec and an SPDX tag.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  gdbstub.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 8618e34311..ea3349d1aa 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1,6 +1,10 @@
>  /*
>   * gdb server stub
>   *
> + * This implements a subset of the remote protocol as described in:
> + *
> + *   https://sourceware.org/gdb/onlinedocs/gdb/Remote-Protocol.html
> + *
>   * Copyright (c) 2003-2005 Fabrice Bellard
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -15,6 +19,8 @@
>   *
>   * You should have received a copy of the GNU Lesser General Public
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
>   */
>  
>  #include "qemu/osdep.h"
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH v1 4/5] gdbstub: add some notes to the header comment

[Richard Henderson]

On 7/5/19 6:04 PM, Alex Bennée wrote:
> Add a link to the remote protocol spec and an SPDX tag.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  gdbstub.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 8618e34311..ea3349d1aa 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1,6 +1,10 @@
>  /*
>   * gdb server stub
>   *
> + * This implements a subset of the remote protocol as described in:
> + *
> + *   https://sourceware.org/gdb/onlinedocs/gdb/Remote-Protocol.html
> + *

Ack.

>   * Copyright (c) 2003-2005 Fabrice Bellard
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -15,6 +19,8 @@
>   *
>   * You should have received a copy of the GNU Lesser General Public
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later

Nack.  The text is for the LGPL.


r~

Re: [Qemu-devel] [PATCH v1 4/5] gdbstub: add some notes to the header comment

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 7/5/19 6:04 PM, Alex Bennée wrote:
>> Add a link to the remote protocol spec and an SPDX tag.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  gdbstub.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 8618e34311..ea3349d1aa 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1,6 +1,10 @@
>>  /*
>>   * gdb server stub
>>   *
>> + * This implements a subset of the remote protocol as described in:
>> + *
>> + *   https://sourceware.org/gdb/onlinedocs/gdb/Remote-Protocol.html
>> + *
>
> Ack.
>
>>   * Copyright (c) 2003-2005 Fabrice Bellard
>>   *
>>   * This library is free software; you can redistribute it and/or
>> @@ -15,6 +19,8 @@
>>   *
>>   * You should have received a copy of the GNU Lesser General Public
>>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>
> Nack.  The text is for the LGPL.

Fixed in my PR.

--
Alex Bennée

[Qemu-devel] [PATCH v1 5/5] gdbstub: revert to previous set_reg behaviour

[Alex Bennée]

The refactoring of handle_set_reg missed the fact we previously had
responded with an empty packet when we were not using XML based
protocols. This broke the fallback behaviour for architectures that
don't have registers defined in QEMU's gdb-xml directory.

Revert to the previous behaviour and clean up the commentary for what
is going on.

Fixes: 62b3320bddd
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Jon Doron <arilou@gmail.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 gdbstub.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index ea3349d1aa..b6df7ee25a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1669,12 +1669,23 @@ static void handle_remove_bp(GdbCmdContext *gdb_ctx, void *user_ctx)
     put_packet(gdb_ctx->s, "E22");
 }
 
+/*
+ * handle_set/get_reg
+ *
+ * Older gdb are really dumb, and don't use 'G/g' if 'P/p' is available.
+ * This works, but can be very slow. Anything new enough to understand
+ * XML also knows how to use this properly. However to use this we
+ * need to define a local XML file as well as be talking to a
+ * reasonably modern gdb. Responding with an empty packet will cause
+ * the remote gdb to fallback to older methods.
+ */
+
 static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
 {
     int reg_size;
 
     if (!gdb_has_xml) {
-        put_packet(gdb_ctx->s, "E00");
+        put_packet(gdb_ctx->s, "");
         return;
     }
 
@@ -1694,11 +1705,6 @@ static void handle_get_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
 {
     int reg_size;
 
-    /*
-     * Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
-     * This works, but can be very slow.  Anything new enough to
-     * understand XML also knows how to use this properly.
-     */
     if (!gdb_has_xml) {
         put_packet(gdb_ctx->s, "");
         return;
-- 
2.20.1

Re: [Qemu-devel] [PATCH v1 5/5] gdbstub: revert to previous set_reg behaviour

[Richard Henderson]

On 7/5/19 6:04 PM, Alex Bennée wrote:
> The refactoring of handle_set_reg missed the fact we previously had
> responded with an empty packet when we were not using XML based
> protocols. This broke the fallback behaviour for architectures that
> don't have registers defined in QEMU's gdb-xml directory.
> 
> Revert to the previous behaviour and clean up the commentary for what
> is going on.
> 
> Fixes: 62b3320bddd
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Jon Doron <arilou@gmail.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  gdbstub.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v1 5/5] gdbstub: revert to previous set_reg behaviour

[Aleksandar Markovic]

On Jul 5, 2019 6:08 PM, "Alex Bennée" <alex.bennee@linaro.org> wrote:
>
> The refactoring of handle_set_reg missed the fact we previously had
> responded with an empty packet when we were not using XML based
> protocols. This broke the fallback behaviour for architectures that
> don't have registers defined in QEMU's gdb-xml directory.
>
> Revert to the previous behaviour and clean up the commentary for what
> is going on.
>
> Fixes: 62b3320bddd
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Jon Doron <arilou@gmail.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

Do you plan to integrate this patch in 4.1?

Thanks, Aleksandar

>  gdbstub.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index ea3349d1aa..b6df7ee25a 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1669,12 +1669,23 @@ static void handle_remove_bp(GdbCmdContext
*gdb_ctx, void *user_ctx)
>      put_packet(gdb_ctx->s, "E22");
>  }
>
> +/*
> + * handle_set/get_reg
> + *
> + * Older gdb are really dumb, and don't use 'G/g' if 'P/p' is available.
> + * This works, but can be very slow. Anything new enough to understand
> + * XML also knows how to use this properly. However to use this we
> + * need to define a local XML file as well as be talking to a
> + * reasonably modern gdb. Responding with an empty packet will cause
> + * the remote gdb to fallback to older methods.
> + */
> +
>  static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
>  {
>      int reg_size;
>
>      if (!gdb_has_xml) {
> -        put_packet(gdb_ctx->s, "E00");
> +        put_packet(gdb_ctx->s, "");
>          return;
>      }
>
> @@ -1694,11 +1705,6 @@ static void handle_get_reg(GdbCmdContext *gdb_ctx,
void *user_ctx)
>  {
>      int reg_size;
>
> -    /*
> -     * Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
> -     * This works, but can be very slow.  Anything new enough to
> -     * understand XML also knows how to use this properly.
> -     */
>      if (!gdb_has_xml) {
>          put_packet(gdb_ctx->s, "");
>          return;
> --
> 2.20.1
>
>

Re: [Qemu-devel] [PATCH v1 5/5] gdbstub: revert to previous set_reg behaviour

[Alex Bennée]

Aleksandar Markovic <aleksandar.m.mail@gmail.com> writes:

> On Jul 5, 2019 6:08 PM, "Alex Bennée" <alex.bennee@linaro.org> wrote:
>>
>> The refactoring of handle_set_reg missed the fact we previously had
>> responded with an empty packet when we were not using XML based
>> protocols. This broke the fallback behaviour for architectures that
>> don't have registers defined in QEMU's gdb-xml directory.
>>
>> Revert to the previous behaviour and clean up the commentary for what
>> is going on.
>>
>> Fixes: 62b3320bddd
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Jon Doron <arilou@gmail.com>
>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>
> Do you plan to integrate this patch in 4.1?
>
> Thanks, Aleksandar

Yes - I'm putting together a PR today.

>
>>  gdbstub.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index ea3349d1aa..b6df7ee25a 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1669,12 +1669,23 @@ static void handle_remove_bp(GdbCmdContext
> *gdb_ctx, void *user_ctx)
>>      put_packet(gdb_ctx->s, "E22");
>>  }
>>
>> +/*
>> + * handle_set/get_reg
>> + *
>> + * Older gdb are really dumb, and don't use 'G/g' if 'P/p' is available.
>> + * This works, but can be very slow. Anything new enough to understand
>> + * XML also knows how to use this properly. However to use this we
>> + * need to define a local XML file as well as be talking to a
>> + * reasonably modern gdb. Responding with an empty packet will cause
>> + * the remote gdb to fallback to older methods.
>> + */
>> +
>>  static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
>>  {
>>      int reg_size;
>>
>>      if (!gdb_has_xml) {
>> -        put_packet(gdb_ctx->s, "E00");
>> +        put_packet(gdb_ctx->s, "");
>>          return;
>>      }
>>
>> @@ -1694,11 +1705,6 @@ static void handle_get_reg(GdbCmdContext *gdb_ctx,
> void *user_ctx)
>>  {
>>      int reg_size;
>>
>> -    /*
>> -     * Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
>> -     * This works, but can be very slow.  Anything new enough to
>> -     * understand XML also knows how to use this properly.
>> -     */
>>      if (!gdb_has_xml) {
>>          put_packet(gdb_ctx->s, "");
>>          return;
>> --
>> 2.20.1
>>
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 5/5] gdbstub: revert to previous set_reg behaviour

[Aleksandar Markovic]

On Jul 10, 2019 11:30 AM, "Alex Bennée" <alex.bennee@linaro.org> wrote:
>
>
> Aleksandar Markovic <aleksandar.m.mail@gmail.com> writes:
>
> > On Jul 5, 2019 6:08 PM, "Alex Bennée" <alex.bennee@linaro.org> wrote:
> >>
> >> The refactoring of handle_set_reg missed the fact we previously had
> >> responded with an empty packet when we were not using XML based
> >> protocols. This broke the fallback behaviour for architectures that
> >> don't have registers defined in QEMU's gdb-xml directory.
> >>
> >> Revert to the previous behaviour and clean up the commentary for what
> >> is going on.
> >>
> >> Fixes: 62b3320bddd
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Cc: Jon Doron <arilou@gmail.com>
> >> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >
> > Do you plan to integrate this patch in 4.1?
> >
> > Thanks, Aleksandar
>
> Yes - I'm putting together a PR today.
>

That's great, thanks!!

Aleksandar

> >
> >>  gdbstub.c | 18 ++++++++++++------
> >>  1 file changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/gdbstub.c b/gdbstub.c
> >> index ea3349d1aa..b6df7ee25a 100644
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -1669,12 +1669,23 @@ static void handle_remove_bp(GdbCmdContext
> > *gdb_ctx, void *user_ctx)
> >>      put_packet(gdb_ctx->s, "E22");
> >>  }
> >>
> >> +/*
> >> + * handle_set/get_reg
> >> + *
> >> + * Older gdb are really dumb, and don't use 'G/g' if 'P/p' is
available.
> >> + * This works, but can be very slow. Anything new enough to understand
> >> + * XML also knows how to use this properly. However to use this we
> >> + * need to define a local XML file as well as be talking to a
> >> + * reasonably modern gdb. Responding with an empty packet will cause
> >> + * the remote gdb to fallback to older methods.
> >> + */
> >> +
> >>  static void handle_set_reg(GdbCmdContext *gdb_ctx, void *user_ctx)
> >>  {
> >>      int reg_size;
> >>
> >>      if (!gdb_has_xml) {
> >> -        put_packet(gdb_ctx->s, "E00");
> >> +        put_packet(gdb_ctx->s, "");
> >>          return;
> >>      }
> >>
> >> @@ -1694,11 +1705,6 @@ static void handle_get_reg(GdbCmdContext
*gdb_ctx,
> > void *user_ctx)
> >>  {
> >>      int reg_size;
> >>
> >> -    /*
> >> -     * Older gdb are really dumb, and don't use 'g' if 'p' is
avaialable.
> >> -     * This works, but can be very slow.  Anything new enough to
> >> -     * understand XML also knows how to use this properly.
> >> -     */
> >>      if (!gdb_has_xml) {
> >>          put_packet(gdb_ctx->s, "");
> >>          return;
> >> --
> >> 2.20.1
> >>
> >>
>
>
> --
> Alex Bennée