qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] some testing and gdbstub fixes
@ 2023-08-10 15:36 Alex Bennée
  2023-08-10 15:36 ` [PATCH 1/8] gitlab: enable ccache for many build jobs Alex Bennée
                   ` (8 more replies)
  0 siblings, 9 replies; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta

This is mostly gdbstub focused but I cleaned up some bits while I was
in the testing makefiles. This is mostly to make the "check-tcg"
output as clean as possible without ugly line wraps. I tried to
eliminate the gdbstub info() output but sadly this is harder than
expected.

I've tweaked the gdbstub handling for Ctrl-c packets as suggested by
Matheus. While I was there I also noticed we were being a bit precious
about gdb sending preemptive ACKS so I fixed that as well.

I don't know if this is all late 8.1-rc material but its fairly simple
testing updates and the ccache stuff from Daniel should help as well.

Alex Bennée (7):
  tests/docker: cleanup non-verbose output
  tests/tcg: remove quoting for info output
  tests: remove test-gdbstub.py
  tests/tcg: clean-up gdb confirm/pagination settings
  tests/tcg: ensure system-mode gdb tests start stopped
  gdbstub: more fixes for client Ctrl-C handling
  gdbstub: don't complain about preemptive ACK chars

Daniel P. Berrangé (1):
  gitlab: enable ccache for many build jobs

 docs/devel/ci-jobs.rst.inc                    |   7 +
 gdbstub/gdbstub.c                             |  10 +-
 .gitlab-ci.d/buildtest-template.yml           |  11 ++
 .gitlab-ci.d/crossbuild-template.yml          |  26 +++
 .gitlab-ci.d/windows.yml                      |  13 +-
 gdbstub/trace-events                          |   1 +
 tests/docker/Makefile.include                 |   6 +-
 .../dockerfiles/debian-hexagon-cross.docker   |   9 +-
 tests/guest-debug/run-test.py                 |  11 +-
 tests/guest-debug/test-gdbstub.py             | 177 ------------------
 tests/tcg/aarch64/Makefile.target             |   2 +-
 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py   |   3 -
 tests/tcg/aarch64/gdbstub/test-sve.py         |   3 -
 tests/tcg/multiarch/gdbstub/memory.py         |   3 -
 tests/tcg/multiarch/gdbstub/sha1.py           |   4 -
 .../multiarch/gdbstub/test-proc-mappings.py   |   4 -
 .../multiarch/gdbstub/test-qxfer-auxv-read.py |   4 -
 .../gdbstub/test-thread-breakpoint.py         |   4 -
 .../multiarch/system/Makefile.softmmu-target  |   4 +-
 tests/tcg/s390x/gdbstub/test-signals-s390x.py |   4 -
 tests/tcg/s390x/gdbstub/test-svc.py           |   4 -
 21 files changed, 83 insertions(+), 227 deletions(-)
 delete mode 100644 tests/guest-debug/test-gdbstub.py

