[Bug 1871798] [NEW] Fails to start on Windows host without explicit --disable-pie

[Bug 1871798] [NEW] Fails to start on Windows host without explicit --disable-pie

[James Le Cuirot]

Public bug reported:

Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed the
x86 conditional around PIE, QEMU completely fails to start on a Windows
host unless --disable-pie is explicitly given at build time. Even just
requesting the help text doesn't work. To make testing easier, this can
be replicated with Wine.

** Affects: qemu
     Importance: Undecided
         Status: New


** Tags: windows

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  New

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[Bug 1871798] Re: Fails to start on Windows host without explicit --disable-pie

[Alex Bennée]

** Tags added: configure pie

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  New

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[Bug 1871798] Re: Fails to start on Windows host without explicit --disable-pie

[Alex Bennée]

What compiler and toolchain are you using?

** Changed in: qemu
       Status: New => Incomplete

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[Bug 1871798] Re: Fails to start on Windows host without explicit --disable-pie

[James Le Cuirot]

I'm using GCC 9.3.0 with mingw-w64 7.0.0, all built with Gentoo Linux's
crossdev.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[Bug 1871798] Re: Fails to start on Windows host without explicit --disable-pie

[James Le Cuirot]

I didn't know whether PIE is generally supported on Windows or not. It
was possible that Gentoo is just inadvertently disabling support for it.
It did stem from a bug report though and reading around, others
elsewhere have reported that PIE on Windows doesn't work.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[PATCH for 5.0-rc3 v1 00/11] more random fixes

[Alex Bennée]

Hi,

Here are some more random fixes for the tree. In no particular order
we have:

  - A couple of bugs found in the gdbstub GByteArray conversion
  - A trivial fix to /proc/self/stat output
  - An attempt to fix broken PIE builds for Windows (please test!)
  - Some fixes to get "make docker-all-tests" running again
  - Some travis MacOSX tweaks

I've also included the guest base re-factoring patches as it makes it
easier for me to soak test the tree with the sanitiser although those
actual fixes won't go into 5.0 at this late stage.

The following patches need review:

 - linux-user: fix /proc/self/stat handling
 - configure: disable PIE for Windows builds
 - configure: redirect sphinx-build check to config.log
 - tests/docker: add docs FEATURE flag and use for test-misc
 - linux-user: completely re-write init_guest_space

Alex Bennée (5):
  linux-user: completely re-write init_guest_space
  tests/docker: add docs FEATURE flag and use for test-misc
  configure: redirect sphinx-build check to config.log
  configure: disable PIE for Windows builds
  linux-user: fix /proc/self/stat handling

Peter Xu (1):
  gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

Philippe Mathieu-Daudé (2):
  target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray
  .travis.yml: Build OSX 10.14 with Xcode 10.0

Richard Henderson (3):
  exec/cpu-all: Use bool for have_guest_base
  accel/tcg: Relax va restrictions on 64-bit guests
  linux-user/ppc: Fix padding in mcontext_t for ppc64

 configure                                  |   3 +-
 include/exec/cpu-all.h                     |  25 +-
 linux-user/qemu.h                          |  31 +-
 target/alpha/cpu-param.h                   |  15 +-
 accel/tcg/translate-all.c                  |  15 +-
 bsd-user/main.c                            |   4 +-
 linux-user/elfload.c                       | 503 ++++++++++-----------
 linux-user/flatload.c                      |   6 +
 linux-user/main.c                          |  27 +-
 linux-user/ppc/signal.c                    |  69 ++-
 linux-user/syscall.c                       |  43 +-
 target/i386/gdbstub.c                      |   2 +-
 target/m68k/helper.c                       |   4 +-
 .travis.yml                                |   4 +-
 tests/docker/dockerfiles/debian10.docker   |   2 +
 tests/docker/dockerfiles/debian9.docker    |   2 -
 tests/docker/dockerfiles/fedora.docker     |   2 +-
 tests/docker/dockerfiles/travis.docker     |   2 +-
 tests/docker/dockerfiles/ubuntu.docker     |   2 +-
 tests/docker/dockerfiles/ubuntu1804.docker |   2 +-
 tests/docker/test-misc                     |   2 +
 21 files changed, 375 insertions(+), 390 deletions(-)

-- 
2.20.1

[PATCH v1 01/11] linux-user: completely re-write init_guest_space

[Alex Bennée]

First we ensure all guest space initialisation logic comes through
probe_guest_base once we understand the nature of the binary we are
loading. The convoluted init_guest_space routine is removed and
replaced with a number of pgb_* helpers which are called depending on
what requirements we have when loading the binary.

We first try to do what is requested by the host. Failing that we try
and satisfy the guest requested base address. If all those options
fail we fall back to finding a space in the memory map using our
recently written read_self_maps() helper.

There are some additional complications we try and take into account
when looking for holes in the address space. We try not to go directly
after the system brk() space so there is space for a little growth. We
also don't want to have to use negative offsets which would result in
slightly less efficient code on x86 when it's unable to use the
segment offset register.

Less mind-binding gotos and hopefully clearer logic throughout.

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

---
v3
  - include rth updates that
    - split probe_guest_base into multiple functions
    - more heuristics on gap finding
---
 linux-user/qemu.h     |  31 ++-
 linux-user/elfload.c  | 503 +++++++++++++++++++++---------------------
 linux-user/flatload.c |   6 +
 linux-user/main.c     |  23 +-
 4 files changed, 277 insertions(+), 286 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 792c74290f8..e1febb88cf5 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -219,18 +219,27 @@ void init_qemu_uname_release(void);
 void fork_start(void);
 void fork_end(int child);
 
-/* Creates the initial guest address space in the host memory space using
- * the given host start address hint and size.  The guest_start parameter
- * specifies the start address of the guest space.  guest_base will be the
- * difference between the host start address computed by this function and
- * guest_start.  If fixed is specified, then the mapped address space must
- * start at host_start.  The real start address of the mapped memory space is
- * returned or -1 if there was an error.
+/**
+ * probe_guest_base:
+ * @image_name: the executable being loaded
+ * @loaddr: the lowest fixed address in the executable
+ * @hiaddr: the highest fixed address in the executable
+ *
+ * Creates the initial guest address space in the host memory space.
+ * 
+ * If @loaddr == 0, then no address in the executable is fixed,
+ * i.e. it is fully relocatable.  In that case @hiaddr is the size
+ * of the executable.
+ *
+ * This function will not return if a valid value for guest_base
+ * cannot be chosen.  On return, the executable loader can expect
+ *
+ *    target_mmap(loaddr, hiaddr - loaddr, ...)
+ *
+ * to succeed.
  */
-unsigned long init_guest_space(unsigned long host_start,
-                               unsigned long host_size,
-                               unsigned long guest_start,
-                               bool fixed);
+void probe_guest_base(const char *image_name,
+                      abi_ulong loaddr, abi_ulong hiaddr);
 
 #include "qemu/log.h"
 
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 619c054cc48..01a9323a637 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -11,6 +11,7 @@
 #include "qemu/queue.h"
 #include "qemu/guest-random.h"
 #include "qemu/units.h"
+#include "qemu/selfmap.h"
 
 #ifdef _ARCH_PPC64
 #undef ARCH_DLINFO
