[PATCH v1 0/9] gdbstub/next

[PATCH v1 0/9] gdbstub/next

[Alex Bennée]

Hi,

As another release is cut from the tree we start again the collection
of patches fixes and enhancements that weren't yet ready to be
released on an unsuspecting world.

Some of these patches have been seen before in my random collection
series but these are all gdbstub related, This includes more testing,
a new unix socket mode for linux-user to support the testing and a few
other bits and pieces.

The following patches need review:

 - tests/tcg: add a multiarch linux-user gdb test
 - tests/guest-debug: use the unix socket for linux-user tests
 - gdbstub/linux-user: support debugging over a unix socket
 - gdbstub: eliminate gdbserver_fd global
 - tests/tcg: drop inferior.was_attached() test
 - tests/tcg: better trap gdb failures
 - configure: favour gdb-multiarch if we have it

Alex Bennée (7):
  configure: favour gdb-multiarch if we have it
  tests/tcg: better trap gdb failures
  tests/tcg: drop inferior.was_attached() test
  gdbstub: eliminate gdbserver_fd global
  gdbstub/linux-user: support debugging over a unix socket
  tests/guest-debug: use the unix socket for linux-user tests
  tests/tcg: add a multiarch linux-user gdb test

KONRAD Frederic (1):
  target/m68k: fix gdb for m68xxx

Philippe Mathieu-Daudé (1):
  gdbstub: Introduce gdb_get_float64() to get 64-bit float registers

 configure                                   |   4 +-
 include/exec/gdbstub.h                      |  25 +++-
 bsd-user/main.c                             |   8 +-
 gdbstub.c                                   | 119 ++++++++++++++++----
 linux-user/main.c                           |  12 +-
 target/m68k/cpu.c                           |  52 ++++++---
 target/m68k/helper.c                        |   3 +-
 target/ppc/gdbstub.c                        |   4 +-
 target/ppc/translate_init.inc.c             |   2 +-
 gdb-xml/m68k-core.xml                       |  29 +++++
 tests/guest-debug/run-test.py               |  30 ++++-
 tests/tcg/aarch64/Makefile.target           |   5 +-
 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py |   4 -
 tests/tcg/aarch64/gdbstub/test-sve.py       |   4 -
 tests/tcg/cris/Makefile.target              |   1 +
 tests/tcg/multiarch/Makefile.target         |  14 +++
 tests/tcg/multiarch/gdbstub/sha1.py         |  81 +++++++++++++
 17 files changed, 323 insertions(+), 74 deletions(-)
 create mode 100644 gdb-xml/m68k-core.xml
 create mode 100644 tests/tcg/multiarch/gdbstub/sha1.py

-- 
2.20.1

[PATCH v1 1/9] configure: favour gdb-multiarch if we have it

[Alex Bennée]

As gdb will generally be talking to "foreign" guests lets use that if
we can. Otherwise the chances of gdb barfing are considerably higher.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 23b5e93752..c58787100f 100755
--- a/configure
+++ b/configure
@@ -303,7 +303,7 @@ libs_qga=""
 debug_info="yes"
 stack_protector=""
 use_containers="yes"
-gdb_bin=$(command -v "gdb")
+gdb_bin=$(command -v "gdb-multiarch" || command -v "gdb")
 
 if test -e "$source_path/.git"
 then
-- 
2.20.1

[PATCH v1 2/9] gdbstub: Introduce gdb_get_float64() to get 64-bit float registers

[Alex Bennée]

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

When converted to use GByteArray in commits 462474d760c and
a010bdbe719, the call to stfq_p() was removed. This call
serialize a float.
Since we now use a GByteArray, we can not use stfq_p() directly.
Introduce the gdb_get_float64() helper to load a float64 register.

Fixes: 462474d760c ("target/m68k: use gdb_get_reg helpers")
Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200414163853.12164-3-philmd@redhat.com>
---
 include/exec/gdbstub.h          | 11 +++++++++++
 target/m68k/helper.c            |  3 ++-
 target/ppc/gdbstub.c            |  4 ++--
 target/ppc/translate_init.inc.c |  2 +-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 20e1072692..4a2b8e3089 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -134,6 +134,17 @@ static inline int gdb_get_float32(GByteArray *array, float32 val)
 
     return sizeof(buf);
 }