-- 
2.39.2



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/8] gitlab: enable ccache for many build jobs
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-10 15:36 ` [PATCH 2/8] tests/docker: cleanup non-verbose output Alex Bennée
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta, Daniel P. Berrangé

From: Daniel P. Berrangé <berrange@redhat.com>

The `ccache` tool can be very effective at reducing compilation times
when re-running pipelines with only minor changes each time. For example
a fresh 'build-system-fedora' job will typically take 20 minutes on the
gitlab.com shared runners. With ccache this is reduced to as little as
6 minutes.

Normally meson would auto-detect existance of ccache in $PATH and use
it automatically, but the way we wrap meson from configure breaks this,
as we're passing in an config file with explicitly set compiler paths.
Thus we need to add $CCACHE_WRAPPERSPATH to the front of $PATH. For
unknown reasons if doing this in msys though, gcc becomes unable to
invoke 'cc1' when run from meson. For msys we thus set CC='ccache gcc'
before invoking 'configure' instead.

A second problem with msys is that cache misses are incredibly
expensive, so enabling ccache massively slows down the build when
the cache isn't well populated. This is suspected to be a result of
the cost of spawning processes under the msys architecture. To deal
with this we set CCACHE_DEPEND=1 which enables ccache's 'depend_only'
strategy. This avoids extra spawning of the pre-processor during
cache misses, with the downside that is it less likely ccache will
find a cache hit after semantically benign compiler flag changes.
This is the lesser of two evils, as otherwise we can't use ccache
at all under msys and remain inside the job time limit.

If people are finding ccache to hurt their pipelines, it can be
disabled by setting the 'CCACHE_DISABLE=1' env variable against
their gitlab fork CI settings.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20230804111054.281802-2-berrange@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/ci-jobs.rst.inc                    |  7 +++++
 .gitlab-ci.d/buildtest-template.yml           | 11 ++++++++
 .gitlab-ci.d/crossbuild-template.yml          | 26 +++++++++++++++++++
 .gitlab-ci.d/windows.yml                      | 13 ++++++++--
 .../dockerfiles/debian-hexagon-cross.docker   |  9 ++++++-
 5 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/docs/devel/ci-jobs.rst.inc b/docs/devel/ci-jobs.rst.inc
index 3f6802d51e..4c39cdb2d9 100644
--- a/docs/devel/ci-jobs.rst.inc
+++ b/docs/devel/ci-jobs.rst.inc
@@ -188,3 +188,10 @@ If you've got access to a CentOS Stream 8 x86_64 host that can be
 used as a gitlab-CI runner, you can set this variable to enable the
 tests that require this kind of host. The runner should be tagged with
 both "centos_stream_8" and "x86_64".
+
+CCACHE_DISABLE
+~~~~~~~~~~~~~~
+The jobs are configured to use "ccache" by default since this typically
+reduces compilation time, at the cost of increased storage. If the
+use of "ccache" is suspected to be hurting the overall job execution
+time, setting the "CCACHE_DISABLE=1" env variable to disable it.
diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml
index f3e39b7eb1..4fbfeb6667 100644
--- a/.gitlab-ci.d/buildtest-template.yml
+++ b/.gitlab-ci.d/buildtest-template.yml
@@ -2,11 +2,21 @@
   extends: .base_job_template
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:$QEMU_CI_CONTAINER_TAG
+  cache:
+    paths:
+      - ccache
+    key: "$CI_JOB_NAME"
+    when: always
   before_script:
     - JOBS=$(expr $(nproc) + 1)
   script:
+    - export CCACHE_BASEDIR="$(pwd)"
+    - export CCACHE_DIR="$CCACHE_BASEDIR/ccache"
+    - export CCACHE_MAXSIZE="500M"
+    - export PATH="$CCACHE_WRAPPERSDIR:$PATH"
     - mkdir build
     - cd build
+    - ccache --zero-stats
     - ../configure --enable-werror --disable-docs --enable-fdt=system
           ${TARGETS:+--target-list="$TARGETS"}
           $CONFIGURE_ARGS ||
@@ -20,6 +30,7 @@
       then
         make -j"$JOBS" $MAKE_CHECK_ARGS ;
       fi
+    - ccache --show-stats
 
 # We jump some hoops in common_test_job_template to avoid
 # rebuilding all the object files we skip in the artifacts
diff --git a/.gitlab-ci.d/crossbuild-template.yml b/.gitlab-ci.d/crossbuild-template.yml
index d97611053b..3e5f4d9cd8 100644
--- a/.gitlab-ci.d/crossbuild-template.yml
+++ b/.gitlab-ci.d/crossbuild-template.yml
@@ -2,10 +2,20 @@
   extends: .base_job_template
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:$QEMU_CI_CONTAINER_TAG
+  cache:
+    paths:
+      - ccache
+    key: "$CI_JOB_NAME"
+    when: always
   timeout: 80m
   script:
+    - export CCACHE_BASEDIR="$(pwd)"
+    - export CCACHE_DIR="$CCACHE_BASEDIR/ccache"
+    - export CCACHE_MAXSIZE="500M"
+    - export PATH="$CCACHE_WRAPPERSDIR:$PATH"
     - mkdir build
     - cd build
+    - ccache --zero-stats
     - ../configure --enable-werror --disable-docs --enable-fdt=system
         --disable-user $QEMU_CONFIGURE_OPTS $EXTRA_CONFIGURE_OPTS
         --target-list-exclude="arm-softmmu cris-softmmu
@@ -18,6 +28,7 @@
       version="$(git describe --match v[0-9]* 2>/dev/null || git rev-parse --short HEAD)";
       mv -v qemu-setup*.exe qemu-setup-${version}.exe;
       fi
+    - ccache --show-stats
 
 # Job to cross-build specific accelerators.
 #
@@ -29,7 +40,15 @@
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:$QEMU_CI_CONTAINER_TAG
   timeout: 30m
+  cache:
+    paths:
+      - ccache/
+    key: "$CI_JOB_NAME"
   script:
+    - export CCACHE_BASEDIR="$(pwd)"
+    - export CCACHE_DIR="$CCACHE_BASEDIR/ccache"
+    - export CCACHE_MAXSIZE="500M"
+    - export PATH="$CCACHE_WRAPPERSDIR:$PATH"
     - mkdir build
     - cd build
     - ../configure --enable-werror --disable-docs $QEMU_CONFIGURE_OPTS
@@ -40,7 +59,14 @@
   extends: .base_job_template
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:$QEMU_CI_CONTAINER_TAG
+  cache:
+    paths:
+      - ccache/
+    key: "$CI_JOB_NAME"
   script:
+    - export CCACHE_BASEDIR="$(pwd)"
+    - export CCACHE_DIR="$CCACHE_BASEDIR/ccache"
+    - export CCACHE_MAXSIZE="500M"
     - mkdir build
     - cd build
     - ../configure --enable-werror --disable-docs $QEMU_CONFIGURE_OPTS
diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index cd7622a761..12a987cd71 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -5,13 +5,14 @@
   - windows
   - windows-1809
   cache:
-    key: "${CI_JOB_NAME}-cache"
+    key: "$CI_JOB_NAME"
     paths:
       - msys64/var/cache
+      - ccache
     when: always
   needs: []
   stage: build
-  timeout: 80m
+  timeout: 100m
   variables:
     # This feature doesn't (currently) work with PowerShell, it stops
     # the echo'ing of commands being run and doesn't show any timing
@@ -72,6 +73,7 @@
       bison diffutils flex
       git grep make sed
       $MINGW_TARGET-capstone
+      $MINGW_TARGET-ccache
       $MINGW_TARGET-curl
       $MINGW_TARGET-cyrus-sasl
       $MINGW_TARGET-dtc
@@ -101,11 +103,18 @@
   - Write-Output "Running build at $(Get-Date -Format u)"
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
+  - $env:CCACHE_BASEDIR = "$env:CI_PROJECT_DIR"
+  - $env:CCACHE_DIR = "$env:CCACHE_BASEDIR/ccache"
+  - $env:CCACHE_MAXSIZE = "500M"
+  - $env:CCACHE_DEPEND = 1 # cache misses are too expensive with preprocessor mode
+  - $env:CC = "ccache gcc"
   - mkdir build
   - cd build
+  - ..\msys64\usr\bin\bash -lc "ccache --zero-stats"
   - ..\msys64\usr\bin\bash -lc "../configure --enable-fdt=system $CONFIGURE_ARGS"
   - ..\msys64\usr\bin\bash -lc "make"
   - ..\msys64\usr\bin\bash -lc "make check MTESTARGS='$TEST_ARGS' || { cat meson-logs/testlog.txt; exit 1; } ;"
+  - ..\msys64\usr\bin\bash -lc "ccache --show-stats"
   - Write-Output "Finished build at $(Get-Date -Format u)"
 
 msys2-64bit:
diff --git a/tests/docker/dockerfiles/debian-hexagon-cross.docker b/tests/docker/dockerfiles/debian-hexagon-cross.docker
index c2cfb6a5d0..578269766c 100644
--- a/tests/docker/dockerfiles/debian-hexagon-cross.docker
+++ b/tests/docker/dockerfiles/debian-hexagon-cross.docker
@@ -15,6 +15,7 @@ RUN apt-get update && \
 # Install common build utilities
     apt-get install -y --no-install-recommends \
         curl \
+        ccache \
         xz-utils \
         ca-certificates \
         bison \
@@ -24,13 +25,19 @@ RUN apt-get update && \
         python3-venv && \
 # Install QEMU build deps for use in CI
     DEBIAN_FRONTEND=noninteractive eatmydata \
-    apt build-dep -yy --arch-only qemu
+    apt build-dep -yy --arch-only qemu && \
+    mkdir -p /usr/libexec/ccache-wrappers && \
+    ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/c++ && \
+    ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/cc && \
+    ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/g++ && \
+    ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/gcc
 
 
 ENV TOOLCHAIN_INSTALL /opt
 ENV TOOLCHAIN_RELEASE 16.0.0
 ENV TOOLCHAIN_BASENAME "clang+llvm-${TOOLCHAIN_RELEASE}-cross-hexagon-unknown-linux-musl"
 ENV TOOLCHAIN_URL https://codelinaro.jfrog.io/artifactory/codelinaro-toolchain-for-hexagon/v${TOOLCHAIN_RELEASE}/${TOOLCHAIN_BASENAME}.tar.xz
+ENV CCACHE_WRAPPERSDIR "/usr/libexec/ccache-wrappers"
 
 RUN curl -#SL "$TOOLCHAIN_URL" | tar -xJC "$TOOLCHAIN_INSTALL"
 ENV PATH $PATH:${TOOLCHAIN_INSTALL}/${TOOLCHAIN_BASENAME}/x86_64-linux-gnu/bin
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 2/8] tests/docker: cleanup non-verbose output
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
  2023-08-10 15:36 ` [PATCH 1/8] gitlab: enable ccache for many build jobs Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-14  7:59   ` Thomas Huth
  2023-08-10 15:36 ` [PATCH 3/8] tests/tcg: remove quoting for info output Alex Bennée
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta

Even with --quiet docker will spam the sha256 to the console. Avoid
this by redirecting stdout. While we are at it fix the name we echo
which was broken during 0b1a649047 (tests/docker: use direct RUNC call
to build containers).

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

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 142e8605ee..dfabafab92 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -46,9 +46,9 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
 		--build-arg BUILDKIT_INLINE_CACHE=1 	\
 		$(if $(NOUSER),,			\
 			--build-arg USER=$(USER)	\
-			--build-arg UID=$(UID))	\
-		-t qemu/$* - < $<, 			\
-		"BUILD", $1)
+			--build-arg UID=$(UID))		\
+		-t qemu/$* - < $< $(if $V,,> /dev/null),\
+		"BUILD", $*)
 
 # Special rule for debootstraped binfmt linux-user images
 docker-binfmt-image-debian-%: $(DOCKER_FILES_DIR)/debian-bootstrap.docker
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 3/8] tests/tcg: remove quoting for info output
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
  2023-08-10 15:36 ` [PATCH 1/8] gitlab: enable ccache for many build jobs Alex Bennée
  2023-08-10 15:36 ` [PATCH 2/8] tests/docker: cleanup non-verbose output Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-10 15:36 ` [PATCH 4/8] tests: remove test-gdbstub.py Alex Bennée
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta

This avoids ugly multi-line wrapping for the test on non V=1 builds.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/Makefile.target                  | 2 +-
 tests/tcg/multiarch/system/Makefile.softmmu-target | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 617f821613..75347ebe70 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -14,7 +14,7 @@ AARCH64_TESTS=fcvt pcalign-a64
 fcvt: LDFLAGS+=-lm
 
 run-fcvt: fcvt
-	$(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)")
+	$(call run-test,$<,$(QEMU) $<)
 	$(call diff-out,$<,$(AARCH64_SRC)/fcvt.ref)
 
 config-cc.mak: Makefile
diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target b/tests/tcg/multiarch/system/Makefile.softmmu-target
index 7ba9053375..a051d689d7 100644
--- a/tests/tcg/multiarch/system/Makefile.softmmu-target
+++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
@@ -37,10 +37,10 @@ run-gdbstub-untimely-packet: hello
 		--qemu $(QEMU) \
 		--bin $< --qargs \
 		"-monitor none -display none -chardev file$(COMMA)path=untimely-packet.out$(COMMA)id=output $(QEMU_OPTS)", \
-	"softmmu gdbstub untimely packets")
+	softmmu gdbstub untimely packets)
 	$(call quiet-command, \
 		(! grep -Fq 'Packet instead of Ack, ignoring it' untimely-packet.gdb.err), \
-		"GREP", "file  untimely-packet.gdb.err")
+		"GREP", file untimely-packet.gdb.err)
 else
 run-gdbstub-%:
 	$(call skip-test, "gdbstub test $*", "no guest arch support")
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 4/8] tests: remove test-gdbstub.py
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
                   ` (2 preceding siblings ...)
  2023-08-10 15:36 ` [PATCH 3/8] tests/tcg: remove quoting for info output Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-10 17:10   ` Richard Henderson
  2023-08-10 15:36 ` [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings Alex Bennée
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta

This isn't directly called by our CI and because it doesn't run via
our run-test.py script does things slightly differently. Lets remove
it as we have plenty of working in-tree tests now for various aspects
of gdbstub.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/guest-debug/test-gdbstub.py | 177 ------------------------------
 1 file changed, 177 deletions(-)
 delete mode 100644 tests/guest-debug/test-gdbstub.py

diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
deleted file mode 100644
index 98a5df4d42..0000000000
--- a/tests/guest-debug/test-gdbstub.py
+++ /dev/null
@@ -1,177 +0,0 @@
-#
-# This script needs to be run on startup
-# qemu -kernel ${KERNEL} -s -S
-# and then:
-# gdb ${KERNEL}.vmlinux -x ${QEMU_SRC}/tests/guest-debug/test-gdbstub.py
-
-import gdb
-
-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_step():
-    "Step an instruction, check it moved."
-    start_pc = gdb.parse_and_eval('$pc')
-    gdb.execute("si")
-    end_pc = gdb.parse_and_eval('$pc')
-
-    return not (start_pc == end_pc)
-
-
-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')
-    print ("%s == %s %d" % (end_pc, sym.value(), bp.hit_count))
-    bp.delete()
-
-    # can we test we hit bp?
-    return end_pc == sym.value()
-
-
-# We need to do hbreak manually as the python interface doesn't export it
-def check_hbreak(sym_name):
-    "Setup hardware breakpoint, continue and check we stopped."
-    sym, ok = gdb.lookup_symbol(sym_name)
-    gdb.execute("hbreak %s" % (sym_name))
-    gdb.execute("c")
-
-    # hopefully we came back
-    end_pc = gdb.parse_and_eval('$pc')
-    print ("%s == %s" % (end_pc, sym.value()))
-
-    if end_pc == sym.value():
-        gdb.execute("d 1")
-        return True
-    else:
-        return False
-
-
-class WatchPoint(gdb.Breakpoint):
-
-    def get_wpstr(self, sym_name):
-        "Setup sym and wp_str for given symbol."
-        self.sym, ok = gdb.lookup_symbol(sym_name)
-        wp_addr = gdb.parse_and_eval(sym_name).address
-        self.wp_str = '*(%(type)s)(&%(address)s)' % dict(
-            type = wp_addr.type, address = sym_name)
-
-        return(self.wp_str)
-
-    def __init__(self, sym_name, type):
-        wp_str = self.get_wpstr(sym_name)
-        super(WatchPoint, self).__init__(wp_str, gdb.BP_WATCHPOINT, type)
-
-    def stop(self):
-        end_pc = gdb.parse_and_eval('$pc')
-        print ("HIT WP @ %s" % (end_pc))
-        return True
-
-
-def do_one_watch(sym, wtype, text):
-
-    wp = WatchPoint(sym, wtype)
-    gdb.execute("c")
-    report_str = "%s for %s (%s)" % (text, sym, wp.sym.value())
-
-    if wp.hit_count > 0:
-        report(True, report_str)
-        wp.delete()
-    else:
-        report(False, report_str)
-
-
-def check_watches(sym_name):
-    "Watch a symbol for any access."
-
-    # Should hit for any read
-    do_one_watch(sym_name, gdb.WP_ACCESS, "awatch")
-
-    # Again should hit for reads
-    do_one_watch(sym_name, gdb.WP_READ, "rwatch")
-
-    # Finally when it is written
-    do_one_watch(sym_name, gdb.WP_WRITE, "watch")
-
-
-class CatchBreakpoint(gdb.Breakpoint):
-    def __init__(self, sym_name):
-        super(CatchBreakpoint, self).__init__(sym_name)
-        self.sym, ok = gdb.lookup_symbol(sym_name)
-
-    def stop(self):
-        end_pc = gdb.parse_and_eval('$pc')
-        print ("CB: %s == %s" % (end_pc, self.sym.value()))
-        if end_pc == self.sym.value():
-            report(False, "Hit final catchpoint")
-
-
-def run_test():
-    "Run through the tests one by one"
-
-    print ("Checking we can step the first few instructions")
-    step_ok = 0
-    for i in range(3):
-        if check_step():
-            step_ok += 1
-
-    report(step_ok == 3, "single step in boot code")
-
-    print ("Checking HW breakpoint works")
-    break_ok = check_hbreak("kernel_init")
-    report(break_ok, "hbreak @ kernel_init")
-
-    # Can't set this up until we are in the kernel proper
-    # if we make it to run_init_process we've over-run and
-    # one of the tests failed
-    print ("Setup catch-all for run_init_process")
-    cbp = CatchBreakpoint("run_init_process")
-    cpb2 = CatchBreakpoint("try_to_run_init_process")
-
-    print ("Checking Normal breakpoint works")
-    break_ok = check_break("wait_for_completion")
-    report(break_ok, "break @ wait_for_completion")
-
-    print ("Checking watchpoint works")
-    check_watches("system_state")
-
-#
-# This runs as the script it sourced (via -x)
-#
-
-try:
-    print ("Connecting to remote")
-    gdb.execute("target remote localhost:1234")
-
-    # These are not very useful in scripts
-    gdb.execute("set pagination off")
-    gdb.execute("set confirm off")
-
-    # Run the actual tests
-    run_test()
-
-except:
-    print ("GDB Exception: %s" % (sys.exc_info()[0]))
-    failcount += 1
-    import code
-    code.InteractiveConsole(locals=globals()).interact()
-    raise
-
-# Finally kill the inferior and exit gdb with a count of failures
-gdb.execute("kill")
-exit(failcount)
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
                   ` (3 preceding siblings ...)
  2023-08-10 15:36 ` [PATCH 4/8] tests: remove test-gdbstub.py Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-10 15:47   ` Philippe Mathieu-Daudé
  2023-08-10 17:08   ` Richard Henderson
  2023-08-10 15:36 ` [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped Alex Bennée
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta

We can do this all in the run-test.py script so remove the extraneous
bits from the individual tests which got copied from the original
non-CI gdb tests.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/guest-debug/run-test.py                         | 2 ++
 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py           | 3 ---
 tests/tcg/aarch64/gdbstub/test-sve.py                 | 3 ---
 tests/tcg/multiarch/gdbstub/memory.py                 | 3 ---
 tests/tcg/multiarch/gdbstub/sha1.py                   | 4 ----
 tests/tcg/multiarch/gdbstub/test-proc-mappings.py     | 4 ----
 tests/tcg/multiarch/gdbstub/test-qxfer-auxv-read.py   | 4 ----
 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py | 4 ----
 tests/tcg/s390x/gdbstub/test-signals-s390x.py         | 4 ----
 tests/tcg/s390x/gdbstub/test-svc.py                   | 4 ----
 10 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index de6106a5e5..c0d0075e2e 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -86,6 +86,8 @@ def log(output, msg):
         gdb_cmd += " %s" % (args.gdb_args)
     # run quietly and ignore .gdbinit
     gdb_cmd += " -q -n -batch"
+    # disable pagination
+    gdb_cmd += " -ex 'set pagination off'"
     # disable prompts in case of crash
     gdb_cmd += " -ex 'set confirm off'"
     # connect to remote
diff --git a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
index b9ef169c1a..ee8d467e59 100644
--- a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
+++ b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
@@ -76,9 +76,6 @@ def run_test():
     exit(0)
 
 try:
-    # These are not very useful in scripts
-    gdb.execute("set pagination off")
-
     # Run the actual tests
     run_test()
 except:
diff --git a/tests/tcg/aarch64/gdbstub/test-sve.py b/tests/tcg/aarch64/gdbstub/test-sve.py
index ef57c7412c..afd8ece98d 100644
--- a/tests/tcg/aarch64/gdbstub/test-sve.py
+++ b/tests/tcg/aarch64/gdbstub/test-sve.py
@@ -66,9 +66,6 @@ def run_test():
     exit(0)
 
 try:
-    # These are not very useful in scripts
-    gdb.execute("set pagination off")
-
     # Run the actual tests
     run_test()
 except:
diff --git a/tests/tcg/multiarch/gdbstub/memory.py b/tests/tcg/multiarch/gdbstub/memory.py
index 67864ad902..dd25e72281 100644
--- a/tests/tcg/multiarch/gdbstub/memory.py
+++ b/tests/tcg/multiarch/gdbstub/memory.py
@@ -115,9 +115,6 @@ def run_test():
     exit(0)
 
 try:
-    # These are not very useful in scripts
-    gdb.execute("set pagination off")
-
     # Run the actual tests
     run_test()
 except (gdb.error):
diff --git a/tests/tcg/multiarch/gdbstub/sha1.py b/tests/tcg/multiarch/gdbstub/sha1.py
index 423b720e6d..416728415f 100644
--- a/tests/tcg/multiarch/gdbstub/sha1.py
+++ b/tests/tcg/multiarch/gdbstub/sha1.py
@@ -73,10 +73,6 @@ def run_test():
     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):