@@ -382,68 +383,30 @@ enum {
 
 /* The commpage only exists for 32 bit kernels */
 
-/* Return 1 if the proposed guest space is suitable for the guest.
- * Return 0 if the proposed guest space isn't suitable, but another
- * address space should be tried.
- * Return -1 if there is no way the proposed guest space can be
- * valid regardless of the base.
- * The guest code may leave a page mapped and populate it if the
- * address is suitable.
- */
-static int init_guest_commpage(unsigned long guest_base,
-                               unsigned long guest_size)
-{
-    unsigned long real_start, test_page_addr;
-
-    /* We need to check that we can force a fault on access to the
-     * commpage at 0xffff0fxx
-     */
-    test_page_addr = guest_base + (0xffff0f00 & qemu_host_page_mask);
-
-    /* If the commpage lies within the already allocated guest space,
-     * then there is no way we can allocate it.
-     *
-     * You may be thinking that that this check is redundant because
-     * we already validated the guest size against MAX_RESERVED_VA;
-     * but if qemu_host_page_mask is unusually large, then
-     * test_page_addr may be lower.
-     */
-    if (test_page_addr >= guest_base
-        && test_page_addr < (guest_base + guest_size)) {
-        return -1;
-    }
+#define ARM_COMMPAGE (intptr_t)0xffff0f00u
 
-    /* Note it needs to be writeable to let us initialise it */
-    real_start = (unsigned long)
-                 mmap((void *)test_page_addr, qemu_host_page_size,
-                     PROT_READ | PROT_WRITE,
-                     MAP_ANONYMOUS | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+static bool init_guest_commpage(void)
+{
+    void *want = g2h(ARM_COMMPAGE & -qemu_host_page_size);
+    void *addr = mmap(want, qemu_host_page_size, PROT_READ | PROT_WRITE,
+                      MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
 
-    /* If we can't map it then try another address */
-    if (real_start == -1ul) {
-        return 0;
+    if (addr == MAP_FAILED) {
+        perror("Allocating guest commpage");
+        exit(EXIT_FAILURE);
     }
-
-    if (real_start != test_page_addr) {
-        /* OS didn't put the page where we asked - unmap and reject */
-        munmap((void *)real_start, qemu_host_page_size);
-        return 0;
+    if (addr != want) {
+        return false;
     }
 
-    /* Leave the page mapped
-     * Populate it (mmap should have left it all 0'd)
-     */
-
-    /* Kernel helper versions */
-    __put_user(5, (uint32_t *)g2h(0xffff0ffcul));
+    /* Set kernel helper versions; rest of page is 0.  */
+    __put_user(5, (uint32_t *)g2h(0xffff0ffcu));
 
-    /* Now it's populated make it RO */
-    if (mprotect((void *)test_page_addr, qemu_host_page_size, PROT_READ)) {
+    if (mprotect(addr, qemu_host_page_size, PROT_READ)) {
         perror("Protecting guest commpage");
-        exit(-1);
+        exit(EXIT_FAILURE);
     }
-
-    return 1; /* All good */
+    return true;
 }
 
 #define ELF_HWCAP get_elf_hwcap()
@@ -2075,239 +2038,267 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
     return sp;
 }
 
-unsigned long init_guest_space(unsigned long host_start,
-                               unsigned long host_size,
-                               unsigned long guest_start,
-                               bool fixed)
-{
-    /* In order to use host shmat, we must be able to honor SHMLBA.  */
-    unsigned long align = MAX(SHMLBA, qemu_host_page_size);
-    unsigned long current_start, aligned_start;
-    int flags;
-
-    assert(host_start || host_size);
-
-    /* If just a starting address is given, then just verify that
-     * address.  */
-    if (host_start && !host_size) {
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-        if (init_guest_commpage(host_start, host_size) != 1) {
-            return (unsigned long)-1;
-        }
+#ifndef ARM_COMMPAGE
+#define ARM_COMMPAGE 0
+#define init_guest_commpage() true
 #endif
-        return host_start;
-    }
 
-    /* Setup the initial flags and start address.  */
-    current_start = host_start & -align;
-    flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
-    if (fixed) {
-        flags |= MAP_FIXED;
-    }
+static void pgb_fail_in_use(const char *image_name)
+{
+    error_report("%s: requires virtual address space that is in use "
+                 "(omit the -B option or choose a different value)",
+                 image_name);
+    exit(EXIT_FAILURE);
+}
 
-    /* Otherwise, a non-zero size region of memory needs to be mapped
-     * and validated.  */
+static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr,
+                                abi_ulong guest_hiaddr, long align)
+{
+    const int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
+    void *addr, *test;
 
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-    /* On 32-bit ARM, we need to map not just the usable memory, but
-     * also the commpage.  Try to find a suitable place by allocating
-     * a big chunk for all of it.  If host_start, then the naive
-     * strategy probably does good enough.
-     */
-    if (!host_start) {
-        unsigned long guest_full_size, host_full_size, real_start;
-
-        guest_full_size =
-            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
-        host_full_size = guest_full_size - guest_start;
-        real_start = (unsigned long)
-            mmap(NULL, host_full_size, PROT_NONE, flags, -1, 0);
-        if (real_start == (unsigned long)-1) {
-            if (host_size < host_full_size - qemu_host_page_size) {
-                /* We failed to map a continous segment, but we're
-                 * allowed to have a gap between the usable memory and
-                 * the commpage where other things can be mapped.
-                 * This sparseness gives us more flexibility to find
-                 * an address range.
-                 */
-                goto naive;
-            }
-            return (unsigned long)-1;
+    if (!QEMU_IS_ALIGNED(guest_base, align)) {
+        fprintf(stderr, "Requested guest base 0x%lx does not satisfy "
+                "host minimum alignment (0x%lx)\n",
+                guest_base, align);
+        exit(EXIT_FAILURE);
+    }
+
+    /* Sanity check the guest binary. */
+    if (reserved_va) {
+        if (guest_hiaddr > reserved_va) {
+            error_report("%s: requires more than reserved virtual "
+                         "address space (0x%" PRIx64 " > 0x%lx)",
+                         image_name, (uint64_t)guest_hiaddr, reserved_va);
+            exit(EXIT_FAILURE);
         }
-        munmap((void *)real_start, host_full_size);
-        if (real_start & (align - 1)) {
-            /* The same thing again, but with extra
-             * so that we can shift around alignment.
-             */
-            unsigned long real_size = host_full_size + qemu_host_page_size;
-            real_start = (unsigned long)
-                mmap(NULL, real_size, PROT_NONE, flags, -1, 0);
-            if (real_start == (unsigned long)-1) {
-                if (host_size < host_full_size - qemu_host_page_size) {
-                    goto naive;
-                }
-                return (unsigned long)-1;
-            }
-            munmap((void *)real_start, real_size);
-            real_start = ROUND_UP(real_start, align);
+    } else {
+        if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) {
+            error_report("%s: requires more virtual address space "
+                         "than the host can provide (0x%" PRIx64 ")",
+                         image_name, (uint64_t)guest_hiaddr - guest_base);
+            exit(EXIT_FAILURE);
         }
-        current_start = real_start;
     }
- naive:
-#endif
 
-    while (1) {
-        unsigned long real_start, real_size, aligned_size;
-        aligned_size = real_size = host_size;
+    /*
+     * Expand the allocation to the entire reserved_va.
+     * Exclude the mmap_min_addr hole.
+     */
+    if (reserved_va) {
+        guest_loaddr = (guest_base >= mmap_min_addr ? 0
+                        : mmap_min_addr - guest_base);
+        guest_hiaddr = reserved_va;
+    }
 
-        /* Do not use mmap_find_vma here because that is limited to the
-         * guest address space.  We are going to make the
-         * guest address space fit whatever we're given.
-         */
-        real_start = (unsigned long)
-            mmap((void *)current_start, host_size, PROT_NONE, flags, -1, 0);
-        if (real_start == (unsigned long)-1) {
-            return (unsigned long)-1;
-        }
+    /* Reserve the address space for the binary, or reserved_va. */
+    test = g2h(guest_loaddr);
+    addr = mmap(test, guest_hiaddr - guest_loaddr, PROT_NONE, flags, -1, 0);
+    if (test != addr) {
+        pgb_fail_in_use(image_name);
+    }
+}
 
-        /* Check to see if the address is valid.  */
-        if (host_start && real_start != current_start) {
-            qemu_log_mask(CPU_LOG_PAGE, "invalid %lx && %lx != %lx\n",
-                          host_start, real_start, current_start);
-            goto try_again;
+/* Return value for guest_base, or -1 if no hole found. */
+static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
+                               long align)
+{
+    GSList *maps, *iter;
+    uintptr_t this_start, this_end, next_start, brk;
+    intptr_t ret = -1;
+
+    assert(QEMU_IS_ALIGNED(guest_loaddr, align));
+
+    maps = read_self_maps();
+
+    /* Read brk after we've read the maps, which will malloc. */
+    brk = (uintptr_t)sbrk(0);
+
+    /* The first hole is before the first map entry. */
+    this_start = mmap_min_addr;
+
+    for (iter = maps; iter;
+         this_start = next_start, iter = g_slist_next(iter)) {
+        uintptr_t align_start, hole_size;
+
+        this_end = ((MapInfo *)iter->data)->start;
+        next_start = ((MapInfo *)iter->data)->end;
+        align_start = ROUND_UP(this_start, align);
+
+        /* Skip holes that are too small. */
+        if (align_start >= this_end) {
+            continue;
+        }
+        hole_size = this_end - align_start;
+        if (hole_size < guest_size) {
+            continue;
         }
 
-        /* Ensure the address is properly aligned.  */
-        if (real_start & (align - 1)) {
-            /* Ideally, we adjust like
-             *
-             *    pages: [  ][  ][  ][  ][  ]
-             *      old:   [   real   ]
-             *             [ aligned  ]
-             *      new:   [     real     ]
-             *               [ aligned  ]
-             *
-             * But if there is something else mapped right after it,
-             * then obviously it won't have room to grow, and the
-             * kernel will put the new larger real someplace else with
-             * unknown alignment (if we made it to here, then
-             * fixed=false).  Which is why we grow real by a full page
-             * size, instead of by part of one; so that even if we get
-             * moved, we can still guarantee alignment.  But this does
-             * mean that there is a padding of < 1 page both before
-             * and after the aligned range; the "after" could could
-             * cause problems for ARM emulation where it could butt in
-             * to where we need to put the commpage.
-             */
-            munmap((void *)real_start, host_size);
-            real_size = aligned_size + align;
-            real_start = (unsigned long)
-                mmap((void *)real_start, real_size, PROT_NONE, flags, -1, 0);
-            if (real_start == (unsigned long)-1) {
-                return (unsigned long)-1;
+        /* If this hole contains brk, give ourselves some room to grow. */
+        if (this_start <= brk && brk < this_end) {
+            hole_size -= guest_size;
+            if (sizeof(uintptr_t) == 8 && hole_size >= 1 * GiB) {
+                align_start += 1 * GiB;
+            } else if (hole_size >= 16 * MiB) {
+                align_start += 16 * MiB;
+            } else {
+                align_start = (this_end - guest_size) & -align;
+                if (align_start < this_start) {
+                    continue;
+                }
             }
-            aligned_start = ROUND_UP(real_start, align);
-        } else {
-            aligned_start = real_start;
         }
 
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-        /* On 32-bit ARM, we need to also be able to map the commpage.  */
-        int valid = init_guest_commpage(aligned_start - guest_start,
-                                        aligned_size + guest_start);
-        if (valid == -1) {
-            munmap((void *)real_start, real_size);
-            return (unsigned long)-1;
-        } else if (valid == 0) {
-            goto try_again;
+        /* Record the lowest successful match. */
+        if (ret < 0) {
+            ret = align_start - guest_loaddr;
         }
-#endif
-
-        /* If nothing has said `return -1` or `goto try_again` yet,
-         * then the address we have is good.
-         */
-        break;
-
-    try_again:
-        /* That address didn't work.  Unmap and try a different one.
-         * The address the host picked because is typically right at
-         * the top of the host address space and leaves the guest with
-         * no usable address space.  Resort to a linear search.  We
-         * already compensated for mmap_min_addr, so this should not
-         * happen often.  Probably means we got unlucky and host
-         * address space randomization put a shared library somewhere
-         * inconvenient.
-         *
-         * This is probably a good strategy if host_start, but is
-         * probably a bad strategy if not, which means we got here
-         * because of trouble with ARM commpage setup.
-         */
-        if (munmap((void *)real_start, real_size) != 0) {
-            error_report("%s: failed to unmap %lx:%lx (%s)", __func__,
-                         real_start, real_size, strerror(errno));
-            abort();
+        /* If this hole contains the identity map, select it. */
+        if (align_start <= guest_loaddr &&
+            guest_loaddr + guest_size <= this_end) {
+            ret = 0;
         }
-        current_start += align;
-        if (host_start == current_start) {
-            /* Theoretically possible if host doesn't have any suitably
-             * aligned areas.  Normally the first mmap will fail.
-             */
-            return (unsigned long)-1;
+        /* If this hole ends above the identity map, stop looking. */
+        if (this_end >= guest_loaddr) {
+            break;
         }
     }
+    free_self_maps(maps);
 
-    qemu_log_mask(CPU_LOG_PAGE, "Reserved 0x%lx bytes of guest address space\n", host_size);
-
-    return aligned_start;
+    return ret;
 }
 
-static void probe_guest_base(const char *image_name,
-                             abi_ulong loaddr, abi_ulong hiaddr)
+static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
+                       abi_ulong orig_hiaddr, long align)
 {
-    /* Probe for a suitable guest base address, if the user has not set
-     * it explicitly, and set guest_base appropriately.
-     * In case of error we will print a suitable message and exit.
-     */
-    const char *errmsg;
-    if (!have_guest_base && !reserved_va) {
-        unsigned long host_start, real_start, host_size;
+    uintptr_t loaddr = orig_loaddr;
+    uintptr_t hiaddr = orig_hiaddr;
+    uintptr_t addr;
 
-        /* Round addresses to page boundaries.  */
-        loaddr &= qemu_host_page_mask;
-        hiaddr = HOST_PAGE_ALIGN(hiaddr);
+    if (hiaddr != orig_hiaddr) {
+        error_report("%s: requires virtual address space that the "
+                     "host cannot provide (0x%" PRIx64 ")",
+                     image_name, (uint64_t)orig_hiaddr);
+        exit(EXIT_FAILURE);
+    }
 
-        if (loaddr < mmap_min_addr) {
-            host_start = HOST_PAGE_ALIGN(mmap_min_addr);
+    loaddr &= -align;
+    if (ARM_COMMPAGE) {
+        /*
+         * Extend the allocation to include the commpage.
+         * For a 64-bit host, this is just 4GiB; for a 32-bit host,
+         * the address arithmetic will wrap around, but the difference
+         * will produce the correct allocation size.
+         */
+        if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) {
+            hiaddr = (uintptr_t)4 << 30;
         } else {
-            host_start = loaddr;
-            if (host_start != loaddr) {
-                errmsg = "Address overflow loading ELF binary";
-                goto exit_errmsg;
-            }
+            loaddr = ARM_COMMPAGE & -align;
         }
-        host_size = hiaddr - loaddr;
+    }
 
-        /* Setup the initial guest memory space with ranges gleaned from
-         * the ELF image that is being loaded.
+    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align);
+    if (addr == -1) {
+        /*
+         * If ARM_COMMPAGE, there *might* be a non-consecutive allocation
+         * that can satisfy both.  But as the normal arm32 link base address
+         * is ~32k, and we extend down to include the commpage, making the
+         * overhead only ~96k, this is unlikely.
          */
-        real_start = init_guest_space(host_start, host_size, loaddr, false);
-        if (real_start == (unsigned long)-1) {
-            errmsg = "Unable to find space for application";
-            goto exit_errmsg;
-        }
-        guest_base = real_start - loaddr;
+        error_report("%s: Unable to allocate %#zx bytes of "
+                     "virtual address space", image_name,
+                     (size_t)(hiaddr - loaddr));
+        exit(EXIT_FAILURE);
+    }
+
+    guest_base = addr;
+}
+
+static void pgb_dynamic(const char *image_name, long align)
+{
+    /*
+     * The executable is dynamic and does not require a fixed address.
+     * All we need is a commpage that satisfies align.
+     * If we do not need a commpage, leave guest_base == 0.
+     */
+    if (ARM_COMMPAGE) {
+        uintptr_t addr, commpage;
 
-        qemu_log_mask(CPU_LOG_PAGE, "Relocating guest address space from 0x"
-                      TARGET_ABI_FMT_lx " to 0x%lx\n",
-                      loaddr, real_start);
+        /* 64-bit hosts should have used reserved_va. */
+        assert(sizeof(uintptr_t) == 4);
+
+        /*
+         * By putting the commpage at the first hole, that puts guest_base
+         * just above that, and maximises the positive guest addresses.
+         */
+        commpage = ARM_COMMPAGE & -align;
+        addr = pgb_find_hole(commpage, -commpage, align);
+        assert(addr != -1);
+        guest_base = addr;
     }
-    return;
+}
 
-exit_errmsg:
-    fprintf(stderr, "%s: %s\n", image_name, errmsg);
-    exit(-1);
+static void pgb_reserved_va(const char *image_name, abi_ulong guest_loaddr,
+                            abi_ulong guest_hiaddr, long align)
+{
+    const int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
+    void *addr, *test;
+
+    if (guest_hiaddr > reserved_va) {
+        error_report("%s: requires more than reserved virtual "
+                     "address space (0x%" PRIx64 " > 0x%lx)",
+                     image_name, (uint64_t)guest_hiaddr, reserved_va);
+        exit(EXIT_FAILURE);
+    }
+
+    /* Widen the "image" to the entire reserved address space. */
+    pgb_static(image_name, 0, reserved_va, align);
+
+    /* Reserve the memory on the host. */
+    assert(guest_base != 0);
+    test = g2h(0);
+    addr = mmap(test, reserved_va, PROT_NONE, flags, -1, 0);
+    if (addr == MAP_FAILED) {
+        error_report("Unable to reserve 0x%lx bytes of virtual address "
+                     "space for use as guest address space (check your "
+                     "virtual memory ulimit setting or reserve less "
+                     "using -R option)", reserved_va);
+        exit(EXIT_FAILURE);
+    }
+    assert(addr == test);
 }
 
+void probe_guest_base(const char *image_name, abi_ulong guest_loaddr,
+                      abi_ulong guest_hiaddr)
+{
+    /* In order to use host shmat, we must be able to honor SHMLBA.  */
+    uintptr_t align = MAX(SHMLBA, qemu_host_page_size);
+
+    if (have_guest_base) {
+        pgb_have_guest_base(image_name, guest_loaddr, guest_hiaddr, align);
+    } else if (reserved_va) {
+        pgb_reserved_va(image_name, guest_loaddr, guest_hiaddr, align);
+    } else if (guest_loaddr) {
+        pgb_static(image_name, guest_loaddr, guest_hiaddr, align);
+    } else {
+        pgb_dynamic(image_name, align);
+    }
+
+    /* Reserve and initialize the commpage. */
+    if (!init_guest_commpage()) {
+        /*
+         * With have_guest_base, the user has selected the address and
+         * we are trying to work with that.  Otherwise, we have selected
+         * free space and init_guest_commpage must succeeded.
+         */
+        assert(have_guest_base);
+        pgb_fail_in_use(image_name);
+    }
+
+    assert(QEMU_IS_ALIGNED(guest_base, align));
+    qemu_log_mask(CPU_LOG_PAGE, "Locating guest address space "
+                  "@ 0x%" PRIx64 "\n", (uint64_t)guest_base);
+}
 
 /* Load an ELF image into the address space.
 
@@ -2399,6 +2390,12 @@ static void load_elf_image(const char *image_name, int image_fd,
              * MMAP_MIN_ADDR or the QEMU application itself.
              */
             probe_guest_base(image_name, loaddr, hiaddr);
+        } else {
+            /*
+             * The binary is dynamic, but we still need to
+             * select guest_base.  In this case we pass a size.
+             */
+            probe_guest_base(image_name, 0, hiaddr - loaddr);
         }
     }
 
diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 0122ab3afe6..952b4cb26d8 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -441,6 +441,12 @@ static int load_flat_file(struct linux_binprm * bprm,
     indx_len = MAX_SHARED_LIBS * sizeof(abi_ulong);
     indx_len = (indx_len + 15) & ~(abi_ulong)15;
 
+    /*
+     * Alloate the address space.
+     */
+    probe_guest_base(bprm->filename, 0,
+                     text_len + data_len + extra + indx_len);
+
     /*
      * there are a couple of cases here,  the separate code/data
      * case,  and then the fully copied to RAM case which lumps
diff --git a/linux-user/main.c b/linux-user/main.c
index 22578b16336..1d20a83d4e8 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -24,6 +24,7 @@
 #include "qemu-version.h"
 #include <sys/syscall.h>
 #include <sys/resource.h>
+#include <sys/shm.h>
 
 #include "qapi/error.h"
 #include "qemu.h"
@@ -747,28 +748,6 @@ int main(int argc, char **argv, char **envp)
     target_environ = envlist_to_environ(envlist, NULL);
     envlist_free(envlist);
 
-    /*
-     * Now that page sizes are configured in tcg_exec_init() we can do
-     * proper page alignment for guest_base.
-     */
-    guest_base = HOST_PAGE_ALIGN(guest_base);
-
-    if (reserved_va || have_guest_base) {
-        guest_base = init_guest_space(guest_base, reserved_va, 0,
-                                      have_guest_base);
-        if (guest_base == (unsigned long)-1) {
-            fprintf(stderr, "Unable to reserve 0x%lx bytes of virtual address "
-                    "space for use as guest address space (check your virtual "
-                    "memory ulimit setting or reserve less using -R option)\n",
-                    reserved_va);
-            exit(EXIT_FAILURE);
-        }
-
-        if (reserved_va) {
-            mmap_next_start = reserved_va;
-        }
-    }
-
     /*
      * Read in mmap_min_addr kernel parameter.  This value is used
      * When loading the ELF image to determine whether guest_base
-- 
2.20.1

[PATCH v1 02/11] exec/cpu-all: Use bool for have_guest_base

[Alex Bennée]

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

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/cpu-all.h | 2 +-
 bsd-user/main.c        | 4 ++--
 linux-user/main.c      | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 49384bb66a5..b4fb5832c4a 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -159,7 +159,7 @@ static inline void tswap64s(uint64_t *s)
  * This allows the guest address space to be offset to a convenient location.
  */
 extern unsigned long guest_base;
-extern int have_guest_base;
+extern bool have_guest_base;
 extern unsigned long reserved_va;
 
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 770c2b267ad..aef5531628a 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -42,7 +42,7 @@
 int singlestep;
 unsigned long mmap_min_addr;
 unsigned long guest_base;
-int have_guest_base;
+bool have_guest_base;
 unsigned long reserved_va;
 
 static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
@@ -828,7 +828,7 @@ int main(int argc, char **argv)
             }
         } else if (!strcmp(r, "B")) {
            guest_base = strtol(argv[optind++], NULL, 0);
-           have_guest_base = 1;
+           have_guest_base = true;
         } else if (!strcmp(r, "drop-ld-preload")) {
             (void) envlist_unsetenv(envlist, "LD_PRELOAD");
         } else if (!strcmp(r, "bsd")) {
diff --git a/linux-user/main.c b/linux-user/main.c
index 1d20a83d4e8..90ad365b439 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -59,7 +59,7 @@ static const char *cpu_type;
 static const char *seed_optarg;
 unsigned long mmap_min_addr;
 unsigned long guest_base;
-int have_guest_base;
+bool have_guest_base;
 
 /*
  * Used to implement backwards-compatibility for the `-strace`, and
@@ -334,7 +334,7 @@ static void handle_arg_cpu(const char *arg)
 static void handle_arg_guest_base(const char *arg)
 {
     guest_base = strtol(arg, NULL, 0);
-    have_guest_base = 1;
+    have_guest_base = true;
 }
 
 static void handle_arg_reserved_va(const char *arg)
-- 
2.20.1

[PATCH v1 03/11] accel/tcg: Relax va restrictions on 64-bit guests

[Alex Bennée]

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

We cannot at present limit a 64-bit guest to a virtual address
space smaller than the host.  It will mostly work to ignore this
limitation, except if the guest uses high bits of the address
space for tags.  But it will certainly work better, as presently
we can wind up failing to allocate the guest stack.

Widen our user-only page tree to the host or abi pointer width.
Remove the workaround for this problem from target/alpha.
Always validate guest addresses vs reserved_va, as there we
control allocation ourselves.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/cpu-all.h    | 23 +++++++++++++++++++----
 target/alpha/cpu-param.h  | 15 ++-------------
 accel/tcg/translate-all.c | 15 +++++++++------
 3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index b4fb5832c4a..c0c2fa3cc56 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -162,12 +162,27 @@ extern unsigned long guest_base;
 extern bool have_guest_base;
 extern unsigned long reserved_va;
 
-#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
-#define GUEST_ADDR_MAX (~0ul)
+/*
+ * Limit the guest addresses as best we can.
+ *
+ * When not using -R reserved_va, we cannot really limit the guest
+ * to less address space than the host.  For 32-bit guests, this
+ * acts as a sanity check that we're not giving the guest an address
+ * that it cannot even represent.  For 64-bit guests... the address
+ * might not be what the real kernel would give, but it is at least
+ * representable in the guest.
+ *
+ * TODO: Improve address allocation to avoid this problem, and to
+ * avoid setting bits at the top of guest addresses that might need
+ * to be used for tags.
+ */
+#if MIN(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32
+# define GUEST_ADDR_MAX_  UINT32_MAX
 #else
-#define GUEST_ADDR_MAX (reserved_va ? reserved_va - 1 : \
-                                    (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+# define GUEST_ADDR_MAX_  ~0ul
 #endif
+#define GUEST_ADDR_MAX    (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_)
+
 #else
 
 #include "exec/hwaddr.h"
diff --git a/target/alpha/cpu-param.h b/target/alpha/cpu-param.h
index 692aee27ca9..1153992e42a 100644
--- a/target/alpha/cpu-param.h
+++ b/target/alpha/cpu-param.h
@@ -10,22 +10,11 @@
 
 #define TARGET_LONG_BITS 64
 #define TARGET_PAGE_BITS 13
-#ifdef CONFIG_USER_ONLY
-/*
- * ??? The kernel likes to give addresses in high memory.  If the host has
- * more virtual address space than the guest, this can lead to impossible
- * allocations.  Honor the long-standing assumption that only kernel addrs
- * are negative, but otherwise allow allocations anywhere.  This could lead
- * to tricky emulation problems for programs doing tagged addressing, but
- * that's far fewer than encounter the impossible allocation problem.
- */
-#define TARGET_PHYS_ADDR_SPACE_BITS  63
-#define TARGET_VIRT_ADDR_SPACE_BITS  63
-#else
+
 /* ??? EV4 has 34 phys addr bits, EV5 has 40, EV6 has 44.  */
 #define TARGET_PHYS_ADDR_SPACE_BITS  44
 #define TARGET_VIRT_ADDR_SPACE_BITS  (30 + TARGET_PAGE_BITS)
-#endif
+
 #define NB_MMU_MODES 3
 
 #endif
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 9924e66d1f7..e4f703a7e6d 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -173,8 +173,13 @@ struct page_collection {
 #define TB_FOR_EACH_JMP(head_tb, tb, n)                                 \
     TB_FOR_EACH_TAGGED((head_tb)->jmp_list_head, tb, n, jmp_list_next)
 
-/* In system mode we want L1_MAP to be based on ram offsets,
-   while in user mode we want it to be based on virtual addresses.  */
+/*
+ * In system mode we want L1_MAP to be based on ram offsets,
+ * while in user mode we want it to be based on virtual addresses.
+ *
+ * TODO: For user mode, see the caveat re host vs guest virtual
+ * address spaces near GUEST_ADDR_MAX.
+ */
 #if !defined(CONFIG_USER_ONLY)
 #if HOST_LONG_BITS < TARGET_PHYS_ADDR_SPACE_BITS
 # define L1_MAP_ADDR_SPACE_BITS  HOST_LONG_BITS
@@ -182,7 +187,7 @@ struct page_collection {
 # define L1_MAP_ADDR_SPACE_BITS  TARGET_PHYS_ADDR_SPACE_BITS
 #endif
 #else
-# define L1_MAP_ADDR_SPACE_BITS  TARGET_VIRT_ADDR_SPACE_BITS
+# define L1_MAP_ADDR_SPACE_BITS  MIN(HOST_LONG_BITS, TARGET_ABI_BITS)
 #endif
 
 /* Size of the L2 (and L3, etc) page tables.  */
@@ -2497,9 +2502,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
     /* This function should never be called with addresses outside the
        guest address space.  If this assert fires, it probably indicates
        a missing call to h2g_valid.  */
-#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
-    assert(end <= ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
-#endif
+    assert(end - 1 <= GUEST_ADDR_MAX);
     assert(start < end);
     assert_memory_lock();
 
-- 
2.20.1

[PATCH v1 04/11] linux-user/ppc: Fix padding in mcontext_t for ppc64

[Alex Bennée]

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

The padding that was added in 95cda4c44ee was added to a union,
and so it had no effect.  This fixes misalignment errors detected
by clang sanitizers for ppc64 and ppc64le.

In addition, only ppc64 allocates space for VSX registers, so do
not save them for ppc32.  The kernel only has references to
CONFIG_SPE in signal_32.c, so do not attempt to save them for ppc64.

Fixes: 95cda4c44ee
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/ppc/signal.c | 69 +++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index ecd99736b7e..20a02c197cb 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -35,12 +35,26 @@ struct target_mcontext {
     target_ulong mc_gregs[48];
     /* Includes fpscr.  */
     uint64_t mc_fregs[33];
+
 #if defined(TARGET_PPC64)
     /* Pointer to the vector regs */
     target_ulong v_regs;
+    /*
+     * On ppc64, this mcontext structure is naturally *unaligned*,
+     * or rather it is aligned on a 8 bytes boundary but not on
+     * a 16 byte boundary.  This pad fixes it up.  This is why we
+     * cannot use ppc_avr_t, which would force alignment.  This is
+     * also why the vector regs are referenced in the ABI by the
+     * v_regs pointer above so any amount of padding can be added here.
+     */
+    target_ulong pad;
+    /* VSCR and VRSAVE are saved separately.  Also reserve space for VSX. */
+    struct {
+        uint64_t altivec[34 + 16][2];
+    } mc_vregs;
 #else
     target_ulong mc_pad[2];
-#endif
+
     /* We need to handle Altivec and SPE at the same time, which no
        kernel needs to do.  Fortunately, the kernel defines this bit to
        be Altivec-register-large all the time, rather than trying to
@@ -48,32 +62,14 @@ struct target_mcontext {
     union {
         /* SPE vector registers.  One extra for SPEFSCR.  */
         uint32_t spe[33];
-        /* Altivec vector registers.  The packing of VSCR and VRSAVE
-           varies depending on whether we're PPC64 or not: PPC64 splits
-           them apart; PPC32 stuffs them together.
-           We also need to account for the VSX registers on PPC64
-        */
-#if defined(TARGET_PPC64)
-#define QEMU_NVRREG (34 + 16)
-        /* On ppc64, this mcontext structure is naturally *unaligned*,
-         * or rather it is aligned on a 8 bytes boundary but not on
-         * a 16 bytes one. This pad fixes it up. This is also why the
-         * vector regs are referenced by the v_regs pointer above so
-         * any amount of padding can be added here
-         */
-        target_ulong pad;
-#else
-        /* On ppc32, we are already aligned to 16 bytes */
-#define QEMU_NVRREG 33
-#endif
-        /* We cannot use ppc_avr_t here as we do *not* want the implied
-         * 16-bytes alignment that would result from it. This would have
-         * the effect of making the whole struct target_mcontext aligned
-         * which breaks the layout of struct target_ucontext on ppc64.
+        /*
+         * Altivec vector registers.  One extra for VRSAVE.
+         * On ppc32, we are already aligned to 16 bytes.  We could
+         * use ppc_avr_t, but choose to share the same type as ppc64.
          */
-        uint64_t altivec[QEMU_NVRREG][2];
-#undef QEMU_NVRREG
+        uint64_t altivec[33][2];
     } mc_vregs;
+#endif
 };
 
 /* See arch/powerpc/include/asm/sigcontext.h.  */
@@ -278,6 +274,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
         __put_user((uint32_t)env->spr[SPR_VRSAVE], vrsave);
     }
 
+#if defined(TARGET_PPC64)
     /* Save VSX second halves */
     if (env->insns_flags2 & PPC2_VSX) {
         uint64_t *vsregs = (uint64_t *)&frame->mc_vregs.altivec[34];
@@ -286,6 +283,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
             __put_user(*vsrl, &vsregs[i]);
         }
     }
+#endif
 
     /* Save floating point registers.  */
     if (env->insns_flags & PPC_FLOAT) {
@@ -296,22 +294,18 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
         __put_user((uint64_t) env->fpscr, &frame->mc_fregs[32]);
     }
 
+#if !defined(TARGET_PPC64)
     /* Save SPE registers.  The kernel only saves the high half.  */
     if (env->insns_flags & PPC_SPE) {
-#if defined(TARGET_PPC64)
-        for (i = 0; i < ARRAY_SIZE(env->gpr); i++) {
-            __put_user(env->gpr[i] >> 32, &frame->mc_vregs.spe[i]);
-        }
-#else
         for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
             __put_user(env->gprh[i], &frame->mc_vregs.spe[i]);
         }
-#endif
         /* Set MSR_SPE in the saved MSR value to indicate that
            frame->mc_vregs contains valid data.  */
         msr |= MSR_SPE;
         __put_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
     }
+#endif
 
     /* Store MSR.  */
     __put_user(msr, &frame->mc_gregs[TARGET_PT_MSR]);
@@ -392,6 +386,7 @@ static void restore_user_regs(CPUPPCState *env,
         __get_user(env->spr[SPR_VRSAVE], vrsave);
     }
 
+#if defined(TARGET_PPC64)
     /* Restore VSX second halves */
     if (env->insns_flags2 & PPC2_VSX) {
         uint64_t *vsregs = (uint64_t *)&frame->mc_vregs.altivec[34];
@@ -400,6 +395,7 @@ static void restore_user_regs(CPUPPCState *env,
             __get_user(*vsrl, &vsregs[i]);
         }
     }
+#endif
 
     /* Restore floating point registers.  */
     if (env->insns_flags & PPC_FLOAT) {
@@ -412,22 +408,15 @@ static void restore_user_regs(CPUPPCState *env,
         env->fpscr = (uint32_t) fpscr;
     }
 
+#if !defined(TARGET_PPC64)
     /* Save SPE registers.  The kernel only saves the high half.  */
     if (env->insns_flags & PPC_SPE) {
-#if defined(TARGET_PPC64)
-        for (i = 0; i < ARRAY_SIZE(env->gpr); i++) {
-            uint32_t hi;
-
-            __get_user(hi, &frame->mc_vregs.spe[i]);
-            env->gpr[i] = ((uint64_t)hi << 32) | ((uint32_t) env->gpr[i]);
-        }
-#else
         for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
             __get_user(env->gprh[i], &frame->mc_vregs.spe[i]);
         }
-#endif
         __get_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
     }
+#endif
 }
 
 #if !defined(TARGET_PPC64)
-- 
2.20.1

[PATCH v1 05/11] tests/docker: add docs FEATURE flag and use for test-misc

[Alex Bennée]

The test-misc docker test fails on a number of images which don't have
the prerequisites to build the docs. Use the FEATURES flag so we can
skip those tests.

As the sphinx test fails to detect whatever feature we need to get
hxtool to work we drop them from debian9 so the windows build doesn't
attempt to build the docs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/dockerfiles/debian10.docker   | 2 ++
 tests/docker/dockerfiles/debian9.docker    | 2 --
 tests/docker/dockerfiles/fedora.docker     | 2 +-
 tests/docker/dockerfiles/travis.docker     | 2 +-
 tests/docker/dockerfiles/ubuntu.docker     | 2 +-
 tests/docker/dockerfiles/ubuntu1804.docker | 2 +-
 tests/docker/test-misc                     | 2 ++
 7 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
index 2fcdc406e83..0769700a416 100644
--- a/tests/docker/dockerfiles/debian10.docker
+++ b/tests/docker/dockerfiles/debian10.docker
@@ -34,3 +34,5 @@ RUN apt update && \
         python3-sphinx \
         texinfo \
         $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
+
+ENV FEATURES docs
diff --git a/tests/docker/dockerfiles/debian9.docker b/tests/docker/dockerfiles/debian9.docker
index 92edbbf0f48..08cc970feb1 100644
--- a/tests/docker/dockerfiles/debian9.docker
+++ b/tests/docker/dockerfiles/debian9.docker
@@ -30,6 +30,4 @@ RUN apt update && \
         pkg-config \
         psmisc \
         python3 \
-        python3-sphinx \
-        texinfo \
         $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
index 4bd2c953af8..179575ecaaa 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -103,4 +103,4 @@ ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3
 RUN dnf install -y $PACKAGES
 RUN rpm -q $PACKAGES | sort > /packages.txt
 ENV PATH $PATH:/usr/libexec/python3-sphinx/
-ENV FEATURES mingw clang pyyaml asan
+ENV FEATURES mingw clang pyyaml asan docs
diff --git a/tests/docker/dockerfiles/travis.docker b/tests/docker/dockerfiles/travis.docker
index e8eb48dccfd..591282561bc 100644
--- a/tests/docker/dockerfiles/travis.docker
+++ b/tests/docker/dockerfiles/travis.docker
@@ -13,5 +13,5 @@ RUN apt-get -y install device-tree-compiler python3 python3-yaml dh-autoreconf g
 # Travis tools require PhantomJS / Neo4j / Maven accessible
 # in their PATH (QEMU build won't access them).
 ENV PATH /usr/local/phantomjs/bin:/usr/local/phantomjs:/usr/local/neo4j-3.2.7/bin:/usr/local/maven-3.5.2/bin:/usr/local/cmake-3.9.2/bin:/usr/local/clang-5.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
-ENV FEATURES clang pyyaml
+ENV FEATURES clang pyyaml docs
 USER travis
diff --git a/tests/docker/dockerfiles/ubuntu.docker b/tests/docker/dockerfiles/ubuntu.docker
index b6c7b41dddd..eeb3b22bf20 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -68,4 +68,4 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
     DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
-ENV FEATURES clang pyyaml sdl2
+ENV FEATURES clang pyyaml sdl2 docs
diff --git a/tests/docker/dockerfiles/ubuntu1804.docker b/tests/docker/dockerfiles/ubuntu1804.docker
index 1efedeef995..f66b06f4cff 100644
--- a/tests/docker/dockerfiles/ubuntu1804.docker
+++ b/tests/docker/dockerfiles/ubuntu1804.docker
@@ -54,7 +54,7 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
     DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
-ENV FEATURES clang pyyaml sdl2
+ENV FEATURES clang pyyaml sdl2 docs
 
 # https://bugs.launchpad.net/qemu/+bug/1838763
 ENV QEMU_CONFIGURE_OPTS --disable-libssh
diff --git a/tests/docker/test-misc b/tests/docker/test-misc
index d480afedca7..cc94a738dd0 100755
--- a/tests/docker/test-misc
+++ b/tests/docker/test-misc
@@ -14,6 +14,8 @@
 
 . common.rc
 
+requires docs
+
 cd "$BUILD_DIR"
 
 # build everything else but QEMU
-- 
2.20.1

[PATCH v1 06/11] configure: redirect sphinx-build check to config.log

[Alex Bennée]

Otherwise it's hard to debug whats going on.

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 233c671aaa9..a207cce82bc 100755
--- a/configure
+++ b/configure
@@ -4936,7 +4936,7 @@ has_sphinx_build() {
     # sphinx-build doesn't exist at all or if it is too old.
     mkdir -p "$TMPDIR1/sphinx"
     touch "$TMPDIR1/sphinx/index.rst"
-    "$sphinx_build" -c "$source_path/docs" -b html "$TMPDIR1/sphinx" "$TMPDIR1/sphinx/out" >/dev/null 2>&1
+    "$sphinx_build" -c "$source_path/docs" -b html "$TMPDIR1/sphinx" "$TMPDIR1/sphinx/out" >> config.log 2>&1
 }
 
 # Check if tools are available to build documentation.
-- 
2.20.1

[PATCH v1 07/11] configure: disable PIE for Windows builds

[Alex Bennée]

It seems on some compilers the test can pass but still give you
broken binaries.

[AJB untested - please could windows users test]

Fixes: d2cd29e30736
Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
Cc: Bug 1871798 <1871798@bugs.launchpad.net>
Cc: James Le Cuirot <chewi@aura-online.co.uk>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index a207cce82bc..e9c5f630c14 100755
--- a/configure
+++ b/configure
@@ -807,6 +807,7 @@ MINGW32*)
     audio_drv_list=""
   fi
   supported_os="yes"
+  pie="no"
 ;;
 GNU/kFreeBSD)
   bsd="yes"
-- 
2.20.1

[Bug 1871798] [PATCH v1 07/11] configure: disable PIE for Windows builds

[Alex Bennée]

It seems on some compilers the test can pass but still give you
broken binaries.

[AJB untested - please could windows users test]

Fixes: d2cd29e30736
Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
Cc: Bug 1871798 <1871798@bugs.launchpad.net>
Cc: James Le Cuirot <chewi@aura-online.co.uk>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index a207cce82bc..e9c5f630c14 100755
--- a/configure
+++ b/configure
@@ -807,6 +807,7 @@ MINGW32*)
     audio_drv_list=""
   fi
   supported_os="yes"
+  pie="no"
 ;;
 GNU/kFreeBSD)
   bsd="yes"
-- 
2.20.1

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[PATCH v1 08/11] target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray

[Alex Bennée]

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

Since a010bdbe719 the gdbstub API takes a GByteArray*. Unfortunately
we forgot to update the gdb_get_reg*() calls. Do it now.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200409172509.4078-1-philmd@redhat.com>
---
 target/m68k/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 014657c6372..cad40838956 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -109,8 +109,8 @@ static int m68k_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
 {
     if (n < 8) {
         int len = gdb_get_reg16(mem_buf, env->fregs[n].l.upper);
-        len += gdb_get_reg16(mem_buf + len, 0);
-        len += gdb_get_reg64(mem_buf + len, env->fregs[n].l.lower);
+        len += gdb_get_reg16(mem_buf, 0);
+        len += gdb_get_reg64(mem_buf, env->fregs[n].l.lower);
         return len;
     }
     switch (n) {
-- 
2.20.1

[PATCH v1 09/11] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Alex Bennée]

From: Peter Xu <peterx@redhat.com>

We should only pass in gdb_get_reg16() with the GByteArray* object
itself, no need to shift.  Without this patch, gdb remote attach will
crash QEMU.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200409164954.36902-3-peterx@redhat.com>
---
 target/i386/gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index f3d23b614ee..b98a99500ae 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
         floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
         int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
-        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
+        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
         return len;
     } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
         n -= IDX_XMM_REGS;
-- 
2.20.1

[PATCH v1 10/11] linux-user: fix /proc/self/stat handling

[Alex Bennée]

In the original bug report long files names in Guix caused
/proc/self/stat be truncated without the trailing ") " as specified in
proc manpage which says:
    (2) comm  %s
           The  filename of the executable, in parentheses.  This
           is visible whether or not the  executable  is  swapped
           out.

Additionally it should only be reporting the executable name rather
than the full path. Fix both these failings while cleaning up the code
to use GString to build up the reported values. As the whole function
is cleaned up also adjust the white space to the current coding style.

Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>
Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
Cc: Philippe_Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/syscall.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6495ddc4cda..674f70e70a5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd)
 {
     CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
     TaskState *ts = cpu->opaque;
-    abi_ulong start_stack = ts->info->start_stack;
+    g_autoptr(GString) buf = g_string_new(NULL);
     int i;
 
     for (i = 0; i < 44; i++) {
-      char buf[128];
-      int len;
-      uint64_t val = 0;
-
-      if (i == 0) {
-        /* pid */
-        val = getpid();
-        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
-      } else if (i == 1) {
-        /* app name */
-        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
-      } else if (i == 27) {
-        /* stack bottom */
-        val = start_stack;
-        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
-      } else {
-        /* for the rest, there is MasterCard */
-        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
-      }
+        if (i == 0) {
+            /* pid */
+            g_string_printf(buf, FMT_pid " ", getpid());
+        } else if (i == 1) {
+            /* app name */
+            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
+            bin = bin ? bin + 1 : ts->bprm->argv[0];
+            g_string_printf(buf, "(%.15s) ", bin);
+        } else if (i == 27) {
+            /* stack bottom */
+            g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack);
+        } else {
+            /* for the rest, there is MasterCard */
+            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
+        }
 
-      len = strlen(buf);
-      if (write(fd, buf, len) != len) {
-          return -1;
-      }
+        if (write(fd, buf->str, buf->len) != buf->len) {
+            return -1;
+        }
     }
 
     return 0;
-- 
2.20.1

[PATCH v1 11/11] .travis.yml: Build OSX 10.14 with Xcode 10.0

[Alex Bennée]

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

Travis recently made a change which generates various warnings
such [*]:

    CC      utils.o
  In file included from cs.c:11:
  In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/stdio.h:64:
  /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:93:16: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
          unsigned char   *_base;
                          ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:93:16: note: insert '_Nullable' if the pointer may be null
          unsigned char   *_base;
                          ^
                            _Nullable

We only aim to support MacOS 10.14 and 10.15. 10.14 comes with
Xcode 10.0. These warnings are not emitted with this Xcode version,
so switch back to it.

[*] https://travis-ci.org/github/qemu/qemu/jobs/673000302#L1387

Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200409190618.7402-1-philmd@redhat.com>
---
 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 2fd63eceaac..7c92206ea33 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -272,12 +272,12 @@ jobs:
 
     # MacOSX builds - cirrus.yml also tests some MacOS builds including latest Xcode
 
-    - name: "OSX Xcode 10.3"
+    - name: "OSX 10.14 (Xcode 10.0)"
       env:
         - BASE_CONFIG="--disable-docs --enable-tools"
         - CONFIG="--target-list=i386-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,x86_64-softmmu"
       os: osx
-      osx_image: xcode10.3
+      osx_image: xcode10
       compiler: clang
       addons:
         homebrew:
-- 
2.20.1

Re: [PATCH v1 07/11] configure: disable PIE for Windows builds

[Howard Spoelstra]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On Thu, Apr 9, 2020 at 11:18 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> It seems on some compilers the test can pass but still give you
> broken binaries.
>
> [AJB untested - please could windows users test]
>
> Fixes: d2cd29e30736
> Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
> Cc: Bug 1871798 <1871798@bugs.launchpad.net>
> Cc: James Le Cuirot <chewi@aura-online.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/configure b/configure
> index a207cce82bc..e9c5f630c14 100755
> --- a/configure
> +++ b/configure
> @@ -807,6 +807,7 @@ MINGW32*)
>      audio_drv_list=""
>    fi
>    supported_os="yes"
> +  pie="no"
>  ;;
>  GNU/kFreeBSD)
>    bsd="yes"
> --
> 2.20.1
>

Solves my issue! So,

Tested-by: Howard Spoelstra <hsp.cat7@gmail.com>

[-- Attachment #2: Type: text/html, Size: 1719 bytes --]

[Bug 1871798] Re: [PATCH v1 07/11] configure: disable PIE for Windows builds

[Howard Spoelstra]

On Thu, Apr 9, 2020 at 11:18 PM Alex Bennée <alex.bennee@linaro.org>
wrote:

> It seems on some compilers the test can pass but still give you
> broken binaries.
>
> [AJB untested - please could windows users test]
>
> Fixes: d2cd29e30736
> Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
> Cc: Bug 1871798 <1871798@bugs.launchpad.net>
> Cc: James Le Cuirot <chewi@aura-online.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/configure b/configure
> index a207cce82bc..e9c5f630c14 100755
> --- a/configure
> +++ b/configure
> @@ -807,6 +807,7 @@ MINGW32*)
>      audio_drv_list=""
>    fi
>    supported_os="yes"
> +  pie="no"
>  ;;
>  GNU/kFreeBSD)
>    bsd="yes"
> --
> 2.20.1
>

Solves my issue! So,

Tested-by: Howard Spoelstra <hsp.cat7@gmail.com>

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[Bug 1871798] Re: Fails to start on Windows host without explicit --disable-pie

[James Le Cuirot]

Tested and working. Thank you!

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

Re: [PATCH for 5.0-rc3 v1 00/11] more random fixes

[no-reply]

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



Hi,

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

Subject: [PATCH for 5.0-rc3 v1 00/11] more random fixes
Message-id: 20200409211529.5269-1-alex.bennee@linaro.org
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 ===

Switched to a new branch 'test'
2779bae .travis.yml: Build OSX 10.14 with Xcode 10.0
ffa69ee linux-user: fix /proc/self/stat handling
da73496 gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb
a7392db target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray
6cdbe4b configure: disable PIE for Windows builds
e70b41a configure: redirect sphinx-build check to config.log
84b0026 tests/docker: add docs FEATURE flag and use for test-misc
e991630 linux-user/ppc: Fix padding in mcontext_t for ppc64
3fe0015 accel/tcg: Relax va restrictions on 64-bit guests
3a50414 exec/cpu-all: Use bool for have_guest_base
8327d01 linux-user: completely re-write init_guest_space

=== OUTPUT BEGIN ===
1/11 Checking commit 8327d0183d84 (linux-user: completely re-write init_guest_space)
ERROR: trailing whitespace
#715: FILE: linux-user/qemu.h:229:
+ * $

total: 1 errors, 0 warnings, 681 lines checked

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

2/11 Checking commit 3a504143c770 (exec/cpu-all: Use bool for have_guest_base)
3/11 Checking commit 3fe00151c0e3 (accel/tcg: Relax va restrictions on 64-bit guests)
ERROR: Macros with complex values should be enclosed in parenthesis
#91: FILE: include/exec/cpu-all.h:182:
+# define GUEST_ADDR_MAX_  ~0ul

total: 1 errors, 0 warnings, 88 lines checked

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

4/11 Checking commit e99163099022 (linux-user/ppc: Fix padding in mcontext_t for ppc64)
5/11 Checking commit 84b002680ba0 (tests/docker: add docs FEATURE flag and use for test-misc)
6/11 Checking commit e70b41a4ac1e (configure: redirect sphinx-build check to config.log)
7/11 Checking commit 6cdbe4bcdb6b (configure: disable PIE for Windows builds)
8/11 Checking commit a7392db3e091 (target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray)
9/11 Checking commit da73496e5afd (gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb)
10/11 Checking commit ffa69ee03e63 (linux-user: fix /proc/self/stat handling)
11/11 Checking commit 2779baef4f14 (.travis.yml: Build OSX 10.14 with Xcode 10.0)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200409211529.5269-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 07/11] configure: disable PIE for Windows builds

[Philippe Mathieu-Daudé]

On 4/9/20 11:15 PM, Alex Bennée wrote:
> It seems on some compilers the test can pass but still give you
> broken binaries.
> 
> [AJB untested - please could windows users test]
> 
> Fixes: d2cd29e30736
> Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
> Cc: Bug 1871798 <1871798@bugs.launchpad.net>
> Cc: James Le Cuirot <chewi@aura-online.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   configure | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/configure b/configure
> index a207cce82bc..e9c5f630c14 100755
> --- a/configure
> +++ b/configure
> @@ -807,6 +807,7 @@ MINGW32*)
>       audio_drv_list=""
>     fi
>     supported_os="yes"
> +  pie="no"
>   ;;
>   GNU/kFreeBSD)
>     bsd="yes"
> 

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