+
+static inline int gdb_get_float64(GByteArray *array, float64 val)
+{
+    uint8_t buf[sizeof(CPU_DoubleU)];
+
+    stfq_p(buf, val);
+    g_byte_array_append(array, buf, sizeof(buf));
+
+    return sizeof(buf);
+}
+
 static inline int gdb_get_zeroes(GByteArray *array, size_t len)
 {
     guint oldlen = array->len;
diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index cad4083895..79b0b10ea9 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -72,7 +72,8 @@ static int cf_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
 {
     if (n < 8) {
         float_status s;
-        return gdb_get_reg64(mem_buf, floatx80_to_float64(env->fregs[n].d, &s));
+        return gdb_get_float64(mem_buf,
+                               floatx80_to_float64(env->fregs[n].d, &s));
     }
     switch (n) {
     case 8: /* fpcontrol */
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index eb362dd9ae..5c11c88b2a 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -130,7 +130,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
         gdb_get_regl(buf, env->gpr[n]);
     } else if (n < 64) {
         /* fprs */
-        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
+        gdb_get_float64(buf, *cpu_fpr_ptr(env, n - 32));
     } else {
         switch (n) {
         case 64:
@@ -184,7 +184,7 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
         gdb_get_reg64(buf, env->gpr[n]);
     } else if (n < 64) {
         /* fprs */
-        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
+        gdb_get_float64(buf, *cpu_fpr_ptr(env, n - 32));
     } else if (n < 96) {
         /* Altivec */
         gdb_get_reg64(buf, n - 64);
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index e853164a86..d825cb5975 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -9881,7 +9881,7 @@ static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
 {
     uint8_t *mem_buf;
     if (n < 32) {
-        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
+        gdb_get_float64(buf, *cpu_fpr_ptr(env, n));
         mem_buf = gdb_get_reg_ptr(buf, 8);
         ppc_maybe_bswap_register(env, mem_buf, 8);
         return 8;
-- 
2.20.1

[PATCH v1 3/9] tests/tcg: better trap gdb failures

[Alex Bennée]

It seems older and non-multiarach aware GDBs might not fail gracefully
when faced with something they don't know. For example when faced with
a target XML for s390x the Ubuntu 18.04 gdb will generate an internal
fault and prompt for a core dump.

Work around this by invoking GDB in a more batch orientated way and
then trying to filter out between test failures and gdb failures.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/guest-debug/run-test.py               | 19 ++++++++++++++++++-
 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py |  1 -
 tests/tcg/aarch64/gdbstub/test-sve.py       |  1 -
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index 8c49ee2f22..2bbb8fbaa3 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -50,8 +50,25 @@ if __name__ == '__main__':
     inferior = subprocess.Popen(shlex.split(cmd))
 
     # Now launch gdb with our test and collect the result
-    gdb_cmd = "%s %s -ex 'target remote localhost:1234' -x %s" % (args.gdb, args.binary, args.test)
+    gdb_cmd = "%s %s" % (args.gdb, args.binary)
+    # run quietly and ignore .gdbinit
+    gdb_cmd += " -q -n -batch"
+    # disable prompts in case of crash
+    gdb_cmd += " -ex 'set confirm off'"
+    # connect to remote
+    gdb_cmd += " -ex 'target remote localhost:1234'"
+    # finally the test script itself
+    gdb_cmd += " -x %s" % (args.test)
+
+    print("GDB CMD: %s" % (gdb_cmd))
 
     result = subprocess.call(gdb_cmd, shell=True);
 
+    # A negative result is the result of an internal gdb failure like
+    # a crash. We force a return of 0 so we don't fail the test on
+    # account of broken external tools.
+    if result < 0:
+        print("GDB crashed? SKIPPING")
+        exit(0)
+
     exit(result)
diff --git a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
index 984fbeb277..387b2fc20a 100644
--- a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
+++ b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
@@ -70,7 +70,6 @@ except (gdb.error, AttributeError):
 try:
     # These are not very useful in scripts
     gdb.execute("set pagination off")
-    gdb.execute("set confirm off")
 
     # Run the actual tests
     run_test()
diff --git a/tests/tcg/aarch64/gdbstub/test-sve.py b/tests/tcg/aarch64/gdbstub/test-sve.py
index dbe7f2aa93..5995689625 100644
--- a/tests/tcg/aarch64/gdbstub/test-sve.py
+++ b/tests/tcg/aarch64/gdbstub/test-sve.py
@@ -71,7 +71,6 @@ except (gdb.error, AttributeError):
 try:
     # These are not very useful in scripts
     gdb.execute("set pagination off")
-    gdb.execute("set confirm off")
 
     # Run the actual tests
     run_test()
-- 
2.20.1

[PATCH v1 4/9] tests/tcg: drop inferior.was_attached() test

[Alex Bennée]

This test seems flaky and reports attachment even when we failed to
negotiate the architecture. However the fetching of the guest
architecture will fail tripping up the gdb AttributeError which will
trigger our early no error status exit from the test

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py | 3 ---
 tests/tcg/aarch64/gdbstub/test-sve.py       | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
index 387b2fc20a..972cf73c31 100644
--- a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
+++ b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
@@ -58,9 +58,6 @@ def run_test():
 #
 try:
     inferior = gdb.selected_inferior()
-    if inferior.was_attached == False:
-        print("SKIPPING (failed to attach)", file=sys.stderr)
-        exit(0)
     arch = inferior.architecture()
     report(arch.name() == "aarch64", "connected to aarch64")
 except (gdb.error, AttributeError):
diff --git a/tests/tcg/aarch64/gdbstub/test-sve.py b/tests/tcg/aarch64/gdbstub/test-sve.py
index 5995689625..b96bdbb99a 100644
--- a/tests/tcg/aarch64/gdbstub/test-sve.py
+++ b/tests/tcg/aarch64/gdbstub/test-sve.py
@@ -59,9 +59,6 @@ def run_test():
 #
 try:
     inferior = gdb.selected_inferior()
-    if inferior.was_attached == False:
-        print("SKIPPING (failed to attach)", file=sys.stderr)
-        exit(0)
     arch = inferior.architecture()
     report(arch.name() == "aarch64", "connected to aarch64")
 except (gdb.error, AttributeError):
-- 
2.20.1

[PATCH v1 5/9] gdbstub: eliminate gdbserver_fd global

[Alex Bennée]

We don't really need to track this fd beyond the initial creation of
the socket. We already know if the system has been initialised by
virtue of the gdbserver_state so lets remove it. This makes the later
re-factoring easier.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v1
  - fix coding style issue
---
 gdbstub.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 171e150950..b5381aa520 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -398,8 +398,6 @@ static void reset_gdbserver_state(void)
 bool gdb_has_xml;
 
 #ifdef CONFIG_USER_ONLY
-/* XXX: This is not thread safe.  Do we care?  */
-static int gdbserver_fd = -1;
 
 static int get_char(void)
 {
@@ -2964,7 +2962,7 @@ void gdb_exit(CPUArchState *env, int code)
       return;
   }
 #ifdef CONFIG_USER_ONLY
-  if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
+  if (gdbserver_state.fd < 0) {
       return;
   }
 #endif
@@ -3011,7 +3009,7 @@ gdb_handlesig(CPUState *cpu, int sig)
     char buf[256];
     int n;
 
-    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
         return sig;
     }
 
@@ -3060,7 +3058,7 @@ void gdb_signalled(CPUArchState *env, int sig)
 {
     char buf[4];
 
-    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
         return;
     }
 
@@ -3068,7 +3066,7 @@ void gdb_signalled(CPUArchState *env, int sig)
     put_packet(buf);
 }
 
-static bool gdb_accept(void)
+static bool gdb_accept(int gdb_fd)
 {
     struct sockaddr_in sockaddr;
     socklen_t len;
@@ -3076,7 +3074,7 @@ static bool gdb_accept(void)
 
     for(;;) {
         len = sizeof(sockaddr);
-        fd = accept(gdbserver_fd, (struct sockaddr *)&sockaddr, &len);
+        fd = accept(gdb_fd, (struct sockaddr *)&sockaddr, &len);
         if (fd < 0 && errno != EINTR) {
             perror("accept");
             return false;
@@ -3137,13 +3135,13 @@ static int gdbserver_open(int port)
 
 int gdbserver_start(int port)
 {
-    gdbserver_fd = gdbserver_open(port);
-    if (gdbserver_fd < 0)
+    int gdb_fd = gdbserver_open(port);
+    if (gdb_fd < 0) {
         return -1;
+    }
     /* accept connections */
-    if (!gdb_accept()) {
-        close(gdbserver_fd);
-        gdbserver_fd = -1;
+    if (!gdb_accept(gdb_fd)) {
+        close(gdb_fd);
         return -1;
     }
     return 0;
@@ -3152,7 +3150,7 @@ int gdbserver_start(int port)
 /* Disable gdb stub for child processes.  */
 void gdbserver_fork(CPUState *cpu)
 {
-    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
         return;
     }
     close(gdbserver_state.fd);
-- 
2.20.1

[PATCH v1 6/9] gdbstub/linux-user: support debugging over a unix socket

[Alex Bennée]

While debugging over TCP is fairly straightforward now we have test
cases that want to orchestrate via make and currently a parallel build
fails as two processes can't use the same listening port. While system
emulation offers a wide cornucopia of connection methods thanks to the
chardev abstraction we are a little more limited for linux user.
Thankfully the programming API for a TCP socket and a local UNIX
socket is pretty much the same once it's set up.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/gdbstub.h |  14 ++++--
 bsd-user/main.c        |   8 ++--
 gdbstub.c              | 103 ++++++++++++++++++++++++++++++++++-------
 linux-user/main.c      |  12 ++---
 4 files changed, 106 insertions(+), 31 deletions(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 4a2b8e3089..94d8f83e92 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -177,11 +177,15 @@ static inline uint8_t * gdb_get_reg_ptr(GByteArray *buf, int len)
 
 #endif
 
-#ifdef CONFIG_USER_ONLY
-int gdbserver_start(int);
-#else
-int gdbserver_start(const char *port);
-#endif
+/**
+ * gdbserver_start: start the gdb server
+ * @port_or_device: connection spec for gdb
+ *
+ * For CONFIG_USER this is either a tcp port or a path to a fifo. For
+ * system emulation you can use a full chardev spec for your gdbserver
+ * port.
+ */
+int gdbserver_start(const char *port_or_device);
 
 void gdbserver_cleanup(void);
 
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 770c2b267a..28f122b80e 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -738,7 +738,7 @@ int main(int argc, char **argv)
     CPUState *cpu;
     int optind;
     const char *r;
-    int gdbstub_port = 0;
+    const char *gdbstub = NULL;
     char **target_environ, **wrk;
     envlist_t *envlist = NULL;
     char *trace_file = NULL;
@@ -814,7 +814,7 @@ int main(int argc, char **argv)
                 exit(1);
             }
         } else if (!strcmp(r, "g")) {
-            gdbstub_port = atoi(argv[optind++]);
+            gdbstub = g_strdup(argv[optind++]);
         } else if (!strcmp(r, "r")) {
             qemu_uname_release = argv[optind++];
         } else if (!strcmp(r, "cpu")) {
@@ -1124,8 +1124,8 @@ int main(int argc, char **argv)
 #error unsupported target CPU
 #endif
 
-    if (gdbstub_port) {
-        gdbserver_start (gdbstub_port);
+    if (gdbstub) {
+        gdbserver_start(gdbstub);
         gdb_handlesig(cpu, 0);
     }
     cpu_loop(env);
diff --git a/gdbstub.c b/gdbstub.c
index b5381aa520..6950fd243f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -355,6 +355,7 @@ typedef struct GDBState {
     int signal;
 #ifdef CONFIG_USER_ONLY
     int fd;
+    char *socket_path;
     int running_state;
 #else
     CharBackend chr;
@@ -2962,6 +2963,9 @@ void gdb_exit(CPUArchState *env, int code)
       return;
   }
 #ifdef CONFIG_USER_ONLY
+  if (gdbserver_state.socket_path) {
+      unlink(gdbserver_state.socket_path);
+  }
   if (gdbserver_state.fd < 0) {
       return;
   }
@@ -3066,7 +3070,66 @@ void gdb_signalled(CPUArchState *env, int sig)
     put_packet(buf);
 }
 
-static bool gdb_accept(int gdb_fd)
+static void gdb_accept_init(int fd)
+{
+    init_gdbserver_state();
+    create_default_process(&gdbserver_state);
+    gdbserver_state.processes[0].attached = true;
+    gdbserver_state.c_cpu = gdb_first_attached_cpu();
+    gdbserver_state.g_cpu = gdbserver_state.c_cpu;
+    gdbserver_state.fd = fd;
+    gdb_has_xml = false;
+}
+
+static bool gdb_accept_socket(int gdb_fd)
+{
+    int fd;
+
+    for(;;) {
+        fd = accept(gdb_fd, NULL, NULL);
+        if (fd < 0 && errno != EINTR) {
+            perror("accept socket");
+            return false;
+        } else if (fd >= 0) {
+            qemu_set_cloexec(fd);
+            break;
+        }
+    }
+
+    gdb_accept_init(fd);
+    return true;
+}
+
+static int gdbserver_open_socket(const char *path)
+{
+    struct sockaddr_un sockaddr;
+    int fd, ret;
+
+    fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd < 0) {
+        perror("create socket");
+        return -1;
+    }
+
+    sockaddr.sun_family = AF_UNIX;
+    pstrcpy(sockaddr.sun_path, sizeof(sockaddr.sun_path) - 1, path);
+    ret = bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr));
+    if (ret < 0) {
+        perror("bind socket");
+        close(fd);
+        return -1;
+    }
+    ret = listen(fd, 1);
+    if (ret < 0) {
+        perror("listen socket");
+        close(fd);
+        return -1;
+    }
+
+    return fd;
+}
+
+static bool gdb_accept_tcp(int gdb_fd)
 {
     struct sockaddr_in sockaddr;
     socklen_t len;
@@ -3091,17 +3154,11 @@ static bool gdb_accept(int gdb_fd)
         return false;
     }
 
-    init_gdbserver_state();
-    create_default_process(&gdbserver_state);
-    gdbserver_state.processes[0].attached = true;
-    gdbserver_state.c_cpu = gdb_first_attached_cpu();
-    gdbserver_state.g_cpu = gdbserver_state.c_cpu;
-    gdbserver_state.fd = fd;
-    gdb_has_xml = false;
+    gdb_accept_init(fd);
     return true;
 }
 
-static int gdbserver_open(int port)
+static int gdbserver_open_port(int port)
 {
     struct sockaddr_in sockaddr;
     int fd, ret;
@@ -3130,21 +3187,35 @@ static int gdbserver_open(int port)
         close(fd);
         return -1;
     }
+
     return fd;
 }
 
-int gdbserver_start(int port)
+int gdbserver_start(const char *port_or_path)
 {
-    int gdb_fd = gdbserver_open(port);
+    int port = g_ascii_strtoull(port_or_path, NULL, 10);
+    int gdb_fd;
+
+    if (port > 0) {
+        gdb_fd = gdbserver_open_port(port);
+    } else {
+        gdb_fd = gdbserver_open_socket(port_or_path);
+    }
+
     if (gdb_fd < 0) {
         return -1;
     }
-    /* accept connections */
-    if (!gdb_accept(gdb_fd)) {
-        close(gdb_fd);
-        return -1;
+
+    if (port > 0 && gdb_accept_tcp(gdb_fd)) {
+        return 0;
+    } else if (gdb_accept_socket(gdb_fd)) {
+        gdbserver_state.socket_path = g_strdup(port_or_path);
+        return 0;
     }
-    return 0;
+
+    /* gone wrong */
+    close(gdb_fd);
+    return -1;
 }
 
 /* Disable gdb stub for child processes.  */
diff --git a/linux-user/main.c b/linux-user/main.c
index 22578b1633..2cd443237d 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -51,7 +51,7 @@ char *exec_path;
 
 int singlestep;
 static const char *argv0;
-static int gdbstub_port;
+static const char *gdbstub;
 static envlist_t *envlist;
 static const char *cpu_model;
 static const char *cpu_type;
@@ -310,7 +310,7 @@ static void handle_arg_seed(const char *arg)
 
 static void handle_arg_gdb(const char *arg)
 {
-    gdbstub_port = atoi(arg);
+    gdbstub = g_strdup(arg);
 }
 
 static void handle_arg_uname(const char *arg)
@@ -861,10 +861,10 @@ int main(int argc, char **argv, char **envp)
 
     target_cpu_copy_regs(env, regs);
 
-    if (gdbstub_port) {
-        if (gdbserver_start(gdbstub_port) < 0) {
-            fprintf(stderr, "qemu: could not open gdbserver on port %d\n",
-                    gdbstub_port);
+    if (gdbstub) {
+        if (gdbserver_start(gdbstub) < 0) {
+            fprintf(stderr, "qemu: could not open gdbserver on %s\n",
+                    gdbstub);
             exit(EXIT_FAILURE);
         }
         gdb_handlesig(cpu, 0);
-- 
2.20.1

[PATCH v1 7/9] tests/guest-debug: use the unix socket for linux-user tests

[Alex Bennée]

Now we have support for debugging over a unix socket for linux-user
lets use it in our test harness.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/guest-debug/run-test.py | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index 2bbb8fbaa3..d9af9573b9 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -15,6 +15,8 @@ import argparse
 import subprocess
 import shutil
 import shlex
+import os
+from tempfile import TemporaryDirectory
 
 def get_args():
     parser = argparse.ArgumentParser(description="A gdbstub test runner")
@@ -41,11 +43,15 @@ if __name__ == '__main__':
         print("We need gdb to run the test")
         exit(-1)
 
+    socket_dir = TemporaryDirectory("qemu-gdbstub")
+    socket_name = os.path.join(socket_dir.name, "gdbstub.socket")
+
     # Launch QEMU with binary
     if "system" in args.qemu:
         cmd = "%s %s %s -s -S" % (args.qemu, args.qargs, args.binary)
     else:
-        cmd = "%s %s -g 1234 %s" % (args.qemu, args.qargs, args.binary)
+        cmd = "%s %s -g %s %s" % (args.qemu, args.qargs, socket_name,
+                                  args.binary)
 
     inferior = subprocess.Popen(shlex.split(cmd))
 
@@ -56,7 +62,10 @@ if __name__ == '__main__':
     # disable prompts in case of crash
     gdb_cmd += " -ex 'set confirm off'"
     # connect to remote
-    gdb_cmd += " -ex 'target remote localhost:1234'"
+    if "system" in args.qemu:
+        gdb_cmd += " -ex 'target remote localhost:1234'"
+    else:
+        gdb_cmd += " -ex 'target remote %s'" % (socket_name)
     # finally the test script itself
     gdb_cmd += " -x %s" % (args.test)
 
-- 
2.20.1

[PATCH v1 8/9] tests/tcg: add a multiarch linux-user gdb test

[Alex Bennée]

When the gdbstub code was converted to the new API we missed a few
snafus in the various guests. Add a simple gdb test script which can
be used on all our linux-user guests to check for obvious failures.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use EXTRA_RUNS to queue the tests so as not to break plugins
---
 tests/tcg/aarch64/Makefile.target   |  5 +-
 tests/tcg/cris/Makefile.target      |  1 +
 tests/tcg/multiarch/Makefile.target | 14 +++++
 tests/tcg/multiarch/gdbstub/sha1.py | 81 +++++++++++++++++++++++++++++
 4 files changed, 98 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/multiarch/gdbstub/sha1.py

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index d99b2a9ece..312f36cde5 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -54,9 +54,6 @@ sve-ioctls: CFLAGS+=-march=armv8.1-a+sve
 ifneq ($(HAVE_GDB_BIN),)
 GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
 
-AARCH64_TESTS += gdbstub-sysregs gdbstub-sve-ioctls
-
-.PHONY: gdbstub-sysregs gdbstub-sve-ioctls
 run-gdbstub-sysregs: sysregs
 	$(call run-test, $@, $(GDB_SCRIPT) \
 		--gdb $(HAVE_GDB_BIN) \
@@ -70,6 +67,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
 		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
 		--bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-ioctl.py, \
 	"basic gdbstub SVE ZLEN support")
+
+EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
 endif
 
 endif
diff --git a/tests/tcg/cris/Makefile.target b/tests/tcg/cris/Makefile.target
index 24c7f2e761..e72d3cbdb2 100644
--- a/tests/tcg/cris/Makefile.target
+++ b/tests/tcg/cris/Makefile.target
@@ -23,6 +23,7 @@ CRIS_RUNS = $(patsubst %, run-%, $(CRIS_USABLE_TESTS))
 
 # override the list of tests, as we can't build the multiarch tests
 TESTS = $(CRIS_USABLE_TESTS)
+EXTRA_RUNS =
 VPATH = $(CRIS_SRC)
 
 AS	= $(CC) -x assembler-with-cpp
diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 035b09c853..51fb75ecfd 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -42,5 +42,19 @@ run-test-mmap-%: test-mmap
 	$(call run-test, test-mmap-$*, $(QEMU) -p $* $<,\
 		"$< ($* byte pages) on $(TARGET_NAME)")
 
+ifneq ($(HAVE_GDB_BIN),)
+GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
+
+run-gdbstub-sha1: sha1
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(HAVE_GDB_BIN) \
+		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+		--bin $< --test $(MULTIARCH_SRC)/gdbstub/sha1.py, \
+	"basic gdbstub support")
+
+EXTRA_RUNS += run-gdbstub-sha1
+endif
+
+
 # Update TESTS
 TESTS += $(MULTIARCH_TESTS)
diff --git a/tests/tcg/multiarch/gdbstub/sha1.py b/tests/tcg/multiarch/gdbstub/sha1.py
new file mode 100644
index 0000000000..734553b98b
--- /dev/null
+++ b/tests/tcg/multiarch/gdbstub/sha1.py
@@ -0,0 +1,81 @@
+from __future__ import print_function
+#
+# A very simple smoke test for debugging the SHA1 userspace test on
+# each target.
+#
+# This is launched via tests/guest-debug/run-test.py
+#
+
+import gdb
+import sys
+
+initial_vlen = 0
+failcount = 0
+
+def report(cond, msg):
+    "Report success/fail of test"
+    if cond:
+        print("PASS: %s" % (msg))
+    else:
+        print("FAIL: %s" % (msg))
+        global failcount
+        failcount += 1
+
+def check_break(sym_name):
+    "Setup breakpoint, continue and check we stopped."
+    sym, ok = gdb.lookup_symbol(sym_name)
+    bp = gdb.Breakpoint(sym_name)
+
+    gdb.execute("c")
+
+    # hopefully we came back
+    end_pc = gdb.parse_and_eval('$pc')
+    report(bp.hit_count == 1,
+           "break @ %s (%s %d hits)" % (end_pc, sym.value(), bp.hit_count))
+
+    bp.delete()
+
+def run_test():
+    "Run through the tests one by one"
+
+    check_break("SHA1Init")
+
+    # check step and inspect values
+    gdb.execute("next")
+    val_ctx = gdb.parse_and_eval("context->state[0]")
+    exp_ctx = 0x67452301
+    report(int(val_ctx) == exp_ctx, "context->state[0] == %x" % exp_ctx);
+
+    gdb.execute("next")
+    val_ctx = gdb.parse_and_eval("context->state[1]")
+    exp_ctx = 0xEFCDAB89
+    report(int(val_ctx) == exp_ctx, "context->state[1] == %x" % exp_ctx);
+
+    # finally check we don't barf inspecting registers
+    gdb.execute("info registers")
+
+#
+# This runs as the script it sourced (via -x, via run-test.py)
+#
+try:
+    inferior = gdb.selected_inferior()
+    arch = inferior.architecture()
+    print("ATTACHED: %s" % arch.name())
+except (gdb.error, AttributeError):
+    print("SKIPPING (not connected)", file=sys.stderr)
+    exit(0)
+
+try:
+    # These are not very useful in scripts
+    gdb.execute("set pagination off")
+    gdb.execute("set confirm off")
+
+    # Run the actual tests
+    run_test()
+except (gdb.error):
+    print ("GDB Exception: %s" % (sys.exc_info()[0]))
+    failcount += 1
+    pass
+
+print("All tests complete: %d failures" % failcount)
+exit(failcount)
-- 
2.20.1

[PATCH v1 9/9] target/m68k: fix gdb for m68xxx

[Alex Bennée]

From: KONRAD Frederic <frederic.konrad@adacore.com>

Currently "cf-core.xml" is sent to GDB when using any m68k flavor.  Thing is
it uses the "org.gnu.gdb.coldfire.core" feature name and gdb 8.3 then expects
a coldfire FPU instead of the default m68881 FPU.

This is not OK because the m68881 floats registers are 96 bits wide so it
crashes GDB with the following error message:

(gdb) target remote localhost:7960
Remote debugging using localhost:7960
warning: Register "fp0" has an unsupported size (96 bits)
warning: Register "fp1" has an unsupported size (96 bits)
...
Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
  00000000000[...]0000

With this patch: qemu-system-m68k -M none -cpu m68020 -s -S

(gdb) tar rem :1234
Remote debugging using :1234
warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
0x00000000 in ?? ()
(gdb) p $fp0
$1 = nan(0xffffffffffffffff)

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <1588094279-17913-3-git-send-email-frederic.konrad@adacore.com>
---
 configure             |  2 +-
 target/m68k/cpu.c     | 52 ++++++++++++++++++++++++++++++-------------
 gdb-xml/m68k-core.xml | 29 ++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 16 deletions(-)
 create mode 100644 gdb-xml/m68k-core.xml

diff --git a/configure b/configure
index c58787100f..0d69c360c0 100755
--- a/configure
+++ b/configure
@@ -7825,7 +7825,7 @@ case "$target_name" in
   ;;
   m68k)
     bflt="yes"
-    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
+    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
     TARGET_SYSTBL_ABI=common
   ;;
   microblaze|microblazeel)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 9445fcd6df..72c545149e 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -292,16 +292,38 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
     cc->tcg_initialize = m68k_tcg_init;
 
     cc->gdb_num_core_regs = 18;
-    cc->gdb_core_xml_file = "cf-core.xml";
 
     dc->vmsd = &vmstate_m68k_cpu;
 }
 
-#define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
-    {                                           \
-        .name = M68K_CPU_TYPE_NAME(cpu_model),  \
-        .instance_init = initfn,                \
-        .parent = TYPE_M68K_CPU,                \
+static void m68k_cpu_class_init_cf_core(ObjectClass *c, void *data)
+{
+    CPUClass *cc = CPU_CLASS(c);
+
+    cc->gdb_core_xml_file = "cf-core.xml";
+}
+
+#define DEFINE_M68K_CPU_TYPE_CF(model)               \
+    {                                                \
+        .name = M68K_CPU_TYPE_NAME(#model),          \
+        .instance_init = model##_cpu_initfn,         \
+        .parent = TYPE_M68K_CPU,                     \
+        .class_init = m68k_cpu_class_init_cf_core    \
+    }
+
+static void m68k_cpu_class_init_m68k_core(ObjectClass *c, void *data)
+{
+    CPUClass *cc = CPU_CLASS(c);
+
+    cc->gdb_core_xml_file = "m68k-core.xml";
+}
+
+#define DEFINE_M68K_CPU_TYPE_M68K(model)             \
+    {                                                \
+        .name = M68K_CPU_TYPE_NAME(#model),          \
+        .instance_init = model##_cpu_initfn,         \
+        .parent = TYPE_M68K_CPU,                     \
+        .class_init = m68k_cpu_class_init_m68k_core  \
     }
 
 static const TypeInfo m68k_cpus_type_infos[] = {
@@ -314,15 +336,15 @@ static const TypeInfo m68k_cpus_type_infos[] = {
         .class_size = sizeof(M68kCPUClass),
         .class_init = m68k_cpu_class_init,
     },
-    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("any", any_cpu_initfn),
+    DEFINE_M68K_CPU_TYPE_M68K(m68000),
+    DEFINE_M68K_CPU_TYPE_M68K(m68020),
+    DEFINE_M68K_CPU_TYPE_M68K(m68030),
+    DEFINE_M68K_CPU_TYPE_M68K(m68040),
+    DEFINE_M68K_CPU_TYPE_M68K(m68060),
+    DEFINE_M68K_CPU_TYPE_CF(m5206),
+    DEFINE_M68K_CPU_TYPE_CF(m5208),
+    DEFINE_M68K_CPU_TYPE_CF(cfv4e),
+    DEFINE_M68K_CPU_TYPE_CF(any),
 };
 
 DEFINE_TYPES(m68k_cpus_type_infos)
diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
new file mode 100644
index 0000000000..5b092d26de
--- /dev/null
+++ b/gdb-xml/m68k-core.xml
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2008 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.m68k.core">
+  <reg name="d0" bitsize="32"/>
+  <reg name="d1" bitsize="32"/>
+  <reg name="d2" bitsize="32"/>
+  <reg name="d3" bitsize="32"/>
+  <reg name="d4" bitsize="32"/>
+  <reg name="d5" bitsize="32"/>
+  <reg name="d6" bitsize="32"/>
+  <reg name="d7" bitsize="32"/>
+  <reg name="a0" bitsize="32" type="data_ptr"/>
+  <reg name="a1" bitsize="32" type="data_ptr"/>
+  <reg name="a2" bitsize="32" type="data_ptr"/>
+  <reg name="a3" bitsize="32" type="data_ptr"/>
+  <reg name="a4" bitsize="32" type="data_ptr"/>
+  <reg name="a5" bitsize="32" type="data_ptr"/>
+  <reg name="fp" bitsize="32" type="data_ptr"/>
+  <reg name="sp" bitsize="32" type="data_ptr"/>
+
+  <reg name="ps" bitsize="32"/>
+  <reg name="pc" bitsize="32" type="code_ptr"/>
+
+</feature>
-- 
2.20.1

Re: [PATCH v1 0/9] gdbstub/next

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200430190122.4592-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200430190122.4592-1-alex.bennee@linaro.org
Subject: [PATCH  v1 0/9] gdbstub/next
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200430190122.4592-1-alex.bennee@linaro.org -> patchew/20200430190122.4592-1-alex.bennee@linaro.org
Switched to a new branch 'test'
cbeaa01 target/m68k: fix gdb for m68xxx
0ef9fc4 tests/tcg: add a multiarch linux-user gdb test
2eaef59 tests/guest-debug: use the unix socket for linux-user tests
c0635bc gdbstub/linux-user: support debugging over a unix socket
bc7fbda gdbstub: eliminate gdbserver_fd global
9fc5b1f tests/tcg: drop inferior.was_attached() test
d5abc1a tests/tcg: better trap gdb failures
43ab101 gdbstub: Introduce gdb_get_float64() to get 64-bit float registers
b2085b3 configure: favour gdb-multiarch if we have it

=== OUTPUT BEGIN ===
1/9 Checking commit b2085b336372 (configure: favour gdb-multiarch if we have it)
2/9 Checking commit 43ab101ae057 (gdbstub: Introduce gdb_get_float64() to get 64-bit float registers)
3/9 Checking commit d5abc1a10211 (tests/tcg: better trap gdb failures)
4/9 Checking commit 9fc5b1f56fbd (tests/tcg: drop inferior.was_attached() test)
5/9 Checking commit bc7fbda9f7e4 (gdbstub: eliminate gdbserver_fd global)
ERROR: suspect code indent for conditional statements (2, 6)
#34: FILE: gdbstub.c:2965:
+  if (gdbserver_state.fd < 0) {
       return;

total: 1 errors, 0 warnings, 74 lines checked

Patch 5/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/9 Checking commit c0635bc3b0e0 (gdbstub/linux-user: support debugging over a unix socket)
ERROR: suspect code indent for conditional statements (2, 6)
#67: FILE: gdbstub.c:2966:
+  if (gdbserver_state.socket_path) {
+      unlink(gdbserver_state.socket_path);

ERROR: space required before the open parenthesis '('
#93: FILE: gdbstub.c:3088:
+    for(;;) {

total: 2 errors, 0 warnings, 220 lines checked

Patch 6/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/9 Checking commit 2eaef5943ca6 (tests/guest-debug: use the unix socket for linux-user tests)
8/9 Checking commit 0ef9fc4e47a9 (tests/tcg: add a multiarch linux-user gdb test)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#75: 
new file mode 100644

total: 0 errors, 1 warnings, 124 lines checked

Patch 8/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/9 Checking commit cbeaa014520b (target/m68k: fix gdb for m68xxx)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#51: 
new file mode 100644

total: 0 errors, 1 warnings, 105 lines checked

Patch 9/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200430190122.4592-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v1 1/9] configure: favour gdb-multiarch if we have it

[Philippe Mathieu-Daudé]

On 4/30/20 9:01 PM, Alex Bennée wrote:
> As gdb will generally be talking to "foreign" guests lets use that if
> we can. Otherwise the chances of gdb barfing are considerably higher.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   configure | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 23b5e93752..c58787100f 100755
> --- a/configure
> +++ b/configure
> @@ -303,7 +303,7 @@ libs_qga=""
>   debug_info="yes"
>   stack_protector=""
>   use_containers="yes"
> -gdb_bin=$(command -v "gdb")
> +gdb_bin=$(command -v "gdb-multiarch" || command -v "gdb")
>   
>   if test -e "$source_path/.git"
>   then
> 

This was also already reviewed:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg697748.html

There seem to be a problem in your workflow.

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

Re: [PATCH v1 5/9] gdbstub: eliminate gdbserver_fd global

[Philippe Mathieu-Daudé]

On 4/30/20 9:01 PM, Alex Bennée wrote:
> We don't really need to track this fd beyond the initial creation of
> the socket. We already know if the system has been initialised by
> virtue of the gdbserver_state so lets remove it. This makes the later
> re-factoring easier.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v1
>    - fix coding style issue
> ---
>   gdbstub.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 171e150950..b5381aa520 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -398,8 +398,6 @@ static void reset_gdbserver_state(void)
>   bool gdb_has_xml;
>   
>   #ifdef CONFIG_USER_ONLY
> -/* XXX: This is not thread safe.  Do we care?  */
> -static int gdbserver_fd = -1;
>   
>   static int get_char(void)
>   {
> @@ -2964,7 +2962,7 @@ void gdb_exit(CPUArchState *env, int code)
>         return;
>     }
>   #ifdef CONFIG_USER_ONLY
> -  if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +  if (gdbserver_state.fd < 0) {
>         return;
>     }
>   #endif
> @@ -3011,7 +3009,7 @@ gdb_handlesig(CPUState *cpu, int sig)
>       char buf[256];
>       int n;
>   
> -    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
>           return sig;
>       }
>   
> @@ -3060,7 +3058,7 @@ void gdb_signalled(CPUArchState *env, int sig)
>   {
>       char buf[4];
>   
> -    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
>           return;
>       }
>   
> @@ -3068,7 +3066,7 @@ void gdb_signalled(CPUArchState *env, int sig)
>       put_packet(buf);
>   }
>   
> -static bool gdb_accept(void)
> +static bool gdb_accept(int gdb_fd)
>   {
>       struct sockaddr_in sockaddr;
>       socklen_t len;
> @@ -3076,7 +3074,7 @@ static bool gdb_accept(void)
>   
>       for(;;) {
>           len = sizeof(sockaddr);
> -        fd = accept(gdbserver_fd, (struct sockaddr *)&sockaddr, &len);
> +        fd = accept(gdb_fd, (struct sockaddr *)&sockaddr, &len);
>           if (fd < 0 && errno != EINTR) {
>               perror("accept");
>               return false;
> @@ -3137,13 +3135,13 @@ static int gdbserver_open(int port)
>   
>   int gdbserver_start(int port)
>   {
> -    gdbserver_fd = gdbserver_open(port);
> -    if (gdbserver_fd < 0)
> +    int gdb_fd = gdbserver_open(port);
> +    if (gdb_fd < 0) {
>           return -1;
> +    }
>       /* accept connections */
> -    if (!gdb_accept()) {
> -        close(gdbserver_fd);
> -        gdbserver_fd = -1;
> +    if (!gdb_accept(gdb_fd)) {
> +        close(gdb_fd);
>           return -1;
>       }
>       return 0;
> @@ -3152,7 +3150,7 @@ int gdbserver_start(int port)
>   /* Disable gdb stub for child processes.  */
>   void gdbserver_fork(CPUState *cpu)
>   {
> -    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
>           return;
>       }
>       close(gdbserver_state.fd);
> 

This was also already reviewed:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg697750.html

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

Re: [PATCH v1 1/9] configure: favour gdb-multiarch if we have it

[Alex Bennée]

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

> On 4/30/20 9:01 PM, Alex Bennée wrote:
>> As gdb will generally be talking to "foreign" guests lets use that if
>> we can. Otherwise the chances of gdb barfing are considerably higher.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   configure | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/configure b/configure
>> index 23b5e93752..c58787100f 100755
>> --- a/configure
>> +++ b/configure
>> @@ -303,7 +303,7 @@ libs_qga=""
>>   debug_info="yes"
>>   stack_protector=""
>>   use_containers="yes"
>> -gdb_bin=$(command -v "gdb")
>> +gdb_bin=$(command -v "gdb-multiarch" || command -v "gdb")
>>     if test -e "$source_path/.git"
>>   then
>> 
>
> This was also already reviewed:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg697748.html
>
> There seem to be a problem in your workflow.

Ahh I was only calling:

  #+CALL: check-and-fix-missing-signoffs()

in my PR workflow... fixed now thanks

-- 
Alex Bennée

Re: [PATCH v1 2/9] gdbstub: Introduce gdb_get_float64() to get 64-bit float registers

[Richard Henderson]

On 4/30/20 12:01 PM, Alex Bennée wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> When converted to use GByteArray in commits 462474d760c and
> a010bdbe719, the call to stfq_p() was removed. This call
> serialize a float.
> Since we now use a GByteArray, we can not use stfq_p() directly.
> Introduce the gdb_get_float64() helper to load a float64 register.
> 
> Fixes: 462474d760c ("target/m68k: use gdb_get_reg helpers")
> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200414163853.12164-3-philmd@redhat.com>
> ---
>  include/exec/gdbstub.h          | 11 +++++++++++
>  target/m68k/helper.c            |  3 ++-
>  target/ppc/gdbstub.c            |  4 ++--
>  target/ppc/translate_init.inc.c |  2 +-
>  4 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 20e1072692..4a2b8e3089 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -134,6 +134,17 @@ static inline int gdb_get_float32(GByteArray *array, float32 val)
>  
>      return sizeof(buf);
>  }
> +
> +static inline int gdb_get_float64(GByteArray *array, float64 val)
> +{
> +    uint8_t buf[sizeof(CPU_DoubleU)];
> +
> +    stfq_p(buf, val);
> +    g_byte_array_append(array, buf, sizeof(buf));
> +
> +    return sizeof(buf);
> +}

I think we'd be better off *removing* ldfl_p/ldfq_p/stfl_p/stfq_p and
everything that specializes on them, like this.


r~

Re: [PATCH v1 5/9] gdbstub: eliminate gdbserver_fd global

[Richard Henderson]

On 4/30/20 12:01 PM, Alex Bennée wrote:
> We don't really need to track this fd beyond the initial creation of
> the socket. We already know if the system has been initialised by
> virtue of the gdbserver_state so lets remove it. This makes the later
> re-factoring easier.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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


r~

Re: [PATCH v1 6/9] gdbstub/linux-user: support debugging over a unix socket

[Richard Henderson]

On 4/30/20 12:01 PM, Alex Bennée wrote:
> @@ -814,7 +814,7 @@ int main(int argc, char **argv)
>                  exit(1);
>              }
>          } else if (!strcmp(r, "g")) {
> -            gdbstub_port = atoi(argv[optind++]);
> +            gdbstub = g_strdup(argv[optind++]);

You've got one too many strdup.

The one here in bsd-user main, and a similar one in linux-user handle_arg_gdb,
are redundant with...

> +        gdbserver_state.socket_path = g_strdup(port_or_path);

... this one, in gdbserver_start.

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


r~

Re: [PATCH v1 9/9] target/m68k: fix gdb for m68xxx

[Richard Henderson]

On 4/30/20 12:01 PM, Alex Bennée wrote:
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> Currently "cf-core.xml" is sent to GDB when using any m68k flavor.  Thing is
> it uses the "org.gnu.gdb.coldfire.core" feature name and gdb 8.3 then expects
> a coldfire FPU instead of the default m68881 FPU.
> 
> This is not OK because the m68881 floats registers are 96 bits wide so it
> crashes GDB with the following error message:
> 
> (gdb) target remote localhost:7960
> Remote debugging using localhost:7960
> warning: Register "fp0" has an unsupported size (96 bits)
> warning: Register "fp1" has an unsupported size (96 bits)
> ...
> Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
>   00000000000[...]0000
> 
> With this patch: qemu-system-m68k -M none -cpu m68020 -s -S
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> 0x00000000 in ?? ()
> (gdb) p $fp0
> $1 = nan(0xffffffffffffffff)
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <1588094279-17913-3-git-send-email-frederic.konrad@adacore.com>
> ---


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


r~

Re: [PATCH v1 7/9] tests/guest-debug: use the unix socket for linux-user tests

[Richard Henderson]

On 4/30/20 12:01 PM, Alex Bennée wrote:
> +    if "system" in args.qemu:
> +        gdb_cmd += " -ex 'target remote localhost:1234'"
> +    else:
> +        gdb_cmd += " -ex 'target remote %s'" % (socket_name)

Why should not system testing be moved to sockets?
Surely that has the same problem in the end.


r~

Re: [PATCH v1 9/9] target/m68k: fix gdb for m68xxx

[Laurent Vivier]

Le 30/04/2020 à 21:01, Alex Bennée a écrit :
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> Currently "cf-core.xml" is sent to GDB when using any m68k flavor.  Thing is
> it uses the "org.gnu.gdb.coldfire.core" feature name and gdb 8.3 then expects
> a coldfire FPU instead of the default m68881 FPU.
> 
> This is not OK because the m68881 floats registers are 96 bits wide so it
> crashes GDB with the following error message:
> 
> (gdb) target remote localhost:7960
> Remote debugging using localhost:7960
> warning: Register "fp0" has an unsupported size (96 bits)
> warning: Register "fp1" has an unsupported size (96 bits)
> ...
> Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
>   00000000000[...]0000
> 
> With this patch: qemu-system-m68k -M none -cpu m68020 -s -S
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> 0x00000000 in ?? ()
> (gdb) p $fp0
> $1 = nan(0xffffffffffffffff)
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <1588094279-17913-3-git-send-email-frederic.konrad@adacore.com>
> ---
>  configure             |  2 +-
>  target/m68k/cpu.c     | 52 ++++++++++++++++++++++++++++++-------------
>  gdb-xml/m68k-core.xml | 29 ++++++++++++++++++++++++
>  3 files changed, 67 insertions(+), 16 deletions(-)
>  create mode 100644 gdb-xml/m68k-core.xml
> 
> diff --git a/configure b/configure
> index c58787100f..0d69c360c0 100755
> --- a/configure
> +++ b/configure
> @@ -7825,7 +7825,7 @@ case "$target_name" in
>    ;;
>    m68k)
>      bflt="yes"
> -    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
> +    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
>      TARGET_SYSTBL_ABI=common
>    ;;
>    microblaze|microblazeel)
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 9445fcd6df..72c545149e 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -292,16 +292,38 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>      cc->tcg_initialize = m68k_tcg_init;
>  
>      cc->gdb_num_core_regs = 18;
> -    cc->gdb_core_xml_file = "cf-core.xml";
>  
>      dc->vmsd = &vmstate_m68k_cpu;
>  }
>  
> -#define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
> -    {                                           \
> -        .name = M68K_CPU_TYPE_NAME(cpu_model),  \
> -        .instance_init = initfn,                \
> -        .parent = TYPE_M68K_CPU,                \
> +static void m68k_cpu_class_init_cf_core(ObjectClass *c, void *data)
> +{
> +    CPUClass *cc = CPU_CLASS(c);
> +
> +    cc->gdb_core_xml_file = "cf-core.xml";
> +}
> +
> +#define DEFINE_M68K_CPU_TYPE_CF(model)               \
> +    {                                                \
> +        .name = M68K_CPU_TYPE_NAME(#model),          \
> +        .instance_init = model##_cpu_initfn,         \
> +        .parent = TYPE_M68K_CPU,                     \
> +        .class_init = m68k_cpu_class_init_cf_core    \
> +    }
> +
> +static void m68k_cpu_class_init_m68k_core(ObjectClass *c, void *data)
> +{
> +    CPUClass *cc = CPU_CLASS(c);
> +
> +    cc->gdb_core_xml_file = "m68k-core.xml";
> +}
> +
> +#define DEFINE_M68K_CPU_TYPE_M68K(model)             \
> +    {                                                \
> +        .name = M68K_CPU_TYPE_NAME(#model),          \
> +        .instance_init = model##_cpu_initfn,         \
> +        .parent = TYPE_M68K_CPU,                     \
> +        .class_init = m68k_cpu_class_init_m68k_core  \
>      }
>  
>  static const TypeInfo m68k_cpus_type_infos[] = {
> @@ -314,15 +336,15 @@ static const TypeInfo m68k_cpus_type_infos[] = {
>          .class_size = sizeof(M68kCPUClass),
>          .class_init = m68k_cpu_class_init,
>      },
> -    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("any", any_cpu_initfn),
> +    DEFINE_M68K_CPU_TYPE_M68K(m68000),
> +    DEFINE_M68K_CPU_TYPE_M68K(m68020),
> +    DEFINE_M68K_CPU_TYPE_M68K(m68030),
> +    DEFINE_M68K_CPU_TYPE_M68K(m68040),
> +    DEFINE_M68K_CPU_TYPE_M68K(m68060),
> +    DEFINE_M68K_CPU_TYPE_CF(m5206),
> +    DEFINE_M68K_CPU_TYPE_CF(m5208),
> +    DEFINE_M68K_CPU_TYPE_CF(cfv4e),
> +    DEFINE_M68K_CPU_TYPE_CF(any),
>  };
>  
>  DEFINE_TYPES(m68k_cpus_type_infos)
> diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
> new file mode 100644
> index 0000000000..5b092d26de
> --- /dev/null
> +++ b/gdb-xml/m68k-core.xml
> @@ -0,0 +1,29 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2008 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.m68k.core">
> +  <reg name="d0" bitsize="32"/>
> +  <reg name="d1" bitsize="32"/>
> +  <reg name="d2" bitsize="32"/>
> +  <reg name="d3" bitsize="32"/>
> +  <reg name="d4" bitsize="32"/>
> +  <reg name="d5" bitsize="32"/>
> +  <reg name="d6" bitsize="32"/>
> +  <reg name="d7" bitsize="32"/>
> +  <reg name="a0" bitsize="32" type="data_ptr"/>
> +  <reg name="a1" bitsize="32" type="data_ptr"/>
> +  <reg name="a2" bitsize="32" type="data_ptr"/>
> +  <reg name="a3" bitsize="32" type="data_ptr"/>
> +  <reg name="a4" bitsize="32" type="data_ptr"/>
> +  <reg name="a5" bitsize="32" type="data_ptr"/>
> +  <reg name="fp" bitsize="32" type="data_ptr"/>
> +  <reg name="sp" bitsize="32" type="data_ptr"/>
> +
> +  <reg name="ps" bitsize="32"/>
> +  <reg name="pc" bitsize="32" type="code_ptr"/>
> +
> +</feature>
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v1 8/9] tests/tcg: add a multiarch linux-user gdb test

[Richard Henderson]

On 4/30/20 12:01 PM, Alex Bennée wrote:
> When the gdbstub code was converted to the new API we missed a few
> snafus in the various guests. Add a simple gdb test script which can
> be used on all our linux-user guests to check for obvious failures.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - use EXTRA_RUNS to queue the tests so as not to break plugins
> ---

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


r~

Re: [PATCH v1 7/9] tests/guest-debug: use the unix socket for linux-user tests

[Alex Bennée]

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

> On 4/30/20 12:01 PM, Alex Bennée wrote:
>> +    if "system" in args.qemu:
>> +        gdb_cmd += " -ex 'target remote localhost:1234'"
>> +    else:
>> +        gdb_cmd += " -ex 'target remote %s'" % (socket_name)
>
> Why should not system testing be moved to sockets?
> Surely that has the same problem in the end.

Sure - I can cook up a patch for that. I hadn't bothered because
currently we don't run any system level debug tests for the gdbstub
test.

-- 
Alex Bennée