diff --git a/tests/tcg/multiarch/gdbstub/test-proc-mappings.py b/tests/tcg/multiarch/gdbstub/test-proc-mappings.py
index 5e3e5a2fb7..04ec61d219 100644
--- a/tests/tcg/multiarch/gdbstub/test-proc-mappings.py
+++ b/tests/tcg/multiarch/gdbstub/test-proc-mappings.py
@@ -51,10 +51,6 @@ def main():
         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:
diff --git a/tests/tcg/multiarch/gdbstub/test-qxfer-auxv-read.py b/tests/tcg/multiarch/gdbstub/test-qxfer-auxv-read.py
index d91e8fdf19..926fa962b7 100644
--- a/tests/tcg/multiarch/gdbstub/test-qxfer-auxv-read.py
+++ b/tests/tcg/multiarch/gdbstub/test-qxfer-auxv-read.py
@@ -42,10 +42,6 @@ def run_test():
     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):
diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py
index 798d508bc7..e57d2a8db8 100644
--- a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py
+++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py
@@ -45,10 +45,6 @@ def run_test():
     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):
diff --git a/tests/tcg/s390x/gdbstub/test-signals-s390x.py b/tests/tcg/s390x/gdbstub/test-signals-s390x.py
index 80a284b475..ca2bbc0b03 100644
--- a/tests/tcg/s390x/gdbstub/test-signals-s390x.py
+++ b/tests/tcg/s390x/gdbstub/test-signals-s390x.py
@@ -61,10 +61,6 @@ def run_test():
     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):