[Bug 1871798] Re: [PATCH v1 07/11] configure: disable PIE for Windows builds

[Philippe Mathieu-Daudé]

On 4/9/20 11:15 PM, Alex Bennée wrote:
> It seems on some compilers the test can pass but still give you
> broken binaries.
> 
> [AJB untested - please could windows users test]
> 
> Fixes: d2cd29e30736
> Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
> Cc: Bug 1871798 <1871798@bugs.launchpad.net>
> Cc: James Le Cuirot <chewi@aura-online.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   configure | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/configure b/configure
> index a207cce82bc..e9c5f630c14 100755
> --- a/configure
> +++ b/configure
> @@ -807,6 +807,7 @@ MINGW32*)
>       audio_drv_list=""
>     fi
>     supported_os="yes"
> +  pie="no"
>   ;;
>   GNU/kFreeBSD)
>     bsd="yes"
> 

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

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

Re: [PATCH v1 06/11] configure: redirect sphinx-build check to config.log

[Philippe Mathieu-Daudé]

On 4/9/20 11:15 PM, Alex Bennée wrote:
> Otherwise it's hard to debug whats going on.
> 
> 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 233c671aaa9..a207cce82bc 100755
> --- a/configure
> +++ b/configure
> @@ -4936,7 +4936,7 @@ has_sphinx_build() {
>       # sphinx-build doesn't exist at all or if it is too old.
>       mkdir -p "$TMPDIR1/sphinx"
>       touch "$TMPDIR1/sphinx/index.rst"
> -    "$sphinx_build" -c "$source_path/docs" -b html "$TMPDIR1/sphinx" "$TMPDIR1/sphinx/out" >/dev/null 2>&1
> +    "$sphinx_build" -c "$source_path/docs" -b html "$TMPDIR1/sphinx" "$TMPDIR1/sphinx/out" >> config.log 2>&1
>   }
>   
>   # Check if tools are available to build documentation.
> 

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

Re: [PATCH v1 05/11] tests/docker: add docs FEATURE flag and use for test-misc

[Philippe Mathieu-Daudé]

On 4/9/20 11:15 PM, Alex Bennée wrote:
> The test-misc docker test fails on a number of images which don't have
> the prerequisites to build the docs. Use the FEATURES flag so we can
> skip those tests.
> 
> As the sphinx test fails to detect whatever feature we need to get
> hxtool to work we drop them from debian9 so the windows build doesn't
> attempt to build the docs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/docker/dockerfiles/debian10.docker   | 2 ++
>   tests/docker/dockerfiles/debian9.docker    | 2 --
>   tests/docker/dockerfiles/fedora.docker     | 2 +-
>   tests/docker/dockerfiles/travis.docker     | 2 +-
>   tests/docker/dockerfiles/ubuntu.docker     | 2 +-
>   tests/docker/dockerfiles/ubuntu1804.docker | 2 +-
>   tests/docker/test-misc                     | 2 ++
>   7 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
> index 2fcdc406e83..0769700a416 100644
> --- a/tests/docker/dockerfiles/debian10.docker
> +++ b/tests/docker/dockerfiles/debian10.docker
> @@ -34,3 +34,5 @@ RUN apt update && \
>           python3-sphinx \
>           texinfo \
>           $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
> +
> +ENV FEATURES docs
> diff --git a/tests/docker/dockerfiles/debian9.docker b/tests/docker/dockerfiles/debian9.docker
> index 92edbbf0f48..08cc970feb1 100644
> --- a/tests/docker/dockerfiles/debian9.docker
> +++ b/tests/docker/dockerfiles/debian9.docker
> @@ -30,6 +30,4 @@ RUN apt update && \
>           pkg-config \
>           psmisc \
>           python3 \
> -        python3-sphinx \
> -        texinfo \
>           $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
> diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
> index 4bd2c953af8..179575ecaaa 100644
> --- a/tests/docker/dockerfiles/fedora.docker
> +++ b/tests/docker/dockerfiles/fedora.docker
> @@ -103,4 +103,4 @@ ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3
>   RUN dnf install -y $PACKAGES
>   RUN rpm -q $PACKAGES | sort > /packages.txt
>   ENV PATH $PATH:/usr/libexec/python3-sphinx/
> -ENV FEATURES mingw clang pyyaml asan
> +ENV FEATURES mingw clang pyyaml asan docs
> diff --git a/tests/docker/dockerfiles/travis.docker b/tests/docker/dockerfiles/travis.docker
> index e8eb48dccfd..591282561bc 100644
> --- a/tests/docker/dockerfiles/travis.docker
> +++ b/tests/docker/dockerfiles/travis.docker
> @@ -13,5 +13,5 @@ RUN apt-get -y install device-tree-compiler python3 python3-yaml dh-autoreconf g
>   # Travis tools require PhantomJS / Neo4j / Maven accessible
>   # in their PATH (QEMU build won't access them).
>   ENV PATH /usr/local/phantomjs/bin:/usr/local/phantomjs:/usr/local/neo4j-3.2.7/bin:/usr/local/maven-3.5.2/bin:/usr/local/cmake-3.9.2/bin:/usr/local/clang-5.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
> -ENV FEATURES clang pyyaml
> +ENV FEATURES clang pyyaml docs
>   USER travis
> diff --git a/tests/docker/dockerfiles/ubuntu.docker b/tests/docker/dockerfiles/ubuntu.docker
> index b6c7b41dddd..eeb3b22bf20 100644
> --- a/tests/docker/dockerfiles/ubuntu.docker
> +++ b/tests/docker/dockerfiles/ubuntu.docker
> @@ -68,4 +68,4 @@ ENV PACKAGES flex bison \
>   RUN apt-get update && \
>       DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
>   RUN dpkg -l $PACKAGES | sort > /packages.txt
> -ENV FEATURES clang pyyaml sdl2
> +ENV FEATURES clang pyyaml sdl2 docs
> diff --git a/tests/docker/dockerfiles/ubuntu1804.docker b/tests/docker/dockerfiles/ubuntu1804.docker
> index 1efedeef995..f66b06f4cff 100644
> --- a/tests/docker/dockerfiles/ubuntu1804.docker
> +++ b/tests/docker/dockerfiles/ubuntu1804.docker
> @@ -54,7 +54,7 @@ ENV PACKAGES flex bison \
>   RUN apt-get update && \
>       DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
>   RUN dpkg -l $PACKAGES | sort > /packages.txt
> -ENV FEATURES clang pyyaml sdl2
> +ENV FEATURES clang pyyaml sdl2 docs
>   
>   # https://bugs.launchpad.net/qemu/+bug/1838763
>   ENV QEMU_CONFIGURE_OPTS --disable-libssh
> diff --git a/tests/docker/test-misc b/tests/docker/test-misc
> index d480afedca7..cc94a738dd0 100755
> --- a/tests/docker/test-misc
> +++ b/tests/docker/test-misc
> @@ -14,6 +14,8 @@
>   
>   . common.rc
>   
> +requires docs

TIL 'requires' macro. Clean fix.

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

> +
>   cd "$BUILD_DIR"
>   
>   # build everything else but QEMU
> 

Re: [PATCH v1 02/11] exec/cpu-all: Use bool for have_guest_base

[Philippe Mathieu-Daudé]