diff --git a/tests/tcg/s390x/gdbstub/test-svc.py b/tests/tcg/s390x/gdbstub/test-svc.py
index 18fad3f163..804705fede 100644
--- a/tests/tcg/s390x/gdbstub/test-svc.py
+++ b/tests/tcg/s390x/gdbstub/test-svc.py
@@ -49,10 +49,6 @@ def main():
         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:
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
                   ` (4 preceding siblings ...)
  2023-08-10 15:36 ` [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-10 15:48   ` Philippe Mathieu-Daudé
  2023-08-10 17:08   ` Richard Henderson
  2023-08-10 15:36 ` [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling Alex Bennée
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta

Without -S we run into potential races with tests starting before the
gdbstub attaches. We don't need to worry about user-mode as enabling
the gdbstub implies we wait for the initial connection.

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

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index c0d0075e2e..b13b27d4b1 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -69,13 +69,10 @@ def log(output, msg):
 
     # Launch QEMU with binary
     if "system" in args.qemu:
-        cmd = "%s %s %s -gdb unix:path=%s,server=on" % (args.qemu,
-                                                        args.qargs,
-                                                        args.binary,
-                                                        socket_name)
+        cmd = f'{args.qemu} {args.qargs} {args.binary}' \
+            f' -S -gdb unix:path={socket_name},server=on'
     else:
-        cmd = "%s %s -g %s %s" % (args.qemu, args.qargs, socket_name,
-                                  args.binary)
+        cmd = f'{args.qemu} {args.qargs} -g {socket_name} {args.binary}'
 
     log(output, "QEMU CMD: %s" % (cmd))
     inferior = subprocess.Popen(shlex.split(cmd))
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
                   ` (5 preceding siblings ...)
  2023-08-10 15:36 ` [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-10 15:56   ` Philippe Mathieu-Daudé
  2023-08-10 16:26   ` Richard Henderson
  2023-08-10 15:36 ` [PATCH 8/8] gdbstub: don't complain about preemptive ACK chars Alex Bennée
  2023-08-10 15:58 ` [PATCH 0/8] some testing and gdbstub fixes Peter Maydell
  8 siblings, 2 replies; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta, Matheus Tavares Bernardino

The original fix caused problems with spurious characters on other
system emulation. So:

  - instead of spamming output make the warning a trace point
  - ensure we only allow a stop reply if it was 0x3

Suggested-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <456ed3318421dd7946bdfb5ceda7e05332da368c.1690910333.git.quic_mathbern@quicinc.com>
---
 gdbstub/gdbstub.c    | 5 +++--
 gdbstub/trace-events | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index e74ecc78cc..20b6fe03fb 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -2059,9 +2059,10 @@ void gdb_read_byte(uint8_t ch)
          * here, but it does expect a stop reply.
          */
         if (ch != 0x03) {
-            warn_report("gdbstub: client sent packet while target running\n");
+            trace_gdbstub_err_unexpected_runpkt(ch);
+        } else {
+            gdbserver_state.allow_stop_reply = true;
         }
-        gdbserver_state.allow_stop_reply = true;
         vm_stop(RUN_STATE_PAUSED);
     } else
 #endif
diff --git a/gdbstub/trace-events b/gdbstub/trace-events
index 0c18a4d70a..b383bf8d29 100644
--- a/gdbstub/trace-events
+++ b/gdbstub/trace-events
@@ -26,6 +26,7 @@ gdbstub_err_invalid_repeat(uint8_t ch) "got invalid RLE count: 0x%02x"
 gdbstub_err_invalid_rle(void) "got invalid RLE sequence"
 gdbstub_err_checksum_invalid(uint8_t ch) "got invalid command checksum digit: 0x%02x"
 gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packet with incorrect checksum, expected=0x%02x, received=0x%02x"
+gdbstub_err_unexpected_runpkt(uint8_t ch) "unexpected packet (%c) while target running"
 
 # softmmu.c
 gdbstub_hit_watchpoint(const char *type, int cpu_gdb_index, uint64_t vaddr) "Watchpoint hit, type=\"%s\" cpu=%d, vaddr=0x%" PRIx64 ""
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 8/8] gdbstub: don't complain about preemptive ACK chars
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
                   ` (6 preceding siblings ...)
  2023-08-10 15:36 ` [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling Alex Bennée
@ 2023-08-10 15:36 ` Alex Bennée
  2023-08-10 15:50   ` Philippe Mathieu-Daudé
  2023-08-10 15:58 ` [PATCH 0/8] some testing and gdbstub fixes Peter Maydell
  8 siblings, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Peter Maydell, Philippe Mathieu-Daudé,
	Thomas Huth, David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta, gdb-patches

When starting a remote connection GDB sends an '+':

  /* Ack any packet which the remote side has already sent.  */
  remote_serial_write ("+", 1);

which gets flagged as a garbage character in the gdbstub state
machine. As gdb does send it out lets be permissive about the handling
so we can better see real issues.

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

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 20b6fe03fb..5f28d5cf57 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -2074,6 +2074,11 @@ void gdb_read_byte(uint8_t ch)
                 gdbserver_state.line_buf_index = 0;
                 gdbserver_state.line_sum = 0;
                 gdbserver_state.state = RS_GETLINE;
+            } else if (ch == '+') {
+                /*
+                 * do nothing, gdb may preemptively send out ACKs on
+                 * initial connection
+                 */
             } else {
                 trace_gdbstub_err_garbage(ch);
             }
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings
  2023-08-10 15:36 ` [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings Alex Bennée
@ 2023-08-10 15:47   ` Philippe Mathieu-Daudé
  2023-08-10 17:08   ` Richard Henderson
  1 sibling, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-10 15:47 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Richard Henderson,
	Peter Maydell, Thomas Huth, David Hildenbrand, qemu-arm,
	qemu-s390x, Wainer dos Santos Moschetta

On 10/8/23 17:36, Alex Bennée wrote:
> We can do this all in the run-test.py script so remove the extraneous
> bits from the individual tests which got copied from the original
> non-CI gdb tests.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/guest-debug/run-test.py                         | 2 ++
>   tests/tcg/aarch64/gdbstub/test-sve-ioctl.py           | 3 ---
>   tests/tcg/aarch64/gdbstub/test-sve.py                 | 3 ---
>   tests/tcg/multiarch/gdbstub/memory.py                 | 3 ---
>   tests/tcg/multiarch/gdbstub/sha1.py                   | 4 ----
>   tests/tcg/multiarch/gdbstub/test-proc-mappings.py     | 4 ----
>   tests/tcg/multiarch/gdbstub/test-qxfer-auxv-read.py   | 4 ----
>   tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py | 4 ----
>   tests/tcg/s390x/gdbstub/test-signals-s390x.py         | 4 ----
>   tests/tcg/s390x/gdbstub/test-svc.py                   | 4 ----
>   10 files changed, 2 insertions(+), 33 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped
  2023-08-10 15:36 ` [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped Alex Bennée
@ 2023-08-10 15:48   ` Philippe Mathieu-Daudé
  2023-08-10 17:08   ` Richard Henderson
  1 sibling, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-10 15:48 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Richard Henderson,
	Peter Maydell, Thomas Huth, David Hildenbrand, qemu-arm,
	qemu-s390x, Wainer dos Santos Moschetta

On 10/8/23 17:36, Alex Bennée wrote:
> Without -S we run into potential races with tests starting before the
> gdbstub attaches. We don't need to worry about user-mode as enabling
> the gdbstub implies we wait for the initial connection.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/guest-debug/run-test.py | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 8/8] gdbstub: don't complain about preemptive ACK chars
  2023-08-10 15:36 ` [PATCH 8/8] gdbstub: don't complain about preemptive ACK chars Alex Bennée
@ 2023-08-10 15:50   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-10 15:50 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Richard Henderson,
	Peter Maydell, Thomas Huth, David Hildenbrand, qemu-arm,
	qemu-s390x, Wainer dos Santos Moschetta, gdb-patches

On 10/8/23 17:36, Alex Bennée wrote:
> When starting a remote connection GDB sends an '+':
> 
>    /* Ack any packet which the remote side has already sent.  */
>    remote_serial_write ("+", 1);
> 
> which gets flagged as a garbage character in the gdbstub state
> machine. As gdb does send it out lets be permissive about the handling
> so we can better see real issues.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: gdb-patches@sourceware.org
> ---
>   gdbstub/gdbstub.c | 5 +++++
>   1 file changed, 5 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling
  2023-08-10 15:36 ` [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling Alex Bennée
@ 2023-08-10 15:56   ` Philippe Mathieu-Daudé
  2023-08-10 16:26   ` Richard Henderson
  1 sibling, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-10 15:56 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Richard Henderson,
	Peter Maydell, Thomas Huth, David Hildenbrand, qemu-arm,
	qemu-s390x, Wainer dos Santos Moschetta,
	Matheus Tavares Bernardino

On 10/8/23 17:36, Alex Bennée wrote:
> The original fix caused problems with spurious characters on other
> system emulation. So:
> 
>    - instead of spamming output make the warning a trace point
>    - ensure we only allow a stop reply if it was 0x3
> 
> Suggested-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <456ed3318421dd7946bdfb5ceda7e05332da368c.1690910333.git.quic_mathbern@quicinc.com>
> ---
>   gdbstub/gdbstub.c    | 5 +++--
>   gdbstub/trace-events | 1 +
>   2 files changed, 4 insertions(+), 2 deletions(-)


> diff --git a/gdbstub/trace-events b/gdbstub/trace-events
> index 0c18a4d70a..b383bf8d29 100644
> --- a/gdbstub/trace-events
> +++ b/gdbstub/trace-events
> @@ -26,6 +26,7 @@ gdbstub_err_invalid_repeat(uint8_t ch) "got invalid RLE count: 0x%02x"
>   gdbstub_err_invalid_rle(void) "got invalid RLE sequence"
>   gdbstub_err_checksum_invalid(uint8_t ch) "got invalid command checksum digit: 0x%02x"
>   gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packet with incorrect checksum, expected=0x%02x, received=0x%02x"
> +gdbstub_err_unexpected_runpkt(uint8_t ch) "unexpected packet (%c) while target running"

Since unexpected packet can be non-ASCII, better log its hexa value.

Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>





^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] some testing and gdbstub fixes
  2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
                   ` (7 preceding siblings ...)
  2023-08-10 15:36 ` [PATCH 8/8] gdbstub: don't complain about preemptive ACK chars Alex Bennée