On 4/9/20 11:15 PM, Alex Bennée wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/exec/cpu-all.h | 2 +-
>   bsd-user/main.c        | 4 ++--
>   linux-user/main.c      | 4 ++--
>   3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 49384bb66a5..b4fb5832c4a 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -159,7 +159,7 @@ static inline void tswap64s(uint64_t *s)
>    * This allows the guest address space to be offset to a convenient location.
>    */
>   extern unsigned long guest_base;
> -extern int have_guest_base;
> +extern bool have_guest_base;
>   extern unsigned long reserved_va;
>   
>   #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 770c2b267ad..aef5531628a 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -42,7 +42,7 @@
>   int singlestep;
>   unsigned long mmap_min_addr;
>   unsigned long guest_base;
> -int have_guest_base;
> +bool have_guest_base;
>   unsigned long reserved_va;
>   
>   static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
> @@ -828,7 +828,7 @@ int main(int argc, char **argv)
>               }
>           } else if (!strcmp(r, "B")) {
>              guest_base = strtol(argv[optind++], NULL, 0);
> -           have_guest_base = 1;
> +           have_guest_base = true;
>           } else if (!strcmp(r, "drop-ld-preload")) {
>               (void) envlist_unsetenv(envlist, "LD_PRELOAD");
>           } else if (!strcmp(r, "bsd")) {
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 1d20a83d4e8..90ad365b439 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -59,7 +59,7 @@ static const char *cpu_type;
>   static const char *seed_optarg;
>   unsigned long mmap_min_addr;
>   unsigned long guest_base;
> -int have_guest_base;
> +bool have_guest_base;
>   
>   /*
>    * Used to implement backwards-compatibility for the `-strace`, and
> @@ -334,7 +334,7 @@ static void handle_arg_cpu(const char *arg)
>   static void handle_arg_guest_base(const char *arg)
>   {
>       guest_base = strtol(arg, NULL, 0);
> -    have_guest_base = 1;
> +    have_guest_base = true;
>   }
>   
>   static void handle_arg_reserved_va(const char *arg)
> 

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

Re: [PATCH v1 10/11] linux-user: fix /proc/self/stat handling

[Philippe Mathieu-Daudé]

Cc'ing Ludovic in case he can test with Guix-HPC.

On 4/9/20 11:15 PM, Alex Bennée wrote:
> In the original bug report long files names in Guix caused
> /proc/self/stat be truncated without the trailing ") " as specified in
> proc manpage which says:
>      (2) comm  %s
>             The  filename of the executable, in parentheses.  This
>             is visible whether or not the  executable  is  swapped
>             out.
> 
> Additionally it should only be reporting the executable name rather
> than the full path. Fix both these failings while cleaning up the code
> to use GString to build up the reported values. As the whole function
> is cleaned up also adjust the white space to the current coding style.
> 
> Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>
> Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
> Cc: Philippe_Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   linux-user/syscall.c | 43 +++++++++++++++++++------------------------
>   1 file changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 6495ddc4cda..674f70e70a5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd)
>   {
>       CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
>       TaskState *ts = cpu->opaque;
> -    abi_ulong start_stack = ts->info->start_stack;
> +    g_autoptr(GString) buf = g_string_new(NULL);
>       int i;
>   
>       for (i = 0; i < 44; i++) {
> -      char buf[128];
> -      int len;
> -      uint64_t val = 0;
> -
> -      if (i == 0) {
> -        /* pid */
> -        val = getpid();
> -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
> -      } else if (i == 1) {
> -        /* app name */
> -        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
> -      } else if (i == 27) {
> -        /* stack bottom */
> -        val = start_stack;
> -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
> -      } else {
> -        /* for the rest, there is MasterCard */
> -        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
> -      }
> +        if (i == 0) {
> +            /* pid */
> +            g_string_printf(buf, FMT_pid " ", getpid());
> +        } else if (i == 1) {
> +            /* app name */
> +            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
> +            bin = bin ? bin + 1 : ts->bprm->argv[0];
> +            g_string_printf(buf, "(%.15s) ", bin);

15 or 125? 15 seems short. From your previous test I understood it was 
124, for 
sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40").

> +        } else if (i == 27) {
> +            /* stack bottom */
> +            g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack);
> +        } else {
> +            /* for the rest, there is MasterCard */
> +            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
> +        }
>   
> -      len = strlen(buf);
> -      if (write(fd, buf, len) != len) {
> -          return -1;
> -      }
> +        if (write(fd, buf->str, buf->len) != buf->len) {
> +            return -1;
> +        }
>       }
>   
>       return 0;
> 

Re: [PATCH v1 10/11] linux-user: fix /proc/self/stat handling

[Alex Bennée]

[-- Attachment #1: Type: text/plain, Size: 3853 bytes --]

That was by inspection on my system which seems to truncate a lot earlier.
It would be nice to find where in the Linux kernel it is output but I
failed to grep the relevant function last night.

On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com>
wrote:

> Cc'ing Ludovic in case he can test with Guix-HPC.
>
> On 4/9/20 11:15 PM, Alex Bennée wrote:
> > In the original bug report long files names in Guix caused
> > /proc/self/stat be truncated without the trailing ") " as specified in
> > proc manpage which says:
> >      (2) comm  %s
> >             The  filename of the executable, in parentheses.  This
> >             is visible whether or not the  executable  is  swapped
> >             out.
> >
> > Additionally it should only be reporting the executable name rather
> > than the full path. Fix both these failings while cleaning up the code
> > to use GString to build up the reported values. As the whole function
> > is cleaned up also adjust the white space to the current coding style.
> >
> > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>
> > Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
> > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com>
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >   linux-user/syscall.c | 43 +++++++++++++++++++------------------------
> >   1 file changed, 19 insertions(+), 24 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 6495ddc4cda..674f70e70a5 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd)
> >   {
> >       CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
> >       TaskState *ts = cpu->opaque;
> > -    abi_ulong start_stack = ts->info->start_stack;
> > +    g_autoptr(GString) buf = g_string_new(NULL);
> >       int i;
> >
> >       for (i = 0; i < 44; i++) {
> > -      char buf[128];
> > -      int len;
> > -      uint64_t val = 0;
> > -
> > -      if (i == 0) {
> > -        /* pid */
> > -        val = getpid();
> > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
> > -      } else if (i == 1) {
> > -        /* app name */
> > -        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
> > -      } else if (i == 27) {
> > -        /* stack bottom */
> > -        val = start_stack;
> > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
> > -      } else {
> > -        /* for the rest, there is MasterCard */
> > -        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
> > -      }
> > +        if (i == 0) {
> > +            /* pid */
> > +            g_string_printf(buf, FMT_pid " ", getpid());
> > +        } else if (i == 1) {
> > +            /* app name */
> > +            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
> > +            bin = bin ? bin + 1 : ts->bprm->argv[0];
> > +            g_string_printf(buf, "(%.15s) ", bin);
>
> 15 or 125? 15 seems short. From your previous test I understood it was
> 124, for
>
> sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40").
>
> > +        } else if (i == 27) {
> > +            /* stack bottom */
> > +            g_string_printf(buf, TARGET_ABI_FMT_ld " ",
> ts->info->start_stack);
> > +        } else {
> > +            /* for the rest, there is MasterCard */
> > +            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
> > +        }
> >
> > -      len = strlen(buf);
> > -      if (write(fd, buf, len) != len) {
> > -          return -1;
> > -      }
> > +        if (write(fd, buf->str, buf->len) != buf->len) {
> > +            return -1;
> > +        }
> >       }
> >
> >       return 0;
> >
>
>

[-- Attachment #2: Type: text/html, Size: 5335 bytes --]

Re: [PATCH v1 10/11] linux-user: fix /proc/self/stat handling

[Philippe Mathieu-Daudé]

On 4/10/20 2:33 PM, Alex Bennée wrote:
> That was by inspection on my system which seems to truncate a lot 
> earlier. It would be nice to find where in the Linux kernel it is output 
> but I failed to grep the relevant function last night.

OK. Patch is correct with this value, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com 
> <mailto:philmd@redhat.com>> wrote:
> 
>     Cc'ing Ludovic in case he can test with Guix-HPC.
> 
>     On 4/9/20 11:15 PM, Alex Bennée wrote:
>      > In the original bug report long files names in Guix caused
>      > /proc/self/stat be truncated without the trailing ") " as
>     specified in
>      > proc manpage which says:
>      >      (2) comm  %s
>      >             The  filename of the executable, in parentheses.  This
>      >             is visible whether or not the  executable  is  swapped
>      >             out.
>      >
>      > Additionally it should only be reporting the executable name rather
>      > than the full path. Fix both these failings while cleaning up the
>     code
>      > to use GString to build up the reported values. As the whole function
>      > is cleaned up also adjust the white space to the current coding
>     style.
>      >
>      > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr
>     <mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>>
>      > Reported-by: Brice Goglin <Brice.Goglin@inria.fr
>     <mailto:Brice.Goglin@inria.fr>>
>      > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>      > Signed-off-by: Alex Bennée <alex.bennee@linaro.org
>     <mailto:alex.bennee@linaro.org>>
>      > ---
>      >   linux-user/syscall.c | 43
>     +++++++++++++++++++------------------------
>      >   1 file changed, 19 insertions(+), 24 deletions(-)
>      >
>      > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>      > index 6495ddc4cda..674f70e70a5 100644
>      > --- a/linux-user/syscall.c
>      > +++ b/linux-user/syscall.c
>      > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env,
>     int fd)
>      >   {
>      >       CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
>      >       TaskState *ts = cpu->opaque;
>      > -    abi_ulong start_stack = ts->info->start_stack;
>      > +    g_autoptr(GString) buf = g_string_new(NULL);
>      >       int i;
>      >
>      >       for (i = 0; i < 44; i++) {
>      > -      char buf[128];
>      > -      int len;
>      > -      uint64_t val = 0;
>      > -
>      > -      if (i == 0) {
>      > -        /* pid */
>      > -        val = getpid();
>      > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
>      > -      } else if (i == 1) {
>      > -        /* app name */
>      > -        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
>      > -      } else if (i == 27) {
>      > -        /* stack bottom */
>      > -        val = start_stack;
>      > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
>      > -      } else {
>      > -        /* for the rest, there is MasterCard */
>      > -        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
>      > -      }
>      > +        if (i == 0) {
>      > +            /* pid */
>      > +            g_string_printf(buf, FMT_pid " ", getpid());
>      > +        } else if (i == 1) {
>      > +            /* app name */
>      > +            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
>      > +            bin = bin ? bin + 1 : ts->bprm->argv[0];
>      > +            g_string_printf(buf, "(%.15s) ", bin);
> 
>     15 or 125? 15 seems short. From your previous test I understood it was
>     124, for
>     sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40").
> 
>      > +        } else if (i == 27) {
>      > +            /* stack bottom */
>      > +            g_string_printf(buf, TARGET_ABI_FMT_ld " ",
>     ts->info->start_stack);
>      > +        } else {
>      > +            /* for the rest, there is MasterCard */

Already 23 years =)

>      > +            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
>      > +        }
>      >
>      > -      len = strlen(buf);
>      > -      if (write(fd, buf, len) != len) {
>      > -          return -1;
>      > -      }
>      > +        if (write(fd, buf->str, buf->len) != buf->len) {
>      > +            return -1;
>      > +        }
>      >       }
>      >
>      >       return 0;
>      >
> 

Re: [PATCH v1 09/11] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Stefano Garzarella]

On Thu, Apr 09, 2020 at 10:15:27PM +0100, Alex Bennée wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> We should only pass in gdb_get_reg16() with the GByteArray* object
> itself, no need to shift.  Without this patch, gdb remote attach will
> crash QEMU.
> 
> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200409164954.36902-3-peterx@redhat.com>
> ---
>  target/i386/gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index f3d23b614ee..b98a99500ae 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
>          floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
>          int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
> -        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
> +        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
>          return len;
>      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>          n -= IDX_XMM_REGS;
> -- 
> 2.20.1
> 
>

I had the following issue while attaching to qemu started with gdbserver
listening:

(gdb) target remote :1234
Remote debugging using :1234
Remote communication error.  Target disconnected.: Connection reset by peer.

$ qemu-system-x86_64 -m 1G -smp 4 ... -s
ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)
Bail out! ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)


Thanks to Philippe, I tried this patch and it solves my issue:

Tested-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [PATCH v1 10/11] linux-user: fix /proc/self/stat handling

[Brice Goglin]

[-- Attachment #1: Type: text/plain, Size: 4819 bytes --]

Le 10/04/2020 à 14:33, Alex Bennée a écrit :
> That was by inspection on my system which seems to truncate a lot
> earlier. It would be nice to find where in the Linux kernel it is
> output but I failed to grep the relevant function last night.


It's in proc/array.c, do_task_stat() calls proc_task_name(). In the end,
it seems to use task->tcomm or task->comm which is limited by

#define TASK_COMM_LEN			16

Brice



>
> On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com
> <mailto:philmd@redhat.com>> wrote:
>
>     Cc'ing Ludovic in case he can test with Guix-HPC.
>
>     On 4/9/20 11:15 PM, Alex Bennée wrote:
>     > In the original bug report long files names in Guix caused
>     > /proc/self/stat be truncated without the trailing ") " as
>     specified in
>     > proc manpage which says:
>     >      (2) comm  %s
>     >             The  filename of the executable, in parentheses.  This
>     >             is visible whether or not the  executable  is  swapped
>     >             out.
>     >
>     > Additionally it should only be reporting the executable name rather
>     > than the full path. Fix both these failings while cleaning up
>     the code
>     > to use GString to build up the reported values. As the whole
>     function
>     > is cleaned up also adjust the white space to the current coding
>     style.
>     >
>     > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr
>     <mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>>
>     > Reported-by: Brice Goglin <Brice.Goglin@inria.fr
>     <mailto:Brice.Goglin@inria.fr>>
>     > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     > Signed-off-by: Alex Bennée <alex.bennee@linaro.org
>     <mailto:alex.bennee@linaro.org>>
>     > ---
>     >   linux-user/syscall.c | 43
>     +++++++++++++++++++------------------------
>     >   1 file changed, 19 insertions(+), 24 deletions(-)
>     >
>     > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>     > index 6495ddc4cda..674f70e70a5 100644
>     > --- a/linux-user/syscall.c
>     > +++ b/linux-user/syscall.c
>     > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env,
>     int fd)
>     >   {
>     >       CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
>     >       TaskState *ts = cpu->opaque;
>     > -    abi_ulong start_stack = ts->info->start_stack;
>     > +    g_autoptr(GString) buf = g_string_new(NULL);
>     >       int i;
>     >   
>     >       for (i = 0; i < 44; i++) {
>     > -      char buf[128];
>     > -      int len;
>     > -      uint64_t val = 0;
>     > -
>     > -      if (i == 0) {
>     > -        /* pid */
>     > -        val = getpid();
>     > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
>     > -      } else if (i == 1) {
>     > -        /* app name */
>     > -        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
>     > -      } else if (i == 27) {
>     > -        /* stack bottom */
>     > -        val = start_stack;
>     > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
>     > -      } else {
>     > -        /* for the rest, there is MasterCard */
>     > -        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
>     > -      }
>     > +        if (i == 0) {
>     > +            /* pid */
>     > +            g_string_printf(buf, FMT_pid " ", getpid());
>     > +        } else if (i == 1) {
>     > +            /* app name */
>     > +            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
>     > +            bin = bin ? bin + 1 : ts->bprm->argv[0];
>     > +            g_string_printf(buf, "(%.15s) ", bin);
>
>     15 or 125? 15 seems short. From your previous test I understood it
>     was
>     124, for
>     sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40").
>
>     > +        } else if (i == 27) {
>     > +            /* stack bottom */
>     > +            g_string_printf(buf, TARGET_ABI_FMT_ld " ",
>     ts->info->start_stack);
>     > +        } else {
>     > +            /* for the rest, there is MasterCard */
>     > +            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
>     > +        }
>     >   
>     > -      len = strlen(buf);
>     > -      if (write(fd, buf, len) != len) {
>     > -          return -1;
>     > -      }
>     > +        if (write(fd, buf->str, buf->len) != buf->len) {
>     > +            return -1;
>     > +        }
>     >       }
>     >   
>     >       return 0;
>     >
>

[-- Attachment #2: Type: text/html, Size: 7309 bytes --]

Re: [PATCH v1 06/11] configure: redirect sphinx-build check to config.log

[Richard Henderson]

On 4/9/20 2:15 PM, Alex Bennée wrote:
> Otherwise it's hard to debug whats going on.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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


r~

Re: [PATCH v1 05/11] tests/docker: add docs FEATURE flag and use for test-misc

[Richard Henderson]

On 4/9/20 2:15 PM, Alex Bennée wrote:
> The test-misc docker test fails on a number of images which don't have
> the prerequisites to build the docs. Use the FEATURES flag so we can
> skip those tests.
> 
> As the sphinx test fails to detect whatever feature we need to get
> hxtool to work we drop them from debian9 so the windows build doesn't
> attempt to build the docs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/docker/dockerfiles/debian10.docker   | 2 ++
>  tests/docker/dockerfiles/debian9.docker    | 2 --
>  tests/docker/dockerfiles/fedora.docker     | 2 +-
>  tests/docker/dockerfiles/travis.docker     | 2 +-
>  tests/docker/dockerfiles/ubuntu.docker     | 2 +-
>  tests/docker/dockerfiles/ubuntu1804.docker | 2 +-
>  tests/docker/test-misc                     | 2 ++
>  7 files changed, 8 insertions(+), 6 deletions(-)

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


r~

Re: [PATCH v1 07/11] configure: disable PIE for Windows builds

[Richard Henderson]

On 4/9/20 2:15 PM, Alex Bennée wrote:
> It seems on some compilers the test can pass but still give you
> broken binaries.
> 
> [AJB untested - please could windows users test]
> 
> Fixes: d2cd29e30736
> Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
> Cc: Bug 1871798 <1871798@bugs.launchpad.net>
> Cc: James Le Cuirot <chewi@aura-online.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)

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


r~

Re: [PATCH v1 09/11] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Richard Henderson]

On 4/9/20 2:15 PM, Alex Bennée wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> We should only pass in gdb_get_reg16() with the GByteArray* object
> itself, no need to shift.  Without this patch, gdb remote attach will
> crash QEMU.
> 
> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200409164954.36902-3-peterx@redhat.com>
> ---
>  target/i386/gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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


r~

Re: [PATCH v1 08/11] target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray

[Richard Henderson]

On 4/9/20 2:15 PM, Alex Bennée wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Since a010bdbe719 the gdbstub API takes a GByteArray*. Unfortunately
> we forgot to update the gdb_get_reg*() calls. Do it now.
> 
> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200409172509.4078-1-philmd@redhat.com>
> ---
>  target/m68k/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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


r~

Re: [PATCH v1 10/11] linux-user: fix /proc/self/stat handling

[Richard Henderson]

On 4/9/20 2:15 PM, Alex Bennée wrote:
> In the original bug report long files names in Guix caused
> /proc/self/stat be truncated without the trailing ") " as specified in
> proc manpage which says:
>     (2) comm  %s
>            The  filename of the executable, in parentheses.  This
>            is visible whether or not the  executable  is  swapped
>            out.
> 
> Additionally it should only be reporting the executable name rather
> than the full path. Fix both these failings while cleaning up the code
> to use GString to build up the reported values. As the whole function
> is cleaned up also adjust the white space to the current coding style.
> 
> Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>
> Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
> Cc: Philippe_Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/syscall.c | 43 +++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)

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


r~

Re: [PATCH v1 09/11] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Alex Bennée]

Stefano Garzarella <sgarzare@redhat.com> writes:

> On Thu, Apr 09, 2020 at 10:15:27PM +0100, Alex Bennée wrote:
>> From: Peter Xu <peterx@redhat.com>
>> 
>> We should only pass in gdb_get_reg16() with the GByteArray* object
>> itself, no need to shift.  Without this patch, gdb remote attach will
>> crash QEMU.
>> 
>> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20200409164954.36902-3-peterx@redhat.com>
>> ---
>>  target/i386/gdbstub.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
>> index f3d23b614ee..b98a99500ae 100644
>> --- a/target/i386/gdbstub.c
>> +++ b/target/i386/gdbstub.c
>> @@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>>      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
>>          floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
>>          int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
>> -        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
>> +        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
>>          return len;
>>      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>>          n -= IDX_XMM_REGS;
>> -- 
>> 2.20.1
>> 
>>
>
> I had the following issue while attaching to qemu started with gdbserver
> listening:
>
> (gdb) target remote :1234
> Remote debugging using :1234
> Remote communication error.  Target disconnected.: Connection reset by peer.
>
> $ qemu-system-x86_64 -m 1G -smp 4 ... -s
> ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)
> Bail out! ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion
> failed: (len == gdbserver_state.mem_buf->len)

I'll see if the new gdb testcases can be generalised - they would have
caught these snafus.

>
>
> Thanks to Philippe, I tried this patch and it solves my issue:
>
> Tested-by: Stefano Garzarella <sgarzare@redhat.com>
>
> Thanks,
> Stefano


-- 
Alex Bennée

Re: [PATCH v1 10/11] linux-user: fix /proc/self/stat handling

[Alex Bennée]

Brice Goglin <Brice.Goglin@inria.fr> writes:

> Le 10/04/2020 à 14:33, Alex Bennée a écrit :
>> That was by inspection on my system which seems to truncate a lot
>> earlier. It would be nice to find where in the Linux kernel it is
>> output but I failed to grep the relevant function last night.
>
>
> It's in proc/array.c, do_task_stat() calls proc_task_name(). In the end,
> it seems to use task->tcomm or task->comm which is limited by
>
> #define TASK_COMM_LEN			16

Thanks. I'll amend the commit message. Are you happy with the fix on
your end?

>
> Brice
>
>
>
>>
>> On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com
>> <mailto:philmd@redhat.com>> wrote:
>>
>>     Cc'ing Ludovic in case he can test with Guix-HPC.
>>
>>     On 4/9/20 11:15 PM, Alex Bennée wrote:
>>     > In the original bug report long files names in Guix caused
>>     > /proc/self/stat be truncated without the trailing ") " as
>>     specified in
>>     > proc manpage which says:
>>     >      (2) comm  %s
>>     >             The  filename of the executable, in parentheses.  This
>>     >             is visible whether or not the  executable  is  swapped
>>     >             out.
>>     >
>>     > Additionally it should only be reporting the executable name rather
>>     > than the full path. Fix both these failings while cleaning up
>>     the code
>>     > to use GString to build up the reported values. As the whole
>>     function
>>     > is cleaned up also adjust the white space to the current coding
>>     style.
>>     >
>>     > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr
>>     <mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>>
>>     > Reported-by: Brice Goglin <Brice.Goglin@inria.fr
>>     <mailto:Brice.Goglin@inria.fr>>
>>     > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com
>>     <mailto:philmd@redhat.com>>
>>     > Signed-off-by: Alex Bennée <alex.bennee@linaro.org
>>     <mailto:alex.bennee@linaro.org>>
>>     > ---
>>     >   linux-user/syscall.c | 43
>>     +++++++++++++++++++------------------------
>>     >   1 file changed, 19 insertions(+), 24 deletions(-)
>>     >
>>     > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>     > index 6495ddc4cda..674f70e70a5 100644
>>     > --- a/linux-user/syscall.c
>>     > +++ b/linux-user/syscall.c
>>     > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env,
>>     int fd)
>>     >   {
>>     >       CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
>>     >       TaskState *ts = cpu->opaque;
>>     > -    abi_ulong start_stack = ts->info->start_stack;
>>     > +    g_autoptr(GString) buf = g_string_new(NULL);
>>     >       int i;
>>     >   
>>     >       for (i = 0; i < 44; i++) {
>>     > -      char buf[128];
>>     > -      int len;
>>     > -      uint64_t val = 0;
>>     > -
>>     > -      if (i == 0) {
>>     > -        /* pid */
>>     > -        val = getpid();
>>     > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
>>     > -      } else if (i == 1) {
>>     > -        /* app name */
>>     > -        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
>>     > -      } else if (i == 27) {
>>     > -        /* stack bottom */
>>     > -        val = start_stack;
>>     > -        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
>>     > -      } else {
>>     > -        /* for the rest, there is MasterCard */
>>     > -        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
>>     > -      }
>>     > +        if (i == 0) {
>>     > +            /* pid */
>>     > +            g_string_printf(buf, FMT_pid " ", getpid());
>>     > +        } else if (i == 1) {
>>     > +            /* app name */
>>     > +            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
>>     > +            bin = bin ? bin + 1 : ts->bprm->argv[0];
>>     > +            g_string_printf(buf, "(%.15s) ", bin);
>>
>>     15 or 125? 15 seems short. From your previous test I understood it
>>     was
>>     124, for
>>     sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40").
>>
>>     > +        } else if (i == 27) {
>>     > +            /* stack bottom */
>>     > +            g_string_printf(buf, TARGET_ABI_FMT_ld " ",
>>     ts->info->start_stack);
>>     > +        } else {
>>     > +            /* for the rest, there is MasterCard */
>>     > +            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
>>     > +        }
>>     >   
>>     > -      len = strlen(buf);
>>     > -      if (write(fd, buf, len) != len) {
>>     > -          return -1;
>>     > -      }
>>     > +        if (write(fd, buf->str, buf->len) != buf->len) {
>>     > +            return -1;
>>     > +        }
>>     >       }
>>     >   
>>     >       return 0;
>>     >
>>


-- 
Alex Bennée

Re: [PATCH v1 09/11] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Philippe Mathieu-Daudé]