@ 2023-08-10 15:58 ` Peter Maydell
  2023-08-10 16:35   ` Alex Bennée
  2023-08-10 16:35   ` Richard Henderson
  8 siblings, 2 replies; 23+ messages in thread
From: Peter Maydell @ 2023-08-10 15:58 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Philippe Mathieu-Daudé, Thomas Huth,
	David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta

On Thu, 10 Aug 2023 at 16:36, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This is mostly gdbstub focused but I cleaned up some bits while I was
> in the testing makefiles. This is mostly to make the "check-tcg"
> output as clean as possible without ugly line wraps. I tried to
> eliminate the gdbstub info() output but sadly this is harder than
> expected.
>
> I've tweaked the gdbstub handling for Ctrl-c packets as suggested by
> Matheus. While I was there I also noticed we were being a bit precious
> about gdb sending preemptive ACKS so I fixed that as well.
>
> I don't know if this is all late 8.1-rc material but its fairly simple
> testing updates and the ccache stuff from Daniel should help as well.

At this point in the cycle I would favour putting in
the minimum that actually fixes bugs / test failure issues.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling
  2023-08-10 15:36 ` [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling Alex Bennée
  2023-08-10 15:56   ` Philippe Mathieu-Daudé
@ 2023-08-10 16:26   ` Richard Henderson
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2023-08-10 16:26 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Peter Maydell,
	Philippe Mathieu-Daudé, Thomas Huth, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta,
	Matheus Tavares Bernardino

On 8/10/23 08:36, Alex Bennée wrote:
> The original fix caused problems with spurious characters on other
> system emulation. So:
> 
>    - instead of spamming output make the warning a trace point
>    - ensure we only allow a stop reply if it was 0x3
> 
> Suggested-by: Matheus Tavares Bernardino<quic_mathbern@quicinc.com>
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> Message-Id:<456ed3318421dd7946bdfb5ceda7e05332da368c.1690910333.git.quic_mathbern@quicinc.com>
> ---
>   gdbstub/gdbstub.c    | 5 +++--
>   gdbstub/trace-events | 1 +
>   2 files changed, 4 insertions(+), 2 deletions(-)

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


r~


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] some testing and gdbstub fixes
  2023-08-10 15:58 ` [PATCH 0/8] some testing and gdbstub fixes Peter Maydell
@ 2023-08-10 16:35   ` Alex Bennée
  2023-08-10 16:43     ` Richard Henderson
  2023-08-10 16:35   ` Richard Henderson
  1 sibling, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2023-08-10 16:35 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-devel, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Richard Henderson, Philippe Mathieu-Daudé, Thomas Huth,
	David Hildenbrand, qemu-arm, qemu-s390x,
	Wainer dos Santos Moschetta


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