On 4/10/20 3:08 PM, Stefano Garzarella wrote:
> On Thu, Apr 09, 2020 at 10:15:27PM +0100, Alex Bennée wrote:
>> From: Peter Xu <peterx@redhat.com>
>>
>> We should only pass in gdb_get_reg16() with the GByteArray* object
>> itself, no need to shift.  Without this patch, gdb remote attach will
>> crash QEMU.
>>
>> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20200409164954.36902-3-peterx@redhat.com>
>> ---
>>   target/i386/gdbstub.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
>> index f3d23b614ee..b98a99500ae 100644
>> --- a/target/i386/gdbstub.c
>> +++ b/target/i386/gdbstub.c
>> @@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>>       } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
>>           floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
>>           int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
>> -        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
>> +        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
>>           return len;
>>       } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>>           n -= IDX_XMM_REGS;
>> -- 
>> 2.20.1
>>
>>
> 
> I had the following issue while attaching to qemu started with gdbserver
> listening:
> 

Alex, if possible can you amend this info please?

<---

> (gdb) target remote :1234
> Remote debugging using :1234
> Remote communication error.  Target disconnected.: Connection reset by peer.
> 
> $ qemu-system-x86_64 -m 1G -smp 4 ... -s
> ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)
> Bail out! ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)

--->

Thanks!

> 
> 
> Thanks to Philippe, I tried this patch and it solves my issue:
> 
> Tested-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> Thanks,
> Stefano
> 
> 

Re: [PATCH v1 09/11] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Stefano Garzarella]

On Sat, Apr 11, 2020 at 01:58:07PM +0100, Alex Bennée wrote:
> 
> Stefano Garzarella <sgarzare@redhat.com> writes:
> 
> > On Thu, Apr 09, 2020 at 10:15:27PM +0100, Alex Bennée wrote:
> >> From: Peter Xu <peterx@redhat.com>
> >> 
> >> We should only pass in gdb_get_reg16() with the GByteArray* object
> >> itself, no need to shift.  Without this patch, gdb remote attach will
> >> crash QEMU.
> >> 
> >> Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
> >> Signed-off-by: Peter Xu <peterx@redhat.com>
> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Message-Id: <20200409164954.36902-3-peterx@redhat.com>
> >> ---
> >>  target/i386/gdbstub.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> >> index f3d23b614ee..b98a99500ae 100644
> >> --- a/target/i386/gdbstub.c
> >> +++ b/target/i386/gdbstub.c
> >> @@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
> >>      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> >>          floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
> >>          int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
> >> -        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
> >> +        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
> >>          return len;
> >>      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
> >>          n -= IDX_XMM_REGS;
> >> -- 
> >> 2.20.1
> >> 
> >>
> >
> > I had the following issue while attaching to qemu started with gdbserver
> > listening:
> >
> > (gdb) target remote :1234
> > Remote debugging using :1234
> > Remote communication error.  Target disconnected.: Connection reset by peer.
> >
> > $ qemu-system-x86_64 -m 1G -smp 4 ... -s
> > ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)
> > Bail out! ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion
> > failed: (len == gdbserver_state.mem_buf->len)
> 
> I'll see if the new gdb testcases can be generalised - they would have
> caught these snafus.

Yeah, that would be great!

Thanks,
Stefano

Re: [PATCH v1 11/11] .travis.yml: Build OSX 10.14 with Xcode 10.0

[Daniel P. Berrangé]

On Thu, Apr 09, 2020 at 10:15:29PM +0100, Alex Bennée wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Travis recently made a change which generates various warnings
> such [*]:
> 
>     CC      utils.o
>   In file included from cs.c:11:
>   In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/stdio.h:64:
>   /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:93:16: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
>           unsigned char   *_base;
>                           ^
>   /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:93:16: note: insert '_Nullable' if the pointer may be null
>           unsigned char   *_base;
>                           ^
>                             _Nullable
> 
> We only aim to support MacOS 10.14 and 10.15. 10.14 comes with
> Xcode 10.0. These warnings are not emitted with this Xcode version,
> so switch back to it.

It appears Travis have fixed the flaw in 10.3, as both QEMU and libvirt
are building correctly on 10.3 again. So I think you can drop this patch.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 for 5.0-rc3 00/17] more randome fixes (user, pie, docker and gdbstub)

[Alex Bennée]

Hi,

This is version 2 of my random fixes for rc3 series. In no particular
order we have:

  - A trivial fix to /proc/self/stat output
  - An attempt to fix broken PIE builds for Windows (please test!)
  - Some fixes to get "make docker-all-tests" running again
  - A tweak to .gitignore 
  - A couple of bugs found in the gdbstub GByteArray conversion
  - A new test case for linux-user gdbstub support
  - Which found a few more gdbstub bugs :-/

The m68k FPU patch is definitely a hack but it seemed easier than
disabling completely. The chatter on #gdb seems to be there is some
combination of qemu and gdb tress that does work but I suspect this
needs some maintainer action.

I've dropped the MacOSX travis patch as Travis has hopefully sorted
itself out.

I've also included the guest base re-factoring patches as it makes it
easier for me to soak test the tree with the sanitiser although those
actual fixes won't go into 5.0 at this late stage.

The following patches need review:

 - tests/tcg: add a multiarch linux-user gdb test
 - tests/tcg: drop inferior.was_attached() test
 - target/m68k: hack around the FPU register support (HACK!)
 - linux-user: completely re-write init_guest_space

Alex Bennée (9):
  linux-user: completely re-write init_guest_space
  .gitignore: include common build sub-directories
  tests/docker: add docs FEATURE flag and use for test-misc
  configure: redirect sphinx-build check to config.log
  configure: disable PIE for Windows builds
  linux-user: fix /proc/self/stat handling
  target/m68k: hack around the FPU register support (HACK!)
  tests/tcg: drop inferior.was_attached() test
  tests/tcg: add a multiarch linux-user gdb test

Peter Xu (1):
  gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

Philippe Mathieu-Daudé (4):
  target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray
  gdbstub: Do not use memset() on GByteArray
  gdbstub: Introduce gdb_get_float32() to get 32-bit float registers
  gdbstub: Introduce gdb_get_float64() to get 64-bit float registers

Richard Henderson (3):
  exec/cpu-all: Use bool for have_guest_base
  accel/tcg: Relax va restrictions on 64-bit guests
  linux-user/ppc: Fix padding in mcontext_t for ppc64

 configure                                   |   5 +-
 include/exec/cpu-all.h                      |  25 +-
 include/exec/gdbstub.h                      |  29 ++
 linux-user/qemu.h                           |  31 +-
 target/alpha/cpu-param.h                    |  15 +-
 accel/tcg/translate-all.c                   |  15 +-
 bsd-user/main.c                             |   4 +-
 linux-user/elfload.c                        | 503 ++++++++++----------
 linux-user/flatload.c                       |   6 +
 linux-user/main.c                           |  27 +-
 linux-user/ppc/signal.c                     |  69 ++-
 linux-user/syscall.c                        |  43 +-
 target/arm/gdbstub.c                        |   3 +-
 target/i386/gdbstub.c                       |   2 +-
 target/m68k/helper.c                        |  14 +-
 target/ppc/gdbstub.c                        |   4 +-
 target/ppc/translate_init.inc.c             |   2 +-
 target/sh4/gdbstub.c                        |   6 +-
 target/xtensa/gdbstub.c                     |   6 +-
 .gitignore                                  |   1 +
 gdb-xml/m68k-fp.xml                         |  16 +-
 tests/docker/dockerfiles/debian10.docker    |   2 +
 tests/docker/dockerfiles/debian9.docker     |   2 -
 tests/docker/dockerfiles/fedora.docker      |   2 +-
 tests/docker/dockerfiles/travis.docker      |   2 +-
 tests/docker/dockerfiles/ubuntu.docker      |   2 +-
 tests/docker/dockerfiles/ubuntu1804.docker  |   2 +-
 tests/docker/test-misc                      |   2 +
 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py |   3 -
 tests/tcg/aarch64/gdbstub/test-sve.py       |   3 -
 tests/tcg/multiarch/Makefile.target         |  15 +
 tests/tcg/multiarch/gdbstub/sha1.py         |  81 ++++
 32 files changed, 522 insertions(+), 420 deletions(-)
 create mode 100644 tests/tcg/multiarch/gdbstub/sha1.py

-- 
2.20.1

[PATCH v2 01/17] linux-user: completely re-write init_guest_space

[Alex Bennée]

First we ensure all guest space initialisation logic comes through
probe_guest_base once we understand the nature of the binary we are
loading. The convoluted init_guest_space routine is removed and
replaced with a number of pgb_* helpers which are called depending on
what requirements we have when loading the binary.

We first try to do what is requested by the host. Failing that we try
and satisfy the guest requested base address. If all those options
fail we fall back to finding a space in the memory map using our
recently written read_self_maps() helper.

There are some additional complications we try and take into account
when looking for holes in the address space. We try not to go directly
after the system brk() space so there is space for a little growth. We
also don't want to have to use negative offsets which would result in
slightly less efficient code on x86 when it's unable to use the
segment offset register.

Less mind-binding gotos and hopefully clearer logic throughout.

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

---
v3
  - include rth updates that
    - split probe_guest_base into multiple functions
    - more heuristics on gap finding
v4
  - whitespace fix
---
 linux-user/qemu.h     |  31 ++-
 linux-user/elfload.c  | 503 +++++++++++++++++++++---------------------
 linux-user/flatload.c |   6 +
 linux-user/main.c     |  23 +-
 4 files changed, 277 insertions(+), 286 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 792c74290f8..ce902f5132a 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -219,18 +219,27 @@ void init_qemu_uname_release(void);
 void fork_start(void);
 void fork_end(int child);
 
-/* Creates the initial guest address space in the host memory space using
- * the given host start address hint and size.  The guest_start parameter
- * specifies the start address of the guest space.  guest_base will be the
- * difference between the host start address computed by this function and
- * guest_start.  If fixed is specified, then the mapped address space must
- * start at host_start.  The real start address of the mapped memory space is
- * returned or -1 if there was an error.
+/**
+ * probe_guest_base:
+ * @image_name: the executable being loaded
+ * @loaddr: the lowest fixed address in the executable
+ * @hiaddr: the highest fixed address in the executable
+ *
+ * Creates the initial guest address space in the host memory space.
+ *
+ * If @loaddr == 0, then no address in the executable is fixed,
+ * i.e. it is fully relocatable.  In that case @hiaddr is the size
+ * of the executable.
+ *
+ * This function will not return if a valid value for guest_base
+ * cannot be chosen.  On return, the executable loader can expect
+ *
+ *    target_mmap(loaddr, hiaddr - loaddr, ...)
+ *
+ * to succeed.
  */
-unsigned long init_guest_space(unsigned long host_start,
-                               unsigned long host_size,
-                               unsigned long guest_start,
-                               bool fixed);
+void probe_guest_base(const char *image_name,
+                      abi_ulong loaddr, abi_ulong hiaddr);
 
 #include "qemu/log.h"
 
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 619c054cc48..01a9323a637 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -11,6 +11,7 @@
 #include "qemu/queue.h"
 #include "qemu/guest-random.h"
 #include "qemu/units.h"
+#include "qemu/selfmap.h"
 
 #ifdef _ARCH_PPC64
 #undef ARCH_DLINFO