> On Thu, 10 Aug 2023 at 16:36, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> This is mostly gdbstub focused but I cleaned up some bits while I was
>> in the testing makefiles. This is mostly to make the "check-tcg"
>> output as clean as possible without ugly line wraps. I tried to
>> eliminate the gdbstub info() output but sadly this is harder than
>> expected.
>>
>> I've tweaked the gdbstub handling for Ctrl-c packets as suggested by
>> Matheus. While I was there I also noticed we were being a bit precious
>> about gdb sending preemptive ACKS so I fixed that as well.
>>
>> I don't know if this is all late 8.1-rc material but its fairly simple
>> testing updates and the ccache stuff from Daniel should help as well.
>
> At this point in the cycle I would favour putting in
> the minimum that actually fixes bugs / test failure issues.

So 7 and 8? I would argue for 6 as well given that's a foot gun just
waiting to happen.

>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] some testing and gdbstub fixes
  2023-08-10 15:58 ` [PATCH 0/8] some testing and gdbstub fixes Peter Maydell
  2023-08-10 16:35   ` Alex Bennée
@ 2023-08-10 16:35   ` Richard Henderson
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2023-08-10 16:35 UTC (permalink / raw)
  To: Peter Maydell, Alex Bennée
  Cc: qemu-devel, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Philippe Mathieu-Daudé, Thomas Huth, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta

On 8/10/23 08:58, Peter Maydell wrote:
> On Thu, 10 Aug 2023 at 16:36, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> This is mostly gdbstub focused but I cleaned up some bits while I was
>> in the testing makefiles. This is mostly to make the "check-tcg"
>> output as clean as possible without ugly line wraps. I tried to
>> eliminate the gdbstub info() output but sadly this is harder than
>> expected.
>>
>> I've tweaked the gdbstub handling for Ctrl-c packets as suggested by
>> Matheus. While I was there I also noticed we were being a bit precious
>> about gdb sending preemptive ACKS so I fixed that as well.
>>
>> I don't know if this is all late 8.1-rc material but its fairly simple
>> testing updates and the ccache stuff from Daniel should help as well.
> 
> At this point in the cycle I would favour putting in
> the minimum that actually fixes bugs / test failure issues.

Indeed.  Which I think would be patches 7 and 8.


r~


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] some testing and gdbstub fixes
  2023-08-10 16:35   ` Alex Bennée
@ 2023-08-10 16:43     ` Richard Henderson
  2023-08-10 17:33       ` Richard Henderson
  0 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2023-08-10 16:43 UTC (permalink / raw)
  To: Alex Bennée, Peter Maydell
  Cc: qemu-devel, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Philippe Mathieu-Daudé, Thomas Huth, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta

On 8/10/23 09:35, Alex Bennée wrote:
> So 7 and 8? I would argue for 6 as well given that's a foot gun just
> waiting to happen.

Yes, the timing issues with 6 are nasty.


r~


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped
  2023-08-10 15:36 ` [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped Alex Bennée
  2023-08-10 15:48   ` Philippe Mathieu-Daudé
@ 2023-08-10 17:08   ` Richard Henderson
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2023-08-10 17:08 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Peter Maydell,
	Philippe Mathieu-Daudé, Thomas Huth, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta

On 8/10/23 08:36, Alex Bennée wrote:
> Without -S we run into potential races with tests starting before the
> gdbstub attaches. We don't need to worry about user-mode as enabling
> the gdbstub implies we wait for the initial connection.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   tests/guest-debug/run-test.py | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)

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

r~


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings
  2023-08-10 15:36 ` [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings Alex Bennée
  2023-08-10 15:47   ` Philippe Mathieu-Daudé
@ 2023-08-10 17:08   ` Richard Henderson
  1 sibling, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2023-08-10 17:08 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Peter Maydell,
	Philippe Mathieu-Daudé, Thomas Huth, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta

On 8/10/23 08:36, Alex Bennée wrote:
> We can do this all in the run-test.py script so remove the extraneous
> bits from the individual tests which got copied from the original
> non-CI gdb tests.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   tests/guest-debug/run-test.py                         | 2 ++
>   tests/tcg/aarch64/gdbstub/test-sve-ioctl.py           | 3 ---
>   tests/tcg/aarch64/gdbstub/test-sve.py                 | 3 ---
>   tests/tcg/multiarch/gdbstub/memory.py                 | 3 ---
>   tests/tcg/multiarch/gdbstub/sha1.py                   | 4 ----
>   tests/tcg/multiarch/gdbstub/test-proc-mappings.py     | 4 ----
>   tests/tcg/multiarch/gdbstub/test-qxfer-auxv-read.py   | 4 ----
>   tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py | 4 ----
>   tests/tcg/s390x/gdbstub/test-signals-s390x.py         | 4 ----
>   tests/tcg/s390x/gdbstub/test-svc.py                   | 4 ----
>   10 files changed, 2 insertions(+), 33 deletions(-)

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

r~


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/8] tests: remove test-gdbstub.py
  2023-08-10 15:36 ` [PATCH 4/8] tests: remove test-gdbstub.py Alex Bennée
@ 2023-08-10 17:10   ` Richard Henderson
  0 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2023-08-10 17:10 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Peter Maydell,
	Philippe Mathieu-Daudé, Thomas Huth, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta

On 8/10/23 08:36, Alex Bennée wrote:
> This isn't directly called by our CI and because it doesn't run via
> our run-test.py script does things slightly differently. Lets remove
> it as we have plenty of working in-tree tests now for various aspects
> of gdbstub.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   tests/guest-debug/test-gdbstub.py | 177 ------------------------------
>   1 file changed, 177 deletions(-)
>   delete mode 100644 tests/guest-debug/test-gdbstub.py

The first sentence could be clearer.  But as it's unused,

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


r~


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] some testing and gdbstub fixes
  2023-08-10 16:43     ` Richard Henderson
@ 2023-08-10 17:33       ` Richard Henderson
  0 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2023-08-10 17:33 UTC (permalink / raw)
  To: Alex Bennée, Peter Maydell
  Cc: qemu-devel, Ilya Leoshkevich, Yonggang Luo, Beraldo Leal,
	Philippe Mathieu-Daudé, Thomas Huth, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta

On 8/10/23 09:43, Richard Henderson wrote:
> On 8/10/23 09:35, Alex Bennée wrote:
>> So 7 and 8? I would argue for 6 as well given that's a foot gun just
>> waiting to happen.
> 
> Yes, the timing issues with 6 are nasty.

I'm going to queue 6-8 to tcg-next, along with the %x change Phil suggested for logging 
non-ASCII characters.


r~



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/8] tests/docker: cleanup non-verbose output
  2023-08-10 15:36 ` [PATCH 2/8] tests/docker: cleanup non-verbose output Alex Bennée
@ 2023-08-14  7:59   ` Thomas Huth
  0 siblings, 0 replies; 23+ messages in thread
From: Thomas Huth @ 2023-08-14  7:59 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel
  Cc: Ilya Leoshkevich, Yonggang Luo, Beraldo Leal, Richard Henderson,
	Peter Maydell, Philippe Mathieu-Daudé, David Hildenbrand,
	qemu-arm, qemu-s390x, Wainer dos Santos Moschetta

On 10/08/2023 17.36, Alex Bennée wrote:
> Even with --quiet docker will spam the sha256 to the console. Avoid
> this by redirecting stdout. While we are at it fix the name we echo
> which was broken during 0b1a649047 (tests/docker: use direct RUNC call
> to build containers).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/docker/Makefile.include | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index 142e8605ee..dfabafab92 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -46,9 +46,9 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
>   		--build-arg BUILDKIT_INLINE_CACHE=1 	\
>   		$(if $(NOUSER),,			\
>   			--build-arg USER=$(USER)	\
> -			--build-arg UID=$(UID))	\
> -		-t qemu/$* - < $<, 			\
> -		"BUILD", $1)
> +			--build-arg UID=$(UID))		\
> +		-t qemu/$* - < $< $(if $V,,> /dev/null),\
> +		"BUILD", $*)

Reviewed-by: Thomas Huth <thuth@redhat.com>



^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2023-08-14  8:00 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-10 15:36 [PATCH 0/8] some testing and gdbstub fixes Alex Bennée
2023-08-10 15:36 ` [PATCH 1/8] gitlab: enable ccache for many build jobs Alex Bennée
2023-08-10 15:36 ` [PATCH 2/8] tests/docker: cleanup non-verbose output Alex Bennée
2023-08-14  7:59   ` Thomas Huth
2023-08-10 15:36 ` [PATCH 3/8] tests/tcg: remove quoting for info output Alex Bennée
2023-08-10 15:36 ` [PATCH 4/8] tests: remove test-gdbstub.py Alex Bennée
2023-08-10 17:10   ` Richard Henderson
2023-08-10 15:36 ` [PATCH 5/8] tests/tcg: clean-up gdb confirm/pagination settings Alex Bennée
2023-08-10 15:47   ` Philippe Mathieu-Daudé
2023-08-10 17:08   ` Richard Henderson
2023-08-10 15:36 ` [PATCH 6/8] tests/tcg: ensure system-mode gdb tests start stopped Alex Bennée
2023-08-10 15:48   ` Philippe Mathieu-Daudé
2023-08-10 17:08   ` Richard Henderson
2023-08-10 15:36 ` [PATCH 7/8] gdbstub: more fixes for client Ctrl-C handling Alex Bennée
2023-08-10 15:56   ` Philippe Mathieu-Daudé
2023-08-10 16:26   ` Richard Henderson
2023-08-10 15:36 ` [PATCH 8/8] gdbstub: don't complain about preemptive ACK chars Alex Bennée
2023-08-10 15:50   ` Philippe Mathieu-Daudé
2023-08-10 15:58 ` [PATCH 0/8] some testing and gdbstub fixes Peter Maydell
2023-08-10 16:35   ` Alex Bennée
2023-08-10 16:43     ` Richard Henderson
2023-08-10 17:33       ` Richard Henderson
2023-08-10 16:35   ` Richard Henderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).