@@ -382,68 +383,30 @@ enum {
 
 /* The commpage only exists for 32 bit kernels */
 
-/* Return 1 if the proposed guest space is suitable for the guest.
- * Return 0 if the proposed guest space isn't suitable, but another
- * address space should be tried.
- * Return -1 if there is no way the proposed guest space can be
- * valid regardless of the base.
- * The guest code may leave a page mapped and populate it if the
- * address is suitable.
- */
-static int init_guest_commpage(unsigned long guest_base,
-                               unsigned long guest_size)
-{
-    unsigned long real_start, test_page_addr;
-
-    /* We need to check that we can force a fault on access to the
-     * commpage at 0xffff0fxx
-     */
-    test_page_addr = guest_base + (0xffff0f00 & qemu_host_page_mask);
-
-    /* If the commpage lies within the already allocated guest space,
-     * then there is no way we can allocate it.
-     *
-     * You may be thinking that that this check is redundant because
-     * we already validated the guest size against MAX_RESERVED_VA;
-     * but if qemu_host_page_mask is unusually large, then
-     * test_page_addr may be lower.
-     */
-    if (test_page_addr >= guest_base
-        && test_page_addr < (guest_base + guest_size)) {
-        return -1;
-    }
+#define ARM_COMMPAGE (intptr_t)0xffff0f00u
 
-    /* Note it needs to be writeable to let us initialise it */
-    real_start = (unsigned long)
-                 mmap((void *)test_page_addr, qemu_host_page_size,
-                     PROT_READ | PROT_WRITE,
-                     MAP_ANONYMOUS | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+static bool init_guest_commpage(void)
+{
+    void *want = g2h(ARM_COMMPAGE & -qemu_host_page_size);
+    void *addr = mmap(want, qemu_host_page_size, PROT_READ | PROT_WRITE,
+                      MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
 
-    /* If we can't map it then try another address */
-    if (real_start == -1ul) {
-        return 0;
+    if (addr == MAP_FAILED) {
+        perror("Allocating guest commpage");
+        exit(EXIT_FAILURE);
     }
-
-    if (real_start != test_page_addr) {
-        /* OS didn't put the page where we asked - unmap and reject */
-        munmap((void *)real_start, qemu_host_page_size);
-        return 0;
+    if (addr != want) {
+        return false;
     }
 
-    /* Leave the page mapped
-     * Populate it (mmap should have left it all 0'd)
-     */
-
-    /* Kernel helper versions */
-    __put_user(5, (uint32_t *)g2h(0xffff0ffcul));
+    /* Set kernel helper versions; rest of page is 0.  */
+    __put_user(5, (uint32_t *)g2h(0xffff0ffcu));
 
-    /* Now it's populated make it RO */
-    if (mprotect((void *)test_page_addr, qemu_host_page_size, PROT_READ)) {
+    if (mprotect(addr, qemu_host_page_size, PROT_READ)) {
         perror("Protecting guest commpage");
-        exit(-1);
+        exit(EXIT_FAILURE);
     }
-
-    return 1; /* All good */
+    return true;
 }
 
 #define ELF_HWCAP get_elf_hwcap()
@@ -2075,239 +2038,267 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
     return sp;
 }
 
-unsigned long init_guest_space(unsigned long host_start,
-                               unsigned long host_size,
-                               unsigned long guest_start,
-                               bool fixed)
-{
-    /* In order to use host shmat, we must be able to honor SHMLBA.  */
-    unsigned long align = MAX(SHMLBA, qemu_host_page_size);
-    unsigned long current_start, aligned_start;
-    int flags;
-
-    assert(host_start || host_size);
-
-    /* If just a starting address is given, then just verify that
-     * address.  */
-    if (host_start && !host_size) {
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-        if (init_guest_commpage(host_start, host_size) != 1) {
-            return (unsigned long)-1;
-        }
+#ifndef ARM_COMMPAGE
+#define ARM_COMMPAGE 0
+#define init_guest_commpage() true
 #endif
-        return host_start;
-    }
 
-    /* Setup the initial flags and start address.  */
-    current_start = host_start & -align;
-    flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
-    if (fixed) {
-        flags |= MAP_FIXED;
-    }
+static void pgb_fail_in_use(const char *image_name)
+{
+    error_report("%s: requires virtual address space that is in use "
+                 "(omit the -B option or choose a different value)",
+                 image_name);
+    exit(EXIT_FAILURE);
+}
 
-    /* Otherwise, a non-zero size region of memory needs to be mapped
-     * and validated.  */
+static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr,
+                                abi_ulong guest_hiaddr, long align)
+{
+    const int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
+    void *addr, *test;
 
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-    /* On 32-bit ARM, we need to map not just the usable memory, but
-     * also the commpage.  Try to find a suitable place by allocating
-     * a big chunk for all of it.  If host_start, then the naive
-     * strategy probably does good enough.
-     */
-    if (!host_start) {
-        unsigned long guest_full_size, host_full_size, real_start;
-
-        guest_full_size =
-            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
-        host_full_size = guest_full_size - guest_start;
-        real_start = (unsigned long)
-            mmap(NULL, host_full_size, PROT_NONE, flags, -1, 0);
-        if (real_start == (unsigned long)-1) {
-            if (host_size < host_full_size - qemu_host_page_size) {
-                /* We failed to map a continous segment, but we're
-                 * allowed to have a gap between the usable memory and
-                 * the commpage where other things can be mapped.
-                 * This sparseness gives us more flexibility to find
-                 * an address range.
-                 */
-                goto naive;
-            }
-            return (unsigned long)-1;
+    if (!QEMU_IS_ALIGNED(guest_base, align)) {
+        fprintf(stderr, "Requested guest base 0x%lx does not satisfy "
+                "host minimum alignment (0x%lx)\n",
+                guest_base, align);
+        exit(EXIT_FAILURE);
+    }
+
+    /* Sanity check the guest binary. */
+    if (reserved_va) {
+        if (guest_hiaddr > reserved_va) {
+            error_report("%s: requires more than reserved virtual "
+                         "address space (0x%" PRIx64 " > 0x%lx)",
+                         image_name, (uint64_t)guest_hiaddr, reserved_va);
+            exit(EXIT_FAILURE);
         }
-        munmap((void *)real_start, host_full_size);
-        if (real_start & (align - 1)) {
-            /* The same thing again, but with extra
-             * so that we can shift around alignment.
-             */
-            unsigned long real_size = host_full_size + qemu_host_page_size;
-            real_start = (unsigned long)
-                mmap(NULL, real_size, PROT_NONE, flags, -1, 0);
-            if (real_start == (unsigned long)-1) {
-                if (host_size < host_full_size - qemu_host_page_size) {
-                    goto naive;
-                }
-                return (unsigned long)-1;
-            }
-            munmap((void *)real_start, real_size);
-            real_start = ROUND_UP(real_start, align);
+    } else {
+        if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) {
+            error_report("%s: requires more virtual address space "
+                         "than the host can provide (0x%" PRIx64 ")",
+                         image_name, (uint64_t)guest_hiaddr - guest_base);
+            exit(EXIT_FAILURE);
         }
-        current_start = real_start;
     }
- naive:
-#endif
 
-    while (1) {
-        unsigned long real_start, real_size, aligned_size;
-        aligned_size = real_size = host_size;
+    /*
+     * Expand the allocation to the entire reserved_va.
+     * Exclude the mmap_min_addr hole.
+     */
+    if (reserved_va) {
+        guest_loaddr = (guest_base >= mmap_min_addr ? 0
+                        : mmap_min_addr - guest_base);
+        guest_hiaddr = reserved_va;
+    }
 
-        /* Do not use mmap_find_vma here because that is limited to the
-         * guest address space.  We are going to make the
-         * guest address space fit whatever we're given.
-         */
-        real_start = (unsigned long)
-            mmap((void *)current_start, host_size, PROT_NONE, flags, -1, 0);
-        if (real_start == (unsigned long)-1) {
-            return (unsigned long)-1;
-        }
+    /* Reserve the address space for the binary, or reserved_va. */
+    test = g2h(guest_loaddr);
+    addr = mmap(test, guest_hiaddr - guest_loaddr, PROT_NONE, flags, -1, 0);
+    if (test != addr) {
+        pgb_fail_in_use(image_name);
+    }
+}
 
-        /* Check to see if the address is valid.  */
-        if (host_start && real_start != current_start) {
-            qemu_log_mask(CPU_LOG_PAGE, "invalid %lx && %lx != %lx\n",
-                          host_start, real_start, current_start);
-            goto try_again;
+/* Return value for guest_base, or -1 if no hole found. */
+static uintptr_t pgb_find_hole(uintptr_t guest_loaddr, uintptr_t guest_size,
+                               long align)
+{
+    GSList *maps, *iter;
+    uintptr_t this_start, this_end, next_start, brk;
+    intptr_t ret = -1;
+
+    assert(QEMU_IS_ALIGNED(guest_loaddr, align));
+
+    maps = read_self_maps();
+
+    /* Read brk after we've read the maps, which will malloc. */
+    brk = (uintptr_t)sbrk(0);
+
+    /* The first hole is before the first map entry. */
+    this_start = mmap_min_addr;
+
+    for (iter = maps; iter;
+         this_start = next_start, iter = g_slist_next(iter)) {
+        uintptr_t align_start, hole_size;
+
+        this_end = ((MapInfo *)iter->data)->start;
+        next_start = ((MapInfo *)iter->data)->end;
+        align_start = ROUND_UP(this_start, align);
+
+        /* Skip holes that are too small. */
+        if (align_start >= this_end) {
+            continue;
+        }
+        hole_size = this_end - align_start;
+        if (hole_size < guest_size) {
+            continue;
         }
 
-        /* Ensure the address is properly aligned.  */
-        if (real_start & (align - 1)) {
-            /* Ideally, we adjust like
-             *
-             *    pages: [  ][  ][  ][  ][  ]
-             *      old:   [   real   ]
-             *             [ aligned  ]
-             *      new:   [     real     ]
-             *               [ aligned  ]
-             *
-             * But if there is something else mapped right after it,
-             * then obviously it won't have room to grow, and the
-             * kernel will put the new larger real someplace else with
-             * unknown alignment (if we made it to here, then
-             * fixed=false).  Which is why we grow real by a full page
-             * size, instead of by part of one; so that even if we get
-             * moved, we can still guarantee alignment.  But this does
-             * mean that there is a padding of < 1 page both before
-             * and after the aligned range; the "after" could could
-             * cause problems for ARM emulation where it could butt in
-             * to where we need to put the commpage.
-             */
-            munmap((void *)real_start, host_size);
-            real_size = aligned_size + align;
-            real_start = (unsigned long)
-                mmap((void *)real_start, real_size, PROT_NONE, flags, -1, 0);
-            if (real_start == (unsigned long)-1) {
-                return (unsigned long)-1;
+        /* If this hole contains brk, give ourselves some room to grow. */
+        if (this_start <= brk && brk < this_end) {
+            hole_size -= guest_size;
+            if (sizeof(uintptr_t) == 8 && hole_size >= 1 * GiB) {
+                align_start += 1 * GiB;
+            } else if (hole_size >= 16 * MiB) {
+                align_start += 16 * MiB;
+            } else {
+                align_start = (this_end - guest_size) & -align;
+                if (align_start < this_start) {
+                    continue;
+                }
             }
-            aligned_start = ROUND_UP(real_start, align);
-        } else {
-            aligned_start = real_start;
         }
 
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-        /* On 32-bit ARM, we need to also be able to map the commpage.  */
-        int valid = init_guest_commpage(aligned_start - guest_start,
-                                        aligned_size + guest_start);
-        if (valid == -1) {
-            munmap((void *)real_start, real_size);
-            return (unsigned long)-1;
-        } else if (valid == 0) {
-            goto try_again;
+        /* Record the lowest successful match. */
+        if (ret < 0) {
+            ret = align_start - guest_loaddr;
         }
-#endif
-
-        /* If nothing has said `return -1` or `goto try_again` yet,
-         * then the address we have is good.
-         */
-        break;
-
-    try_again:
-        /* That address didn't work.  Unmap and try a different one.
-         * The address the host picked because is typically right at
-         * the top of the host address space and leaves the guest with
-         * no usable address space.  Resort to a linear search.  We
-         * already compensated for mmap_min_addr, so this should not
-         * happen often.  Probably means we got unlucky and host
-         * address space randomization put a shared library somewhere
-         * inconvenient.
-         *
-         * This is probably a good strategy if host_start, but is
-         * probably a bad strategy if not, which means we got here
-         * because of trouble with ARM commpage setup.
-         */
-        if (munmap((void *)real_start, real_size) != 0) {
-            error_report("%s: failed to unmap %lx:%lx (%s)", __func__,
-                         real_start, real_size, strerror(errno));
-            abort();
+        /* If this hole contains the identity map, select it. */
+        if (align_start <= guest_loaddr &&
+            guest_loaddr + guest_size <= this_end) {
+            ret = 0;
         }
-        current_start += align;
-        if (host_start == current_start) {
-            /* Theoretically possible if host doesn't have any suitably
-             * aligned areas.  Normally the first mmap will fail.
-             */
-            return (unsigned long)-1;
+        /* If this hole ends above the identity map, stop looking. */
+        if (this_end >= guest_loaddr) {
+            break;
         }
     }
+    free_self_maps(maps);
 
-    qemu_log_mask(CPU_LOG_PAGE, "Reserved 0x%lx bytes of guest address space\n", host_size);
-
-    return aligned_start;
+    return ret;
 }
 
-static void probe_guest_base(const char *image_name,
-                             abi_ulong loaddr, abi_ulong hiaddr)
+static void pgb_static(const char *image_name, abi_ulong orig_loaddr,
+                       abi_ulong orig_hiaddr, long align)
 {
-    /* Probe for a suitable guest base address, if the user has not set
-     * it explicitly, and set guest_base appropriately.
-     * In case of error we will print a suitable message and exit.
-     */
-    const char *errmsg;
-    if (!have_guest_base && !reserved_va) {
-        unsigned long host_start, real_start, host_size;
+    uintptr_t loaddr = orig_loaddr;
+    uintptr_t hiaddr = orig_hiaddr;
+    uintptr_t addr;
 
-        /* Round addresses to page boundaries.  */
-        loaddr &= qemu_host_page_mask;
-        hiaddr = HOST_PAGE_ALIGN(hiaddr);
+    if (hiaddr != orig_hiaddr) {
+        error_report("%s: requires virtual address space that the "
+                     "host cannot provide (0x%" PRIx64 ")",
+                     image_name, (uint64_t)orig_hiaddr);
+        exit(EXIT_FAILURE);
+    }
 
-        if (loaddr < mmap_min_addr) {
-            host_start = HOST_PAGE_ALIGN(mmap_min_addr);
+    loaddr &= -align;
+    if (ARM_COMMPAGE) {
+        /*
+         * Extend the allocation to include the commpage.
+         * For a 64-bit host, this is just 4GiB; for a 32-bit host,
+         * the address arithmetic will wrap around, but the difference
+         * will produce the correct allocation size.
+         */
+        if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) {
+            hiaddr = (uintptr_t)4 << 30;
         } else {
-            host_start = loaddr;
-            if (host_start != loaddr) {
-                errmsg = "Address overflow loading ELF binary";
-                goto exit_errmsg;
-            }
+            loaddr = ARM_COMMPAGE & -align;
         }
-        host_size = hiaddr - loaddr;
+    }
 
-        /* Setup the initial guest memory space with ranges gleaned from
-         * the ELF image that is being loaded.
+    addr = pgb_find_hole(loaddr, hiaddr - loaddr, align);
+    if (addr == -1) {
+        /*
+         * If ARM_COMMPAGE, there *might* be a non-consecutive allocation
+         * that can satisfy both.  But as the normal arm32 link base address
+         * is ~32k, and we extend down to include the commpage, making the
+         * overhead only ~96k, this is unlikely.
          */
-        real_start = init_guest_space(host_start, host_size, loaddr, false);
-        if (real_start == (unsigned long)-1) {
-            errmsg = "Unable to find space for application";
-            goto exit_errmsg;
-        }
-        guest_base = real_start - loaddr;
+        error_report("%s: Unable to allocate %#zx bytes of "
+                     "virtual address space", image_name,
+                     (size_t)(hiaddr - loaddr));
+        exit(EXIT_FAILURE);
+    }
+
+    guest_base = addr;
+}
+
+static void pgb_dynamic(const char *image_name, long align)
+{
+    /*
+     * The executable is dynamic and does not require a fixed address.
+     * All we need is a commpage that satisfies align.
+     * If we do not need a commpage, leave guest_base == 0.
+     */
+    if (ARM_COMMPAGE) {
+        uintptr_t addr, commpage;
 
-        qemu_log_mask(CPU_LOG_PAGE, "Relocating guest address space from 0x"
-                      TARGET_ABI_FMT_lx " to 0x%lx\n",
-                      loaddr, real_start);
+        /* 64-bit hosts should have used reserved_va. */
+        assert(sizeof(uintptr_t) == 4);
+
+        /*
+         * By putting the commpage at the first hole, that puts guest_base
+         * just above that, and maximises the positive guest addresses.
+         */
+        commpage = ARM_COMMPAGE & -align;
+        addr = pgb_find_hole(commpage, -commpage, align);
+        assert(addr != -1);
+        guest_base = addr;
     }
-    return;
+}
 
-exit_errmsg:
-    fprintf(stderr, "%s: %s\n", image_name, errmsg);
-    exit(-1);
+static void pgb_reserved_va(const char *image_name, abi_ulong guest_loaddr,
+                            abi_ulong guest_hiaddr, long align)
+{
+    const int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
+    void *addr, *test;
+
+    if (guest_hiaddr > reserved_va) {
+        error_report("%s: requires more than reserved virtual "
+                     "address space (0x%" PRIx64 " > 0x%lx)",
+                     image_name, (uint64_t)guest_hiaddr, reserved_va);
+        exit(EXIT_FAILURE);
+    }
+
+    /* Widen the "image" to the entire reserved address space. */
+    pgb_static(image_name, 0, reserved_va, align);
+
+    /* Reserve the memory on the host. */
+    assert(guest_base != 0);
+    test = g2h(0);
+    addr = mmap(test, reserved_va, PROT_NONE, flags, -1, 0);
+    if (addr == MAP_FAILED) {
+        error_report("Unable to reserve 0x%lx bytes of virtual address "
+                     "space for use as guest address space (check your "
+                     "virtual memory ulimit setting or reserve less "
+                     "using -R option)", reserved_va);
+        exit(EXIT_FAILURE);
+    }
+    assert(addr == test);
 }
 
+void probe_guest_base(const char *image_name, abi_ulong guest_loaddr,
+                      abi_ulong guest_hiaddr)
+{
+    /* In order to use host shmat, we must be able to honor SHMLBA.  */
+    uintptr_t align = MAX(SHMLBA, qemu_host_page_size);
+
+    if (have_guest_base) {
+        pgb_have_guest_base(image_name, guest_loaddr, guest_hiaddr, align);
+    } else if (reserved_va) {
+        pgb_reserved_va(image_name, guest_loaddr, guest_hiaddr, align);
+    } else if (guest_loaddr) {
+        pgb_static(image_name, guest_loaddr, guest_hiaddr, align);
+    } else {
+        pgb_dynamic(image_name, align);
+    }
+
+    /* Reserve and initialize the commpage. */
+    if (!init_guest_commpage()) {
+        /*
+         * With have_guest_base, the user has selected the address and
+         * we are trying to work with that.  Otherwise, we have selected
+         * free space and init_guest_commpage must succeeded.
+         */
+        assert(have_guest_base);
+        pgb_fail_in_use(image_name);
+    }
+
+    assert(QEMU_IS_ALIGNED(guest_base, align));
+    qemu_log_mask(CPU_LOG_PAGE, "Locating guest address space "
+                  "@ 0x%" PRIx64 "\n", (uint64_t)guest_base);
+}
 
 /* Load an ELF image into the address space.
 
@@ -2399,6 +2390,12 @@ static void load_elf_image(const char *image_name, int image_fd,
              * MMAP_MIN_ADDR or the QEMU application itself.
              */
             probe_guest_base(image_name, loaddr, hiaddr);
+        } else {
+            /*
+             * The binary is dynamic, but we still need to
+             * select guest_base.  In this case we pass a size.
+             */
+            probe_guest_base(image_name, 0, hiaddr - loaddr);
         }
     }
 
diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 66901f39cc5..8fb448f0bf0 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -441,6 +441,12 @@ static int load_flat_file(struct linux_binprm * bprm,
     indx_len = MAX_SHARED_LIBS * sizeof(abi_ulong);
     indx_len = (indx_len + 15) & ~(abi_ulong)15;
 
+    /*
+     * Alloate the address space.
+     */
+    probe_guest_base(bprm->filename, 0,
+                     text_len + data_len + extra + indx_len);
+
     /*
      * there are a couple of cases here,  the separate code/data
      * case,  and then the fully copied to RAM case which lumps
diff --git a/linux-user/main.c b/linux-user/main.c
index 22578b16336..1d20a83d4e8 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -24,6 +24,7 @@
 #include "qemu-version.h"
 #include <sys/syscall.h>
 #include <sys/resource.h>
+#include <sys/shm.h>
 
 #include "qapi/error.h"
 #include "qemu.h"
@@ -747,28 +748,6 @@ int main(int argc, char **argv, char **envp)
     target_environ = envlist_to_environ(envlist, NULL);
     envlist_free(envlist);
 
-    /*
-     * Now that page sizes are configured in tcg_exec_init() we can do
-     * proper page alignment for guest_base.
-     */
-    guest_base = HOST_PAGE_ALIGN(guest_base);
-
-    if (reserved_va || have_guest_base) {
-        guest_base = init_guest_space(guest_base, reserved_va, 0,
-                                      have_guest_base);
-        if (guest_base == (unsigned long)-1) {
-            fprintf(stderr, "Unable to reserve 0x%lx bytes of virtual address "
-                    "space for use as guest address space (check your virtual "
-                    "memory ulimit setting or reserve less using -R option)\n",
-                    reserved_va);
-            exit(EXIT_FAILURE);
-        }
-
-        if (reserved_va) {
-            mmap_next_start = reserved_va;
-        }
-    }
-
     /*
      * Read in mmap_min_addr kernel parameter.  This value is used
      * When loading the ELF image to determine whether guest_base
-- 
2.20.1

[PATCH v2 02/17] exec/cpu-all: Use bool for have_guest_base

[Alex Bennée]

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

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/cpu-all.h | 2 +-
 bsd-user/main.c        | 4 ++--
 linux-user/main.c      | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 49384bb66a5..b4fb5832c4a 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -159,7 +159,7 @@ static inline void tswap64s(uint64_t *s)
  * This allows the guest address space to be offset to a convenient location.
  */
 extern unsigned long guest_base;
-extern int have_guest_base;
+extern bool have_guest_base;
 extern unsigned long reserved_va;
 
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 770c2b267ad..aef5531628a 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -42,7 +42,7 @@
 int singlestep;
 unsigned long mmap_min_addr;
 unsigned long guest_base;
-int have_guest_base;
+bool have_guest_base;
 unsigned long reserved_va;
 
 static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
@@ -828,7 +828,7 @@ int main(int argc, char **argv)
             }
         } else if (!strcmp(r, "B")) {
            guest_base = strtol(argv[optind++], NULL, 0);
-           have_guest_base = 1;
+           have_guest_base = true;
         } else if (!strcmp(r, "drop-ld-preload")) {
             (void) envlist_unsetenv(envlist, "LD_PRELOAD");
         } else if (!strcmp(r, "bsd")) {
diff --git a/linux-user/main.c b/linux-user/main.c
index 1d20a83d4e8..90ad365b439 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -59,7 +59,7 @@ static const char *cpu_type;
 static const char *seed_optarg;
 unsigned long mmap_min_addr;
 unsigned long guest_base;
-int have_guest_base;
+bool have_guest_base;
 
 /*
  * Used to implement backwards-compatibility for the `-strace`, and
@@ -334,7 +334,7 @@ static void handle_arg_cpu(const char *arg)
 static void handle_arg_guest_base(const char *arg)
 {
     guest_base = strtol(arg, NULL, 0);
-    have_guest_base = 1;
+    have_guest_base = true;
 }
 
 static void handle_arg_reserved_va(const char *arg)
-- 
2.20.1

[PATCH v2 03/17] accel/tcg: Relax va restrictions on 64-bit guests

[Alex Bennée]

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

We cannot at present limit a 64-bit guest to a virtual address
space smaller than the host.  It will mostly work to ignore this
limitation, except if the guest uses high bits of the address
space for tags.  But it will certainly work better, as presently
we can wind up failing to allocate the guest stack.

Widen our user-only page tree to the host or abi pointer width.
Remove the workaround for this problem from target/alpha.
Always validate guest addresses vs reserved_va, as there we
control allocation ourselves.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/cpu-all.h    | 23 +++++++++++++++++++----
 target/alpha/cpu-param.h  | 15 ++-------------
 accel/tcg/translate-all.c | 15 +++++++++------
 3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index b4fb5832c4a..c0c2fa3cc56 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -162,12 +162,27 @@ extern unsigned long guest_base;
 extern bool have_guest_base;
 extern unsigned long reserved_va;
 
-#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
-#define GUEST_ADDR_MAX (~0ul)
+/*
+ * Limit the guest addresses as best we can.
+ *
+ * When not using -R reserved_va, we cannot really limit the guest
+ * to less address space than the host.  For 32-bit guests, this
+ * acts as a sanity check that we're not giving the guest an address
+ * that it cannot even represent.  For 64-bit guests... the address
+ * might not be what the real kernel would give, but it is at least
+ * representable in the guest.
+ *
+ * TODO: Improve address allocation to avoid this problem, and to
+ * avoid setting bits at the top of guest addresses that might need
+ * to be used for tags.
+ */
+#if MIN(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32
+# define GUEST_ADDR_MAX_  UINT32_MAX
 #else
-#define GUEST_ADDR_MAX (reserved_va ? reserved_va - 1 : \
-                                    (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+# define GUEST_ADDR_MAX_  ~0ul
 #endif
+#define GUEST_ADDR_MAX    (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_)
+
 #else
 
 #include "exec/hwaddr.h"
diff --git a/target/alpha/cpu-param.h b/target/alpha/cpu-param.h
index 692aee27ca9..1153992e42a 100644
--- a/target/alpha/cpu-param.h
+++ b/target/alpha/cpu-param.h
@@ -10,22 +10,11 @@
 
 #define TARGET_LONG_BITS 64
 #define TARGET_PAGE_BITS 13
-#ifdef CONFIG_USER_ONLY
-/*
- * ??? The kernel likes to give addresses in high memory.  If the host has
- * more virtual address space than the guest, this can lead to impossible
- * allocations.  Honor the long-standing assumption that only kernel addrs
- * are negative, but otherwise allow allocations anywhere.  This could lead
- * to tricky emulation problems for programs doing tagged addressing, but
- * that's far fewer than encounter the impossible allocation problem.
- */
-#define TARGET_PHYS_ADDR_SPACE_BITS  63
-#define TARGET_VIRT_ADDR_SPACE_BITS  63
-#else
+
 /* ??? EV4 has 34 phys addr bits, EV5 has 40, EV6 has 44.  */
 #define TARGET_PHYS_ADDR_SPACE_BITS  44
 #define TARGET_VIRT_ADDR_SPACE_BITS  (30 + TARGET_PAGE_BITS)
-#endif
+
 #define NB_MMU_MODES 3
 
 #endif
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 9924e66d1f7..e4f703a7e6d 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -173,8 +173,13 @@ struct page_collection {
 #define TB_FOR_EACH_JMP(head_tb, tb, n)                                 \
     TB_FOR_EACH_TAGGED((head_tb)->jmp_list_head, tb, n, jmp_list_next)
 
-/* In system mode we want L1_MAP to be based on ram offsets,
-   while in user mode we want it to be based on virtual addresses.  */
+/*
+ * In system mode we want L1_MAP to be based on ram offsets,
+ * while in user mode we want it to be based on virtual addresses.
+ *
+ * TODO: For user mode, see the caveat re host vs guest virtual
+ * address spaces near GUEST_ADDR_MAX.
+ */
 #if !defined(CONFIG_USER_ONLY)
 #if HOST_LONG_BITS < TARGET_PHYS_ADDR_SPACE_BITS
 # define L1_MAP_ADDR_SPACE_BITS  HOST_LONG_BITS
@@ -182,7 +187,7 @@ struct page_collection {
 # define L1_MAP_ADDR_SPACE_BITS  TARGET_PHYS_ADDR_SPACE_BITS
 #endif
 #else
-# define L1_MAP_ADDR_SPACE_BITS  TARGET_VIRT_ADDR_SPACE_BITS
+# define L1_MAP_ADDR_SPACE_BITS  MIN(HOST_LONG_BITS, TARGET_ABI_BITS)
 #endif
 
 /* Size of the L2 (and L3, etc) page tables.  */
@@ -2497,9 +2502,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
     /* This function should never be called with addresses outside the
        guest address space.  If this assert fires, it probably indicates
        a missing call to h2g_valid.  */
-#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
-    assert(end <= ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
-#endif
+    assert(end - 1 <= GUEST_ADDR_MAX);
     assert(start < end);
     assert_memory_lock();
 
-- 
2.20.1

[PATCH v2 04/17] .gitignore: include common build sub-directories

[Alex Bennée]

As out-of-tree builds become more common (or rather building in a
subdir) we can add a lot of load to "git ls-files" as it hunts down
sub-directories that are irrelevant to the source tree. This is
especially annoying if you have a prompt that attempts to summarise
the current git status on command completion.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---
v2
  - use build*/ to capture build and it's variants
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 0c5af83aa74..8dccb61a44e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -141,6 +141,7 @@ cscope.*
 tags
 TAGS
 docker-src.*
+build*/
 *~
 *.ast_raw
 *.depend_raw
-- 
2.20.1

[PATCH v2 05/17] linux-user/ppc: Fix padding in mcontext_t for ppc64

[Alex Bennée]

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

The padding that was added in 95cda4c44ee was added to a union,
and so it had no effect.  This fixes misalignment errors detected
by clang sanitizers for ppc64 and ppc64le.

In addition, only ppc64 allocates space for VSX registers, so do
not save them for ppc32.  The kernel only has references to
CONFIG_SPE in signal_32.c, so do not attempt to save them for ppc64.

Fixes: 95cda4c44ee
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/ppc/signal.c | 69 +++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index ecd99736b7e..20a02c197cb 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -35,12 +35,26 @@ struct target_mcontext {
     target_ulong mc_gregs[48];
     /* Includes fpscr.  */
     uint64_t mc_fregs[33];
+
 #if defined(TARGET_PPC64)
     /* Pointer to the vector regs */
     target_ulong v_regs;
+    /*
+     * On ppc64, this mcontext structure is naturally *unaligned*,
+     * or rather it is aligned on a 8 bytes boundary but not on
+     * a 16 byte boundary.  This pad fixes it up.  This is why we
+     * cannot use ppc_avr_t, which would force alignment.  This is
+     * also why the vector regs are referenced in the ABI by the
+     * v_regs pointer above so any amount of padding can be added here.
+     */
+    target_ulong pad;
+    /* VSCR and VRSAVE are saved separately.  Also reserve space for VSX. */
+    struct {
+        uint64_t altivec[34 + 16][2];
+    } mc_vregs;
 #else
     target_ulong mc_pad[2];
-#endif
+
     /* We need to handle Altivec and SPE at the same time, which no
        kernel needs to do.  Fortunately, the kernel defines this bit to
        be Altivec-register-large all the time, rather than trying to
@@ -48,32 +62,14 @@ struct target_mcontext {
     union {
         /* SPE vector registers.  One extra for SPEFSCR.  */
         uint32_t spe[33];
-        /* Altivec vector registers.  The packing of VSCR and VRSAVE
-           varies depending on whether we're PPC64 or not: PPC64 splits
-           them apart; PPC32 stuffs them together.
-           We also need to account for the VSX registers on PPC64
-        */
-#if defined(TARGET_PPC64)
-#define QEMU_NVRREG (34 + 16)
-        /* On ppc64, this mcontext structure is naturally *unaligned*,
-         * or rather it is aligned on a 8 bytes boundary but not on
-         * a 16 bytes one. This pad fixes it up. This is also why the
-         * vector regs are referenced by the v_regs pointer above so
-         * any amount of padding can be added here
-         */
-        target_ulong pad;
-#else
-        /* On ppc32, we are already aligned to 16 bytes */
-#define QEMU_NVRREG 33
-#endif
-        /* We cannot use ppc_avr_t here as we do *not* want the implied
-         * 16-bytes alignment that would result from it. This would have
-         * the effect of making the whole struct target_mcontext aligned
-         * which breaks the layout of struct target_ucontext on ppc64.
+        /*
+         * Altivec vector registers.  One extra for VRSAVE.
+         * On ppc32, we are already aligned to 16 bytes.  We could
+         * use ppc_avr_t, but choose to share the same type as ppc64.
          */
-        uint64_t altivec[QEMU_NVRREG][2];
-#undef QEMU_NVRREG
+        uint64_t altivec[33][2];
     } mc_vregs;
+#endif
 };
 
 /* See arch/powerpc/include/asm/sigcontext.h.  */
@@ -278,6 +274,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
         __put_user((uint32_t)env->spr[SPR_VRSAVE], vrsave);
     }
 
+#if defined(TARGET_PPC64)
     /* Save VSX second halves */
     if (env->insns_flags2 & PPC2_VSX) {
         uint64_t *vsregs = (uint64_t *)&frame->mc_vregs.altivec[34];
@@ -286,6 +283,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
             __put_user(*vsrl, &vsregs[i]);
         }
     }
+#endif
 
     /* Save floating point registers.  */
     if (env->insns_flags & PPC_FLOAT) {
@@ -296,22 +294,18 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
         __put_user((uint64_t) env->fpscr, &frame->mc_fregs[32]);
     }
 
+#if !defined(TARGET_PPC64)
     /* Save SPE registers.  The kernel only saves the high half.  */
     if (env->insns_flags & PPC_SPE) {
-#if defined(TARGET_PPC64)
-        for (i = 0; i < ARRAY_SIZE(env->gpr); i++) {
-            __put_user(env->gpr[i] >> 32, &frame->mc_vregs.spe[i]);
-        }
-#else
         for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
             __put_user(env->gprh[i], &frame->mc_vregs.spe[i]);
         }
-#endif
         /* Set MSR_SPE in the saved MSR value to indicate that
            frame->mc_vregs contains valid data.  */
         msr |= MSR_SPE;
         __put_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
     }
+#endif
 
     /* Store MSR.  */
     __put_user(msr, &frame->mc_gregs[TARGET_PT_MSR]);
@@ -392,6 +386,7 @@ static void restore_user_regs(CPUPPCState *env,
         __get_user(env->spr[SPR_VRSAVE], vrsave);
     }
 
+#if defined(TARGET_PPC64)
     /* Restore VSX second halves */
     if (env->insns_flags2 & PPC2_VSX) {
         uint64_t *vsregs = (uint64_t *)&frame->mc_vregs.altivec[34];
@@ -400,6 +395,7 @@ static void restore_user_regs(CPUPPCState *env,
             __get_user(*vsrl, &vsregs[i]);
         }
     }
+#endif
 
     /* Restore floating point registers.  */
     if (env->insns_flags & PPC_FLOAT) {
@@ -412,22 +408,15 @@ static void restore_user_regs(CPUPPCState *env,
         env->fpscr = (uint32_t) fpscr;
     }
 
+#if !defined(TARGET_PPC64)
     /* Save SPE registers.  The kernel only saves the high half.  */
     if (env->insns_flags & PPC_SPE) {
-#if defined(TARGET_PPC64)
-        for (i = 0; i < ARRAY_SIZE(env->gpr); i++) {
-            uint32_t hi;
-
-            __get_user(hi, &frame->mc_vregs.spe[i]);
-            env->gpr[i] = ((uint64_t)hi << 32) | ((uint32_t) env->gpr[i]);
-        }
-#else
         for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
             __get_user(env->gprh[i], &frame->mc_vregs.spe[i]);
         }
-#endif
         __get_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
     }
+#endif
 }
 
 #if !defined(TARGET_PPC64)
-- 
2.20.1

[PATCH v2 06/17] tests/docker: add docs FEATURE flag and use for test-misc

[Alex Bennée]

The test-misc docker test fails on a number of images which don't have
the prerequisites to build the docs. Use the FEATURES flag so we can
skip those tests.

As the sphinx test fails to detect whatever feature we need to get
hxtool to work we drop them from debian9 so the windows build doesn't
attempt to build the docs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/docker/dockerfiles/debian10.docker   | 2 ++
 tests/docker/dockerfiles/debian9.docker    | 2 --
 tests/docker/dockerfiles/fedora.docker     | 2 +-
 tests/docker/dockerfiles/travis.docker     | 2 +-
 tests/docker/dockerfiles/ubuntu.docker     | 2 +-
 tests/docker/dockerfiles/ubuntu1804.docker | 2 +-
 tests/docker/test-misc                     | 2 ++
 7 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
index 2fcdc406e83..0769700a416 100644
--- a/tests/docker/dockerfiles/debian10.docker
+++ b/tests/docker/dockerfiles/debian10.docker
@@ -34,3 +34,5 @@ RUN apt update && \
         python3-sphinx \
         texinfo \
         $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
+
+ENV FEATURES docs
diff --git a/tests/docker/dockerfiles/debian9.docker b/tests/docker/dockerfiles/debian9.docker
index 92edbbf0f48..08cc970feb1 100644
--- a/tests/docker/dockerfiles/debian9.docker
+++ b/tests/docker/dockerfiles/debian9.docker
@@ -30,6 +30,4 @@ RUN apt update && \
         pkg-config \
         psmisc \
         python3 \
-        python3-sphinx \
-        texinfo \
         $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
index 4bd2c953af8..179575ecaaa 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -103,4 +103,4 @@ ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3
 RUN dnf install -y $PACKAGES
 RUN rpm -q $PACKAGES | sort > /packages.txt
 ENV PATH $PATH:/usr/libexec/python3-sphinx/
-ENV FEATURES mingw clang pyyaml asan
+ENV FEATURES mingw clang pyyaml asan docs
diff --git a/tests/docker/dockerfiles/travis.docker b/tests/docker/dockerfiles/travis.docker
index e8eb48dccfd..591282561bc 100644
--- a/tests/docker/dockerfiles/travis.docker
+++ b/tests/docker/dockerfiles/travis.docker
@@ -13,5 +13,5 @@ RUN apt-get -y install device-tree-compiler python3 python3-yaml dh-autoreconf g
 # Travis tools require PhantomJS / Neo4j / Maven accessible
 # in their PATH (QEMU build won't access them).
 ENV PATH /usr/local/phantomjs/bin:/usr/local/phantomjs:/usr/local/neo4j-3.2.7/bin:/usr/local/maven-3.5.2/bin:/usr/local/cmake-3.9.2/bin:/usr/local/clang-5.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
-ENV FEATURES clang pyyaml
+ENV FEATURES clang pyyaml docs
 USER travis
diff --git a/tests/docker/dockerfiles/ubuntu.docker b/tests/docker/dockerfiles/ubuntu.docker
index b6c7b41dddd..eeb3b22bf20 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -68,4 +68,4 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
     DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
-ENV FEATURES clang pyyaml sdl2
+ENV FEATURES clang pyyaml sdl2 docs
diff --git a/tests/docker/dockerfiles/ubuntu1804.docker b/tests/docker/dockerfiles/ubuntu1804.docker
index 1efedeef995..f66b06f4cff 100644
--- a/tests/docker/dockerfiles/ubuntu1804.docker
+++ b/tests/docker/dockerfiles/ubuntu1804.docker
@@ -54,7 +54,7 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
     DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
-ENV FEATURES clang pyyaml sdl2
+ENV FEATURES clang pyyaml sdl2 docs
 
 # https://bugs.launchpad.net/qemu/+bug/1838763
 ENV QEMU_CONFIGURE_OPTS --disable-libssh
diff --git a/tests/docker/test-misc b/tests/docker/test-misc
index d480afedca7..cc94a738dd0 100755
--- a/tests/docker/test-misc
+++ b/tests/docker/test-misc
@@ -14,6 +14,8 @@
 
 . common.rc
 
+requires docs
+
 cd "$BUILD_DIR"
 
 # build everything else but QEMU
-- 
2.20.1

[PATCH v2 07/17] configure: redirect sphinx-build check to config.log

[Alex Bennée]

Otherwise it's hard to debug whats going on.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 configure | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 9b1f5b33e45..25f7d915720 100755
--- a/configure
+++ b/configure
@@ -4942,7 +4942,9 @@ has_sphinx_build() {
     # sphinx-build doesn't exist at all or if it is too old.
     mkdir -p "$TMPDIR1/sphinx"
     touch "$TMPDIR1/sphinx/index.rst"
-    "$sphinx_build" $sphinx_werror -c "$source_path/docs" -b html "$TMPDIR1/sphinx" "$TMPDIR1/sphinx/out" >/dev/null 2>&1
+    "$sphinx_build" $sphinx_werror -c "$source_path/docs" \
+                    -b html "$TMPDIR1/sphinx" \
+                    "$TMPDIR1/sphinx/out"  >> config.log 2>&1
 }
 
 # Check if tools are available to build documentation.
-- 
2.20.1

[PATCH v2 08/17] configure: disable PIE for Windows builds

[Alex Bennée]

It seems on some compilers the test can pass but still give you
broken binaries.

Fixes: d2cd29e30736
Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
Cc: Bug 1871798 <1871798@bugs.launchpad.net>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Howard Spoelstra <hsp.cat7@gmail.com>
Tested-by: James Le Cuirot <chewi@aura-online.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 25f7d915720..23b5e93752b 100755
--- a/configure
+++ b/configure
@@ -807,6 +807,7 @@ MINGW32*)
     audio_drv_list=""
   fi
   supported_os="yes"
+  pie="no"
 ;;
 GNU/kFreeBSD)
   bsd="yes"
-- 
2.20.1

[Bug 1871798] [PATCH v2 08/17] configure: disable PIE for Windows builds

[Alex Bennée]

It seems on some compilers the test can pass but still give you
broken binaries.

Fixes: d2cd29e30736
Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
Cc: Bug 1871798 <1871798@bugs.launchpad.net>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Howard Spoelstra <hsp.cat7@gmail.com>
Tested-by: James Le Cuirot <chewi@aura-online.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 25f7d915720..23b5e93752b 100755
--- a/configure
+++ b/configure
@@ -807,6 +807,7 @@ MINGW32*)
     audio_drv_list=""
   fi
   supported_os="yes"
+  pie="no"
 ;;
 GNU/kFreeBSD)
   bsd="yes"
-- 
2.20.1

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[PATCH v2 09/17] linux-user: fix /proc/self/stat handling

[Alex Bennée]

In the original bug report long files names in Guix caused
/proc/self/stat be truncated without the trailing ") " as specified in
proc manpage which says:
    (2) comm  %s
           The  filename of the executable, in parentheses.  This
           is visible whether or not the  executable  is  swapped
           out.

In the kernel this is currently done by do_task_stat calling
proc_task_name() which uses a structure limited by TASK_COMM_LEN (16).

Additionally it should only be reporting the executable name rather
than the full path. Fix both these failings while cleaning up the code
to use GString to build up the reported values. As the whole function
is cleaned up also adjust the white space to the current coding style.

Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>
Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6495ddc4cda..674f70e70a5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd)
 {
     CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
     TaskState *ts = cpu->opaque;
-    abi_ulong start_stack = ts->info->start_stack;
+    g_autoptr(GString) buf = g_string_new(NULL);
     int i;
 
     for (i = 0; i < 44; i++) {
-      char buf[128];
-      int len;
-      uint64_t val = 0;
-
-      if (i == 0) {
-        /* pid */
-        val = getpid();
-        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
-      } else if (i == 1) {
-        /* app name */
-        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
-      } else if (i == 27) {
-        /* stack bottom */
-        val = start_stack;
-        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
-      } else {
-        /* for the rest, there is MasterCard */
-        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
-      }
+        if (i == 0) {
+            /* pid */
+            g_string_printf(buf, FMT_pid " ", getpid());
+        } else if (i == 1) {
+            /* app name */
+            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
+            bin = bin ? bin + 1 : ts->bprm->argv[0];
+            g_string_printf(buf, "(%.15s) ", bin);
+        } else if (i == 27) {
+            /* stack bottom */
+            g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack);
+        } else {
+            /* for the rest, there is MasterCard */
+            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
+        }
 
-      len = strlen(buf);
-      if (write(fd, buf, len) != len) {
-          return -1;
-      }
+        if (write(fd, buf->str, buf->len) != buf->len) {
+            return -1;
+        }
     }
 
     return 0;
-- 
2.20.1

[PATCH v2 10/17] target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray

[Alex Bennée]

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

Since a010bdbe719 the gdbstub API takes a GByteArray*. Unfortunately
we forgot to update the gdb_get_reg*() calls. Do it now.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200409172509.4078-1-philmd@redhat.com>
---
 target/m68k/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 014657c6372..cad40838956 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -109,8 +109,8 @@ static int m68k_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
 {
     if (n < 8) {
         int len = gdb_get_reg16(mem_buf, env->fregs[n].l.upper);
-        len += gdb_get_reg16(mem_buf + len, 0);
-        len += gdb_get_reg64(mem_buf + len, env->fregs[n].l.lower);
+        len += gdb_get_reg16(mem_buf, 0);
+        len += gdb_get_reg64(mem_buf, env->fregs[n].l.lower);
         return len;
     }
     switch (n) {
-- 
2.20.1

[PATCH v2 11/17] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Alex Bennée]

From: Peter Xu <peterx@redhat.com>

We should only pass in gdb_get_reg16() with the GByteArray* object
itself, no need to shift.  Without this patch, gdb remote attach will
crash QEMU:

  (gdb) target remote :1234
  Remote debugging using :1234
  Remote communication error.  Target disconnected.: Connection reset by peer.
  $ qemu-system-x86_64 -m 1G -smp 4 ... -s
  ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)
  Bail out! ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200409164954.36902-3-peterx@redhat.com>

---
v2
  - tags and expand comment
---
 target/i386/gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index f3d23b614ee..b98a99500ae 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
         floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
         int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
-        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
+        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
         return len;
     } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
         n -= IDX_XMM_REGS;
-- 
2.20.1

[PATCH v2 12/17] gdbstub: Do not use memset() on GByteArray

[Alex Bennée]

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

Introduce gdb_get_zeroes() to fill a GByteArray with zeroes.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200414102427.7459-1-philmd@redhat.com>
[AJB: used slightly more gliby set_size approach]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/gdbstub.h  | 9 +++++++++
 target/arm/gdbstub.c    | 3 +--
 target/xtensa/gdbstub.c | 6 ++----
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 30b909ebd27..3c452fc50c0 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -125,6 +125,15 @@ static inline int gdb_get_reg128(GByteArray *buf, uint64_t val_hi,
     return 16;
 }
 
+static inline int gdb_get_zeroes(GByteArray *array, size_t len)
+{
+    guint oldlen = array->len;
+    g_byte_array_set_size(array, oldlen + len);
+    memset(array->data + oldlen, 0, len);
+
+    return len;
+}
+
 /**
  * gdb_get_reg_ptr: get pointer to start of last element
  * @len: length of element
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 8efc535f2a0..063551df234 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -47,8 +47,7 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         if (gdb_has_xml) {
             return 0;
         }
-        memset(mem_buf, 0, 12);
-        return 12;
+        return gdb_get_zeroes(mem_buf, 12);
     }
     switch (n) {
     case 24:
diff --git a/target/xtensa/gdbstub.c b/target/xtensa/gdbstub.c
index 0ee3feabe54..4d43f1340ae 100644
--- a/target/xtensa/gdbstub.c
+++ b/target/xtensa/gdbstub.c
@@ -105,8 +105,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         default:
             qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported size %d\n",
                           __func__, n, reg->size);
-            memset(mem_buf, 0, reg->size);
-            return reg->size;
+            return gdb_get_zeroes(mem_buf, reg->size);
         }
 
     case xtRegisterTypeWindow: /*a*/
@@ -115,8 +114,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     default:
         qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported type %d\n",
                       __func__, n, reg->type);
-        memset(mem_buf, 0, reg->size);
-        return reg->size;
+        return gdb_get_zeroes(mem_buf, reg->size);
     }
 }
 
-- 
2.20.1

[PATCH v2 13/17] gdbstub: Introduce gdb_get_float32() to get 32-bit float registers

[Alex Bennée]

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

Since we now use a GByteArray, we can not use stfl_p() directly.
Introduce the gdb_get_float32() helper to load a float32 register.

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-2-philmd@redhat.com>
---
 include/exec/gdbstub.h | 9 +++++++++
 target/sh4/gdbstub.c   | 6 ++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 3c452fc50c0..20e10726929 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -125,6 +125,15 @@ static inline int gdb_get_reg128(GByteArray *buf, uint64_t val_hi,
     return 16;
 }
 
+static inline int gdb_get_float32(GByteArray *array, float32 val)
+{
+    uint8_t buf[sizeof(CPU_FloatU)];
+
+    stfl_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/sh4/gdbstub.c b/target/sh4/gdbstub.c
index 49fc4a0cc69..34ad3ca0508 100644
--- a/target/sh4/gdbstub.c
+++ b/target/sh4/gdbstub.c
@@ -58,11 +58,9 @@ int superh_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         return gdb_get_regl(mem_buf, env->fpscr);
     case 25 ... 40:
         if (env->fpscr & FPSCR_FR) {
-            stfl_p(mem_buf, env->fregs[n - 9]);
-        } else {
-            stfl_p(mem_buf, env->fregs[n - 25]);
+            return gdb_get_float32(mem_buf, env->fregs[n - 9]);
         }
-        return 4;
+        return gdb_get_float32(mem_buf, env->fregs[n - 25]);
     case 41:
         return gdb_get_regl(mem_buf, env->ssr);
     case 42:
-- 
2.20.1

[PATCH v2 14/17] 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 20e10726929..4a2b8e30893 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 cad40838956..79b0b10ea9b 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 eb362dd9aec..5c11c88b2a8 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 e853164a865..d825cb59758 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 v2 15/17] target/m68k: hack around the FPU register support (HACK!)

[Alex Bennée]

Attempting to attach to the gdbstub causes GDB to complain:

  warning: Register "fp0" has an unsupported size (80 bits)
  warning: Register "fp1" has an unsupported size (80 bits)
  warning: Register "fp2" has an unsupported size (80 bits)
  warning: Register "fp3" has an unsupported size (80 bits)
  warning: Register "fp4" has an unsupported size (80 bits)
  warning: Register "fp5" has an unsupported size (80 bits)
  warning: Register "fp6" has an unsupported size (80 bits)
  warning: Register "fp7" has an unsupported size (80 bits)
  Remote 'g' packet reply is too long (expected 148 bytes, got 164 bytes): 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000$
  00000000000000000000408009f000000000800003407fffffffffffffffffff7fffffffffffffffffff7fffffffffffffffffff7fffffffffffffffffff7fffffffffffffffffff7fffffffffffffffffff7fffffff$
  fffffffffff7fffffffffffffffffff000000000000000000000000

and then subsequently fail. The root problem seems to be this is an
undefined size register for the target description. There does exist a
floatformats_m68881_ext in GDB but setting "m68881_ext" also fails as
the only "weird" tdesc types gdb seems to understand are:

  { "arm_fpa_ext", TDESC_TYPE_ARM_FPA_EXT },
  { "i387_ext", TDESC_TYPE_I387_EXT }

So present the register as a i386_ext as some sort of hack. The values
are garbage but at least we can continue to connect. Perhaps we should
just delete the code because I don't think this ever worked with
upstream tools.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Laurent Vivier <laurent@vivier.eu>
---
 target/m68k/helper.c | 11 +++++------
 gdb-xml/m68k-fp.xml  | 16 ++++++++--------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 79b0b10ea9b..80069adb8cc 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -109,9 +109,8 @@ static int cf_fpu_gdb_set_reg(CPUM68KState *env, uint8_t *mem_buf, int n)
 static int m68k_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
 {
     if (n < 8) {
-        int len = gdb_get_reg16(mem_buf, env->fregs[n].l.upper);
-        len += gdb_get_reg16(mem_buf, 0);
-        len += gdb_get_reg64(mem_buf, env->fregs[n].l.lower);
+        int len = gdb_get_reg64(mem_buf, cpu_to_le64(env->fregs[n].l.lower));
+        len += gdb_get_reg16(mem_buf, cpu_to_le16(env->fregs[n].l.upper));
         return len;
     }
     switch (n) {
@@ -128,9 +127,9 @@ static int m68k_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
 static int m68k_fpu_gdb_set_reg(CPUM68KState *env, uint8_t *mem_buf, int n)
 {
     if (n < 8) {
-        env->fregs[n].l.upper = lduw_be_p(mem_buf);
-        env->fregs[n].l.lower = ldq_be_p(mem_buf + 4);
-        return 12;
+        env->fregs[n].l.lower = le64_to_cpu(* (uint64_t *) mem_buf);
+        env->fregs[n].l.upper = le16_to_cpu(* (uint16_t *) (mem_buf + 8));
+        return 10;
     }
     switch (n) {
     case 8: /* fpcontrol */
diff --git a/gdb-xml/m68k-fp.xml b/gdb-xml/m68k-fp.xml
index 64290d16306..8eb55af2860 100644
--- a/gdb-xml/m68k-fp.xml
+++ b/gdb-xml/m68k-fp.xml
@@ -6,14 +6,14 @@
      notice and this notice are preserved.  -->
 <!DOCTYPE feature SYSTEM "gdb-target.dtd">
 <feature name="org.gnu.gdb.coldfire.fp">
-  <reg name="fp0" bitsize="96" type="float" group="float"/>
-  <reg name="fp1" bitsize="96" type="float" group="float"/>
-  <reg name="fp2" bitsize="96" type="float" group="float"/>
-  <reg name="fp3" bitsize="96" type="float" group="float"/>
-  <reg name="fp4" bitsize="96" type="float" group="float"/>
-  <reg name="fp5" bitsize="96" type="float" group="float"/>
-  <reg name="fp6" bitsize="96" type="float" group="float"/>
-  <reg name="fp7" bitsize="96" type="float" group="float"/>
+  <reg name="fp0" bitsize="80" type="i387_ext" group="float"/>
+  <reg name="fp1" bitsize="80" type="i387_ext" group="float"/>
+  <reg name="fp2" bitsize="80" type="i387_ext" group="float"/>
+  <reg name="fp3" bitsize="80" type="i387_ext" group="float"/>
+  <reg name="fp4" bitsize="80" type="i387_ext" group="float"/>
+  <reg name="fp5" bitsize="80" type="i387_ext" group="float"/>
+  <reg name="fp6" bitsize="80" type="i387_ext" group="float"/>
+  <reg name="fp7" bitsize="80" type="i387_ext" group="float"/>
 
   <reg name="fpcontrol" bitsize="32" group="float"/>
   <reg name="fpstatus" bitsize="32" group="float"/>,
-- 
2.20.1

[PATCH v2 16/17] 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 984fbeb277e..5824abe09ac 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 dbe7f2aa932..90201d80094 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 v2 17/17] 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>
---
 tests/tcg/multiarch/Makefile.target | 15 ++++++
 tests/tcg/multiarch/gdbstub/sha1.py | 81 +++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 tests/tcg/multiarch/gdbstub/sha1.py

diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 035b09c8533..47fd675aba5 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -42,5 +42,20 @@ 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
+
+MULTIARCH_TESTS += gdbstub-sha1
+
+.PHONY: gdbstub-sha1
+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")
+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 00000000000..734553b98bb
--- /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

Re: [PATCH v2 13/17] gdbstub: Introduce gdb_get_float32() to get 32-bit float registers

[Richard Henderson]

On 4/14/20 1:06 PM, Alex Bennée wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Since we now use a GByteArray, we can not use stfl_p() directly.
> Introduce the gdb_get_float32() helper to load a float32 register.
> 
> 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-2-philmd@redhat.com>
> ---
>  include/exec/gdbstub.h | 9 +++++++++
>  target/sh4/gdbstub.c   | 6 ++----
>  2 files changed, 11 insertions(+), 4 deletions(-)

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


r~

Re: [PATCH v2 14/17] gdbstub: Introduce gdb_get_float64() to get 64-bit float registers

[Richard Henderson]

On 4/14/20 1:06 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(-)

Not for 5.0, please.

As I mentioned in reply to Phil, this is not a bug fix.  Unlike the float32 sh4
usage, the places changed properly use gdb_get_reg64().


r~

Re: [PATCH v2 for 5.0-rc3 00/17] more randome fixes (user, pie, docker and gdbstub)

[no-reply]

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



Hi,

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

Subject: [PATCH  v2 for 5.0-rc3 00/17] more randome fixes (user, pie, docker and gdbstub)
Message-id: 20200414200631.12799-1-alex.bennee@linaro.org
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 ===

Switched to a new branch 'test'
1d9b560 tests/tcg: add a multiarch linux-user gdb test
0421111 tests/tcg: drop inferior.was_attached() test
d252cf5 target/m68k: hack around the FPU register support (HACK!)
fe2d373 gdbstub: Introduce gdb_get_float64() to get 64-bit float registers
a067a36 gdbstub: Introduce gdb_get_float32() to get 32-bit float registers
22159df gdbstub: Do not use memset() on GByteArray
0440574 gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb
661c288 target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray
6d7f04a linux-user: fix /proc/self/stat handling
8077d16 configure: disable PIE for Windows builds
3525a95 configure: redirect sphinx-build check to config.log
d7cc6ea tests/docker: add docs FEATURE flag and use for test-misc
2faca0a linux-user/ppc: Fix padding in mcontext_t for ppc64
bd002dd .gitignore: include common build sub-directories
2cb4915 accel/tcg: Relax va restrictions on 64-bit guests
b81214a exec/cpu-all: Use bool for have_guest_base
56dda0e linux-user: completely re-write init_guest_space

=== OUTPUT BEGIN ===
1/17 Checking commit 56dda0ee6d96 (linux-user: completely re-write init_guest_space)
2/17 Checking commit b81214a4a7b1 (exec/cpu-all: Use bool for have_guest_base)
3/17 Checking commit 2cb4915f8641 (accel/tcg: Relax va restrictions on 64-bit guests)
ERROR: Macros with complex values should be enclosed in parenthesis
#91: FILE: include/exec/cpu-all.h:182:
+# define GUEST_ADDR_MAX_  ~0ul

total: 1 errors, 0 warnings, 88 lines checked

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

4/17 Checking commit bd002dddde97 (.gitignore: include common build sub-directories)
5/17 Checking commit 2faca0a20813 (linux-user/ppc: Fix padding in mcontext_t for ppc64)
6/17 Checking commit d7cc6eab6083 (tests/docker: add docs FEATURE flag and use for test-misc)
7/17 Checking commit 3525a9514500 (configure: redirect sphinx-build check to config.log)
8/17 Checking commit 8077d168511c (configure: disable PIE for Windows builds)
9/17 Checking commit 6d7f04aaf64a (linux-user: fix /proc/self/stat handling)
10/17 Checking commit 661c288d6e96 (target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray)
11/17 Checking commit 04405743253c (gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb)
12/17 Checking commit 22159df16cb9 (gdbstub: Do not use memset() on GByteArray)
13/17 Checking commit a067a369add1 (gdbstub: Introduce gdb_get_float32() to get 32-bit float registers)
14/17 Checking commit fe2d3733f79b (gdbstub: Introduce gdb_get_float64() to get 64-bit float registers)
15/17 Checking commit d252cf53d601 (target/m68k: hack around the FPU register support (HACK!))
ERROR: space prohibited after that '*' (ctx:BxW)
#88: FILE: target/m68k/helper.c:130:
+        env->fregs[n].l.lower = le64_to_cpu(* (uint64_t *) mem_buf);
                                             ^

ERROR: space prohibited after that '*' (ctx:BxW)
#89: FILE: target/m68k/helper.c:131:
+        env->fregs[n].l.upper = le16_to_cpu(* (uint16_t *) (mem_buf + 8));
                                             ^

total: 2 errors, 0 warnings, 45 lines checked

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

16/17 Checking commit 04211112b3bd (tests/tcg: drop inferior.was_attached() test)
17/17 Checking commit 1d9b560da197 (tests/tcg: add a multiarch linux-user gdb test)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#40: 
new file mode 100644

total: 0 errors, 1 warnings, 101 lines checked

Patch 17/17 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/20200414200631.12799-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

[PULL for 5.0-rc3 0/8] a few small fixes (docker, user, pie and gdbstub)

[Alex Bennée]

The following changes since commit 2f7cc1fbd6f6655d900ca7f45973b9bd5330c6dd:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2020-04-14 20:09:52 +0100)

are available in the Git repository at:

  https://github.com/stsquad/qemu.git tags/pull-more-fixes-150420-1

for you to fetch changes up to 377f8f08bebea7cd44617b0ac0a2baf307f5f055:

  gdbstub: Introduce gdb_get_float32() to get 32-bit float registers (2020-04-15 11:38:23 +0100)

----------------------------------------------------------------
More small fixes for rc3

  - tweak docker FEATURE flags for document building
  - include sphinx configure check in config.log
  - disable PIE for Windows builds
  - fix /proc/self/stat handling
  - a number of gdbstub fixups following GByteArray conversion

----------------------------------------------------------------
Alex Bennée (4):
      tests/docker: add docs FEATURE flag and use for test-misc
      configure: redirect sphinx-build check to config.log
      configure: disable PIE for Windows builds
      linux-user: fix /proc/self/stat handling

Peter Xu (1):
      gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

Philippe Mathieu-Daudé (3):
      target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray
      gdbstub: Do not use memset() on GByteArray
      gdbstub: Introduce gdb_get_float32() to get 32-bit float registers

 configure                                  |  5 +++-
 include/exec/gdbstub.h                     | 18 +++++++++++++
 linux-user/syscall.c                       | 43 +++++++++++++-----------------
 target/arm/gdbstub.c                       |  3 +--
 target/i386/gdbstub.c                      |  2 +-
 target/m68k/helper.c                       |  4 +--
 target/sh4/gdbstub.c                       |  6 ++---
 target/xtensa/gdbstub.c                    |  6 ++---
 tests/docker/dockerfiles/debian10.docker   |  2 ++
 tests/docker/dockerfiles/debian9.docker    |  2 --
 tests/docker/dockerfiles/fedora.docker     |  2 +-
 tests/docker/dockerfiles/travis.docker     |  2 +-
 tests/docker/dockerfiles/ubuntu.docker     |  2 +-
 tests/docker/dockerfiles/ubuntu1804.docker |  2 +-
 tests/docker/test-misc                     |  2 ++
 15 files changed, 57 insertions(+), 44 deletions(-)

-- 
2.20.1

[PULL 1/8] tests/docker: add docs FEATURE flag and use for test-misc

[Alex Bennée]

The test-misc docker test fails on a number of images which don't have
the prerequisites to build the docs. Use the FEATURES flag so we can
skip those tests.

As the sphinx test fails to detect whatever feature we need to get
hxtool to work we drop them from debian9 so the windows build doesn't
attempt to build the docs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200414200631.12799-7-alex.bennee@linaro.org>

diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
index 2fcdc406e83..0769700a416 100644
--- a/tests/docker/dockerfiles/debian10.docker
+++ b/tests/docker/dockerfiles/debian10.docker
@@ -34,3 +34,5 @@ RUN apt update && \
         python3-sphinx \
         texinfo \
         $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
+
+ENV FEATURES docs
diff --git a/tests/docker/dockerfiles/debian9.docker b/tests/docker/dockerfiles/debian9.docker
index 92edbbf0f48..08cc970feb1 100644
--- a/tests/docker/dockerfiles/debian9.docker
+++ b/tests/docker/dockerfiles/debian9.docker
@@ -30,6 +30,4 @@ RUN apt update && \
         pkg-config \
         psmisc \
         python3 \
-        python3-sphinx \
-        texinfo \
         $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
index 4bd2c953af8..179575ecaaa 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -103,4 +103,4 @@ ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3
 RUN dnf install -y $PACKAGES
 RUN rpm -q $PACKAGES | sort > /packages.txt
 ENV PATH $PATH:/usr/libexec/python3-sphinx/
-ENV FEATURES mingw clang pyyaml asan
+ENV FEATURES mingw clang pyyaml asan docs
diff --git a/tests/docker/dockerfiles/travis.docker b/tests/docker/dockerfiles/travis.docker
index e8eb48dccfd..591282561bc 100644
--- a/tests/docker/dockerfiles/travis.docker
+++ b/tests/docker/dockerfiles/travis.docker
@@ -13,5 +13,5 @@ RUN apt-get -y install device-tree-compiler python3 python3-yaml dh-autoreconf g
 # Travis tools require PhantomJS / Neo4j / Maven accessible
 # in their PATH (QEMU build won't access them).
 ENV PATH /usr/local/phantomjs/bin:/usr/local/phantomjs:/usr/local/neo4j-3.2.7/bin:/usr/local/maven-3.5.2/bin:/usr/local/cmake-3.9.2/bin:/usr/local/clang-5.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
-ENV FEATURES clang pyyaml
+ENV FEATURES clang pyyaml docs
 USER travis
diff --git a/tests/docker/dockerfiles/ubuntu.docker b/tests/docker/dockerfiles/ubuntu.docker
index b6c7b41dddd..eeb3b22bf20 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -68,4 +68,4 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
     DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
-ENV FEATURES clang pyyaml sdl2
+ENV FEATURES clang pyyaml sdl2 docs
diff --git a/tests/docker/dockerfiles/ubuntu1804.docker b/tests/docker/dockerfiles/ubuntu1804.docker
index 1efedeef995..f66b06f4cff 100644
--- a/tests/docker/dockerfiles/ubuntu1804.docker
+++ b/tests/docker/dockerfiles/ubuntu1804.docker
@@ -54,7 +54,7 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
     DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
-ENV FEATURES clang pyyaml sdl2
+ENV FEATURES clang pyyaml sdl2 docs
 
 # https://bugs.launchpad.net/qemu/+bug/1838763
 ENV QEMU_CONFIGURE_OPTS --disable-libssh
diff --git a/tests/docker/test-misc b/tests/docker/test-misc
index d480afedca7..cc94a738dd0 100755
--- a/tests/docker/test-misc
+++ b/tests/docker/test-misc
@@ -14,6 +14,8 @@
 
 . common.rc
 
+requires docs
+
 cd "$BUILD_DIR"
 
 # build everything else but QEMU
-- 
2.20.1

[PULL 2/8] configure: redirect sphinx-build check to config.log

[Alex Bennée]

Otherwise it's hard to debug whats going on.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200414200631.12799-8-alex.bennee@linaro.org>

diff --git a/configure b/configure
index 9b1f5b33e45..25f7d915720 100755
--- a/configure
+++ b/configure
@@ -4942,7 +4942,9 @@ has_sphinx_build() {
     # sphinx-build doesn't exist at all or if it is too old.
     mkdir -p "$TMPDIR1/sphinx"
     touch "$TMPDIR1/sphinx/index.rst"
-    "$sphinx_build" $sphinx_werror -c "$source_path/docs" -b html "$TMPDIR1/sphinx" "$TMPDIR1/sphinx/out" >/dev/null 2>&1
+    "$sphinx_build" $sphinx_werror -c "$source_path/docs" \
+                    -b html "$TMPDIR1/sphinx" \
+                    "$TMPDIR1/sphinx/out"  >> config.log 2>&1
 }
 
 # Check if tools are available to build documentation.
-- 
2.20.1

[PULL 3/8] configure: disable PIE for Windows builds

[Alex Bennée]

It seems on some compilers the test can pass but still give you
broken binaries.

Fixes: d2cd29e30736
Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
Cc: Bug 1871798 <1871798@bugs.launchpad.net>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Howard Spoelstra <hsp.cat7@gmail.com>
Tested-by: James Le Cuirot <chewi@aura-online.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200414200631.12799-9-alex.bennee@linaro.org>

diff --git a/configure b/configure
index 25f7d915720..23b5e93752b 100755
--- a/configure
+++ b/configure
@@ -807,6 +807,7 @@ MINGW32*)
     audio_drv_list=""
   fi
   supported_os="yes"
+  pie="no"
 ;;
 GNU/kFreeBSD)
   bsd="yes"
-- 
2.20.1

[Bug 1871798] [PULL 3/8] configure: disable PIE for Windows builds

[Alex Bennée]

It seems on some compilers the test can pass but still give you
broken binaries.

Fixes: d2cd29e30736
Fixes: https://bugs.launchpad.net/qemu/+bug/1871798
Cc: Bug 1871798 <1871798@bugs.launchpad.net>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Howard Spoelstra <hsp.cat7@gmail.com>
Tested-by: James Le Cuirot <chewi@aura-online.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200414200631.12799-9-alex.bennee@linaro.org>

diff --git a/configure b/configure
index 25f7d915720..23b5e93752b 100755
--- a/configure
+++ b/configure
@@ -807,6 +807,7 @@ MINGW32*)
     audio_drv_list=""
   fi
   supported_os="yes"
+  pie="no"
 ;;
 GNU/kFreeBSD)
   bsd="yes"
-- 
2.20.1

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Incomplete

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[PULL 4/8] linux-user: fix /proc/self/stat handling

[Alex Bennée]

In the original bug report long files names in Guix caused
/proc/self/stat be truncated without the trailing ") " as specified in
proc manpage which says:
    (2) comm  %s
           The  filename of the executable, in parentheses.  This
           is visible whether or not the  executable  is  swapped
           out.

In the kernel this is currently done by do_task_stat calling
proc_task_name() which uses a structure limited by TASK_COMM_LEN (16).

Additionally it should only be reporting the executable name rather
than the full path. Fix both these failings while cleaning up the code
to use GString to build up the reported values. As the whole function
is cleaned up also adjust the white space to the current coding style.

Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>
Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200414200631.12799-10-alex.bennee@linaro.org>

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6495ddc4cda..674f70e70a5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd)
 {
     CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
     TaskState *ts = cpu->opaque;
-    abi_ulong start_stack = ts->info->start_stack;
+    g_autoptr(GString) buf = g_string_new(NULL);
     int i;
 
     for (i = 0; i < 44; i++) {
-      char buf[128];
-      int len;
-      uint64_t val = 0;
-
-      if (i == 0) {
-        /* pid */
-        val = getpid();
-        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
-      } else if (i == 1) {
-        /* app name */
-        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
-      } else if (i == 27) {
-        /* stack bottom */
-        val = start_stack;
-        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
-      } else {
-        /* for the rest, there is MasterCard */
-        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
-      }
+        if (i == 0) {
+            /* pid */
+            g_string_printf(buf, FMT_pid " ", getpid());
+        } else if (i == 1) {
+            /* app name */
+            gchar *bin = g_strrstr(ts->bprm->argv[0], "/");
+            bin = bin ? bin + 1 : ts->bprm->argv[0];
+            g_string_printf(buf, "(%.15s) ", bin);
+        } else if (i == 27) {
+            /* stack bottom */
+            g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack);
+        } else {
+            /* for the rest, there is MasterCard */
+            g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');
+        }
 
-      len = strlen(buf);
-      if (write(fd, buf, len) != len) {
-          return -1;
-      }
+        if (write(fd, buf->str, buf->len) != buf->len) {
+            return -1;
+        }
     }
 
     return 0;
-- 
2.20.1

[PULL 5/8] target/m68k/helper: Fix m68k_fpu_gdb_get_reg() use of GByteArray

[Alex Bennée]

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

Since a010bdbe719 the gdbstub API takes a GByteArray*. Unfortunately
we forgot to update the gdb_get_reg*() calls. Do it now.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200409172509.4078-1-philmd@redhat.com>
Message-Id: <20200414200631.12799-11-alex.bennee@linaro.org>

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 014657c6372..cad40838956 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -109,8 +109,8 @@ static int m68k_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
 {
     if (n < 8) {
         int len = gdb_get_reg16(mem_buf, env->fregs[n].l.upper);
-        len += gdb_get_reg16(mem_buf + len, 0);
-        len += gdb_get_reg64(mem_buf + len, env->fregs[n].l.lower);
+        len += gdb_get_reg16(mem_buf, 0);
+        len += gdb_get_reg64(mem_buf, env->fregs[n].l.lower);
         return len;
     }
     switch (n) {
-- 
2.20.1

[PULL 6/8] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Alex Bennée]

From: Peter Xu <peterx@redhat.com>

We should only pass in gdb_get_reg16() with the GByteArray* object
itself, no need to shift.  Without this patch, gdb remote attach will
crash QEMU:

  (gdb) target remote :1234
  Remote debugging using :1234
  Remote communication error.  Target disconnected.: Connection reset by peer.
  $ qemu-system-x86_64 -m 1G -smp 4 ... -s
  ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)
  Bail out! ERROR:qemu/gdbstub.c:1843:handle_read_all_regs: assertion failed: (len == gdbserver_state.mem_buf->len)

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200409164954.36902-3-peterx@redhat.com>
Message-Id: <20200414200631.12799-12-alex.bennee@linaro.org>

diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index f3d23b614ee..b98a99500ae 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
         floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
         int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
-        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
+        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
         return len;
     } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
         n -= IDX_XMM_REGS;
-- 
2.20.1

[PULL 7/8] gdbstub: Do not use memset() on GByteArray

[Alex Bennée]

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

Introduce gdb_get_zeroes() to fill a GByteArray with zeroes.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200414102427.7459-1-philmd@redhat.com>
[AJB: used slightly more gliby set_size approach]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200414200631.12799-13-alex.bennee@linaro.org>

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 30b909ebd27..3c452fc50c0 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -125,6 +125,15 @@ static inline int gdb_get_reg128(GByteArray *buf, uint64_t val_hi,
     return 16;
 }
 
+static inline int gdb_get_zeroes(GByteArray *array, size_t len)
+{
+    guint oldlen = array->len;
+    g_byte_array_set_size(array, oldlen + len);
+    memset(array->data + oldlen, 0, len);
+
+    return len;
+}
+
 /**
  * gdb_get_reg_ptr: get pointer to start of last element
  * @len: length of element
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 8efc535f2a0..063551df234 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -47,8 +47,7 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         if (gdb_has_xml) {
             return 0;
         }
-        memset(mem_buf, 0, 12);
-        return 12;
+        return gdb_get_zeroes(mem_buf, 12);
     }
     switch (n) {
     case 24:
diff --git a/target/xtensa/gdbstub.c b/target/xtensa/gdbstub.c
index 0ee3feabe54..4d43f1340ae 100644
--- a/target/xtensa/gdbstub.c
+++ b/target/xtensa/gdbstub.c
@@ -105,8 +105,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         default:
             qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported size %d\n",
                           __func__, n, reg->size);
-            memset(mem_buf, 0, reg->size);
-            return reg->size;
+            return gdb_get_zeroes(mem_buf, reg->size);
         }
 
     case xtRegisterTypeWindow: /*a*/
@@ -115,8 +114,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     default:
         qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported type %d\n",
                       __func__, n, reg->type);
-        memset(mem_buf, 0, reg->size);
-        return reg->size;
+        return gdb_get_zeroes(mem_buf, reg->size);
     }
 }
 
-- 
2.20.1

[PULL 8/8] gdbstub: Introduce gdb_get_float32() to get 32-bit float registers

[Alex Bennée]

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

Since we now use a GByteArray, we can not use stfl_p() directly.
Introduce the gdb_get_float32() helper to load a float32 register.

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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200414163853.12164-2-philmd@redhat.com>
Message-Id: <20200414200631.12799-14-alex.bennee@linaro.org>

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 3c452fc50c0..20e10726929 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -125,6 +125,15 @@ static inline int gdb_get_reg128(GByteArray *buf, uint64_t val_hi,
     return 16;
 }
 
+static inline int gdb_get_float32(GByteArray *array, float32 val)
+{
+    uint8_t buf[sizeof(CPU_FloatU)];
+
+    stfl_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/sh4/gdbstub.c b/target/sh4/gdbstub.c
index 49fc4a0cc69..34ad3ca0508 100644
--- a/target/sh4/gdbstub.c
+++ b/target/sh4/gdbstub.c
@@ -58,11 +58,9 @@ int superh_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         return gdb_get_regl(mem_buf, env->fpscr);
     case 25 ... 40:
         if (env->fpscr & FPSCR_FR) {
-            stfl_p(mem_buf, env->fregs[n - 9]);
-        } else {
-            stfl_p(mem_buf, env->fregs[n - 25]);
+            return gdb_get_float32(mem_buf, env->fregs[n - 9]);
         }
-        return 4;
+        return gdb_get_float32(mem_buf, env->fregs[n - 25]);
     case 41:
         return gdb_get_regl(mem_buf, env->ssr);
     case 42:
-- 
2.20.1

Re: [PULL for 5.0-rc3 0/8] a few small fixes (docker, user, pie and gdbstub)

[Peter Maydell]

On Wed, 15 Apr 2020 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The following changes since commit 2f7cc1fbd6f6655d900ca7f45973b9bd5330c6dd:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2020-04-14 20:09:52 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/stsquad/qemu.git tags/pull-more-fixes-150420-1
>
> for you to fetch changes up to 377f8f08bebea7cd44617b0ac0a2baf307f5f055:
>
>   gdbstub: Introduce gdb_get_float32() to get 32-bit float registers (2020-04-15 11:38:23 +0100)
>
> ----------------------------------------------------------------
> More small fixes for rc3
>
>   - tweak docker FEATURE flags for document building
>   - include sphinx configure check in config.log
>   - disable PIE for Windows builds
>   - fix /proc/self/stat handling
>   - a number of gdbstub fixups following GByteArray conversion
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

[Bug 1871798] Re: Fails to start on Windows host without explicit --disable-pie

[Philippe Mathieu-Daudé]

Fixed in commit 469a788cdd3c618ef1b8a23a339510082b3eeea7.

** Changed in: qemu
       Status: Incomplete => Fix Committed

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Fix Committed

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions

[Bug 1871798] Re: Fails to start on Windows host without explicit --disable-pie

[Laurent Vivier]

** Changed in: qemu
       Status: Fix Committed => Fix Released

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871798

Title:
  Fails to start on Windows host without explicit --disable-pie

Status in QEMU:
  Fix Released

Bug description:
  Since commit d2cd29e30736afd4a1e8cac3cf4da360bbc65978, which removed
  the x86 conditional around PIE, QEMU completely fails to start on a
  Windows host unless --disable-pie is explicitly given at build time.
  Even just requesting the help text doesn't work. To make testing
  easier, this can be replicated with Wine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871798/+subscriptions