[PULL for 5.0-rc2 00/13] various fixes

[PULL for 5.0-rc2 00/13] various fixes

[Alex Bennée]

The following changes since commit 53ef8a92eb04ee19640f5aad3bff36cd4a36c250:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200406' into staging (2020-04-06 12:36:45 +0100)

are available in the Git repository at:

  https://github.com/stsquad/qemu.git tags/pull-misc-fixes-070420-1

for you to fetch changes up to cce743abbf398a324879039cd582349b36da0ea6:

  tcg/i386: Fix %r12 guest_base initialization (2020-04-07 16:19:49 +0100)

----------------------------------------------------------------
Various fixes:

  - add .github repo lockdown config
  - better handle missing symbols in elf-ops
  - protect fcntl64 with #ifdef
  - remove unused macros from test
  - fix handling of /proc/self/maps
  - avoid BAD_SHIFT in x80 softfloat
  - properly terminate on .hex EOF
  - fix configure probe on windows cross build
  - fix %r12 guest_base initialization

----------------------------------------------------------------
Alex Bennée (8):
      elf-ops: bail out if we have no function symbols
      linux-user: protect fcntl64 with an #ifdef
      tests/tcg: remove extraneous pasting macros
      linux-user: more debug for init_guest_space
      target/xtensa: add FIXME for translation memory leak
      linux-user: factor out reading of /proc/self/maps
      linux-user: clean-up padding on /proc/self/maps
      hw/core: properly terminate loading .hex on EOF record

Denis Plotnikov (1):
      gdbstub: fix compiler complaining

Philippe Mathieu-Daudé (1):
      .github: Enable repo-lockdown bot to refuse GitHub pull requests

Richard Henderson (3):
      softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal
      configure: Add -Werror to PIE probe
      tcg/i386: Fix %r12 guest_base initialization

 configure                      |  4 +--
 include/hw/elf_ops.h           | 48 +++++++++++++------------
 include/qemu/selfmap.h         | 44 +++++++++++++++++++++++
 fpu/softfloat.c                |  3 ++
 gdbstub.c                      |  4 +--
 hw/core/loader.c               |  5 ++-
 linux-user/elfload.c           |  8 ++++-
 linux-user/syscall.c           | 80 ++++++++++++++++++++++--------------------
 target/xtensa/translate.c      |  5 +++
 tcg/i386/tcg-target.inc.c      |  2 +-
 util/selfmap.c                 | 78 ++++++++++++++++++++++++++++++++++++++++
 .github/lockdown.yml           | 34 ++++++++++++++++++
 MAINTAINERS                    |  1 +
 tests/tcg/x86_64/system/boot.S |  5 +--
 util/Makefile.objs             |  1 +
 15 files changed, 250 insertions(+), 72 deletions(-)
 create mode 100644 include/qemu/selfmap.h
 create mode 100644 util/selfmap.c
 create mode 100644 .github/lockdown.yml

-- 
2.20.1

[PULL 01/13] .github: Enable repo-lockdown bot to refuse GitHub pull requests

[Alex Bennée]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Some GitHub users try to open pull requests against the GitHub
mirror. Unfortunate these get ignored until eventually someone
notices and closes the request.

Enable the 'Repo Lockdown' [*] 3rd party bot which can autorespond
to pull requests with a friendly comment, close the request, and
then lock it to prevent further comments.

[*] https://github.com/dessant/repo-lockdown

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-Id: <20200406214125.18538-1-f4bug@amsat.org>
[AJB: s/fill/file/ and point at canonical qemu.org/contribute]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/.github/lockdown.yml b/.github/lockdown.yml
new file mode 100644
index 00000000000..9acc393f1cf
--- /dev/null
+++ b/.github/lockdown.yml
@@ -0,0 +1,34 @@
+# Configuration for Repo Lockdown - https://github.com/dessant/repo-lockdown
+
+# Close issues and pull requests
+close: true
+
+# Lock issues and pull requests
+lock: true
+
+issues:
+  comment: |
+    Thank you for your interest in the QEMU project.
+
+    This repository is a read-only mirror of the project's master
+    repostories hosted on https://git.qemu.org/git/qemu.git.
+    The project does not process issues filed on GitHub.
+
+    The project issues are tracked on Launchpad:
+    https://bugs.launchpad.net/qemu
+
+    QEMU welcomes bug report contributions. You can file new ones on:
+    https://bugs.launchpad.net/qemu/+filebug
+
+pulls:
+  comment: |
+    Thank you for your interest in the QEMU project.
+
+    This repository is a read-only mirror of the project's master
+    repostories hosted on https://git.qemu.org/git/qemu.git.
+    The project does not process merge requests filed on GitHub.
+
+    QEMU welcomes contributions of code (either fixing bugs or adding new
+    functionality). However, we get a lot of patches, and so we have some
+    guidelines about contributing on the project website:
+    https://www.qemu.org/contribute/
diff --git a/MAINTAINERS b/MAINTAINERS
index 9d156d73b31..0559e847901 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2833,6 +2833,7 @@ M: Alex Bennée <alex.bennee@linaro.org>
 M: Fam Zheng <fam@euphon.net>
 R: Philippe Mathieu-Daudé <philmd@redhat.com>
 S: Maintained
+F: .github/lockdown.yml
 F: .travis.yml
 F: scripts/travis/
 F: .shippable.yml
-- 
2.20.1

[PULL 02/13] elf-ops: bail out if we have no function symbols

[Alex Bennée]

It's perfectly possible to have no function symbols in your elf file
and if we do the undefined behaviour sanitizer rightly complains about
us passing NULL to qsort. Check nsyms before we go ahead.

While we are at it lets drop the unchecked return value and cleanup
the fail leg by use of g_autoptr.

Another fix was proposed 101 weeks ago in:
Message-Id: 20180421232120.22208-1-f4bug@amsat.org

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: <20200403191150.863-2-alex.bennee@linaro.org>

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index a1411bfcab6..e0bb47bb678 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -104,19 +104,21 @@ static int glue(symcmp, SZ)(const void *s0, const void *s1)
         : ((sym0->st_value > sym1->st_value) ? 1 : 0);
 }
 
-static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
-                                  int clear_lsb, symbol_fn_t sym_cb)
+static void glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
+                                   int clear_lsb, symbol_fn_t sym_cb)
 {
-    struct elf_shdr *symtab, *strtab, *shdr_table = NULL;
-    struct elf_sym *syms = NULL;
+    struct elf_shdr *symtab, *strtab;
+    g_autofree struct elf_shdr *shdr_table = NULL;
+    g_autofree struct elf_sym *syms = NULL;
+    g_autofree char *str = NULL;
     struct syminfo *s;
     int nsyms, i;
-    char *str = NULL;
 
     shdr_table = load_at(fd, ehdr->e_shoff,
                          sizeof(struct elf_shdr) * ehdr->e_shnum);
-    if (!shdr_table)
-        return -1;
+    if (!shdr_table) {
+        return ;
+    }
 
     if (must_swab) {
         for (i = 0; i < ehdr->e_shnum; i++) {
@@ -125,23 +127,25 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
     }
 
     symtab = glue(find_section, SZ)(shdr_table, ehdr->e_shnum, SHT_SYMTAB);
-    if (!symtab)
-        goto fail;
+    if (!symtab) {
+        return;
+    }
     syms = load_at(fd, symtab->sh_offset, symtab->sh_size);
-    if (!syms)
-        goto fail;
+    if (!syms) {
+        return;
+    }
 
     nsyms = symtab->sh_size / sizeof(struct elf_sym);
 
     /* String table */
     if (symtab->sh_link >= ehdr->e_shnum) {
-        goto fail;
+        return;
     }
     strtab = &shdr_table[symtab->sh_link];
 
     str = load_at(fd, strtab->sh_offset, strtab->sh_size);
     if (!str) {
-        goto fail;
+        return;
     }
 
     i = 0;
@@ -170,8 +174,13 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
         }
         i++;
     }
-    syms = g_realloc(syms, nsyms * sizeof(*syms));
 
+    /* check we have symbols left */
+    if (nsyms == 0) {
+        return;
+    }
+
+    syms = g_realloc(syms, nsyms * sizeof(*syms));
     qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ));
     for (i = 0; i < nsyms - 1; i++) {
         if (syms[i].st_size == 0) {
@@ -182,18 +191,11 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
     /* Commit */
     s = g_malloc0(sizeof(*s));
     s->lookup_symbol = glue(lookup_symbol, SZ);
-    glue(s->disas_symtab.elf, SZ) = syms;
+    glue(s->disas_symtab.elf, SZ) = g_steal_pointer(&syms);
     s->disas_num_syms = nsyms;
-    s->disas_strtab = str;
+    s->disas_strtab = g_steal_pointer(&str);
     s->next = syminfos;
     syminfos = s;
-    g_free(shdr_table);
-    return 0;
- fail:
-    g_free(syms);
-    g_free(str);
-    g_free(shdr_table);
-    return -1;
 }
 
 static int glue(elf_reloc, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
-- 
2.20.1

[PULL 03/13] linux-user: protect fcntl64 with an #ifdef

[Alex Bennée]

Checking TARGET_ABI_BITS is sketchy - we should check for the presence
of the define to be sure. Also clean up the white space while we are
there.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200403191150.863-3-alex.bennee@linaro.org>

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5af55fca781..b679bc6b136 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11331,11 +11331,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
            This is a hint, so ignoring and returning success is ok.  */
         return 0;
 #endif
-#if TARGET_ABI_BITS == 32
+#ifdef TARGET_NR_fcntl64
     case TARGET_NR_fcntl64:
     {
-	int cmd;
-	struct flock64 fl;
+        int cmd;
+        struct flock64 fl;
         from_flock64_fn *copyfrom = copy_from_user_flock64;
         to_flock64_fn *copyto = copy_to_user_flock64;
 
@@ -11346,7 +11346,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
         }
 #endif
 
-	cmd = target_to_host_fcntl_cmd(arg2);
+        cmd = target_to_host_fcntl_cmd(arg2);
         if (cmd == -TARGET_EINVAL) {
             return cmd;
         }
-- 
2.20.1

[PULL 04/13] tests/tcg: remove extraneous pasting macros

[Alex Bennée]

We are not using them and they just get in the way.

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: <20200403191150.863-4-alex.bennee@linaro.org>

diff --git a/tests/tcg/x86_64/system/boot.S b/tests/tcg/x86_64/system/boot.S
index 205cfbd3982..73b19a2bda6 100644
--- a/tests/tcg/x86_64/system/boot.S
+++ b/tests/tcg/x86_64/system/boot.S
@@ -41,10 +41,7 @@
 #define XEN_ELFNOTE_PHYS32_ENTRY  18
 
 #define __ASM_FORM(x)	x
-#define __ASM_FORM_RAW(x)     x
-#define __ASM_FORM_COMMA(x) x,
-#define __ASM_SEL(a,b)           __ASM_FORM(b)
-#define __ASM_SEL_RAW(a,b)      __ASM_FORM_RAW(b)
+#define __ASM_SEL(a,b)  __ASM_FORM(b)
 #define _ASM_PTR	__ASM_SEL(.long, .quad)
 
 	ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR 0x100000)
-- 
2.20.1

[PULL 05/13] linux-user: more debug for init_guest_space

[Alex Bennée]

Searching for memory space can cause problems so lets extend the
CPU_LOG_PAGE output so you can watch init_guest_space fail to
allocate memory. A more involved fix is actually required to make this
function play nicely with the large guard pages the sanitiser likes to
use.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Message-Id: <20200403191150.863-5-alex.bennee@linaro.org>

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 8198be04460..619c054cc48 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2172,6 +2172,8 @@ unsigned long init_guest_space(unsigned long host_start,
 
         /* 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;
         }
 
@@ -2240,7 +2242,11 @@ unsigned long init_guest_space(unsigned long host_start,
          * probably a bad strategy if not, which means we got here
          * because of trouble with ARM commpage setup.
          */
-        munmap((void *)real_start, real_size);
+        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();
+        }
         current_start += align;
         if (host_start == current_start) {
             /* Theoretically possible if host doesn't have any suitably
-- 
2.20.1

[PULL 06/13] target/xtensa: add FIXME for translation memory leak

[Alex Bennée]

Dynamically allocating a new structure within the DisasContext can
potentially leak as we can longjmp out of the translation loop (see
test_phys_mem). The proper fix would be to use static allocation
within the DisasContext but as the Xtensa translator imports it's code
from elsewhere I leave that as an exercise for the maintainer.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Max Filippov <jcmvbkbc@gmail.com>
Message-Id: <20200403191150.863-6-alex.bennee@linaro.org>

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 8aa972cafdf..37f65b1f030 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -1174,6 +1174,11 @@ static void xtensa_tr_init_disas_context(DisasContextBase *dcbase,
     dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >>
                    XTENSA_TBFLAG_CALLINC_SHIFT);
 
+    /*
+     * FIXME: This will leak when a failed instruction load or similar
+     * event causes us to longjump out of the translation loop and
+     * hence not clean-up in xtensa_tr_tb_stop
+     */
     if (dc->config->isa) {
         dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa);
         dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa);
-- 
2.20.1

[PULL 07/13] gdbstub: fix compiler complaining

[Alex Bennée]

From: Denis Plotnikov <dplotnikov@virtuozzo.com>

    ./gdbstub.c: In function ‘handle_query_thread_extra’:
        /usr/include/glib-2.0/glib/glib-autocleanups.h:28:10:
    error: ‘cpu_name’ may be used uninitialized in this function
    [-Werror=maybe-uninitialized]
        g_free (*pp);
               ^
    ./gdbstub.c:2063:26: note: ‘cpu_name’ was declared here
        g_autofree char *cpu_name;
                         ^
    cc1: all warnings being treated as errors

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Message-Id: <20200326151407.25046-1-dplotnikov@virtuozzo.com>
Reported-by: Euler Robot <euler.robot@huawei.com>
Reported-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>
Message-Id: <20200325092137.24020-1-kuhn.chenqun@huawei.com>
Reviewed-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>
Message-Id: <20200403191150.863-7-alex.bennee@linaro.org>

diff --git a/gdbstub.c b/gdbstub.c
index 013fb1ac0f1..171e1509509 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2060,8 +2060,8 @@ static void handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
         /* Print the CPU model and name in multiprocess mode */
         ObjectClass *oc = object_get_class(OBJECT(cpu));
         const char *cpu_model = object_class_get_name(oc);
-        g_autofree char *cpu_name;
-        cpu_name  = object_get_canonical_path_component(OBJECT(cpu));
+        g_autofree char *cpu_name =
+            object_get_canonical_path_component(OBJECT(cpu));
         g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
                         cpu->halted ? "halted " : "running");
     } else {
-- 
2.20.1

[PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Alex Bennée]

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

All other calls to normalize*Subnormal detect zero input before
the call -- this is the only outlier.  This case can happen with
+0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of
the correct sign.

Reported-by: Coverity (CID 1421991)
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org>
Message-Id: <20200403191150.863-8-alex.bennee@linaro.org>

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 301ce3b537b..ae6ba718540 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, flag zSign,
         zSig1 = 0;
         zSig0 = aSig + bSig;
         if ( aExp == 0 ) {
+            if (zSig0 == 0) {
+                return packFloatx80(zSign, 0, 0);
+            }
             normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 );
             goto roundAndPack;
         }
-- 
2.20.1

[PULL 09/13] linux-user: factor out reading of /proc/self/maps

[Alex Bennée]

Unfortunately reading /proc/self/maps is still considered the gold
standard for a process finding out about it's own memory layout. As we
will want this data in other contexts soon factor out the code to read
and parse the data. Rather than just blindly copying the existing
sscanf based code we use a more modern glib version of the parsing
code to make a more general purpose map structure.

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

diff --git a/include/qemu/selfmap.h b/include/qemu/selfmap.h
new file mode 100644
index 00000000000..8382c4c779d
--- /dev/null
+++ b/include/qemu/selfmap.h
@@ -0,0 +1,44 @@
+/*
+ * Utility functions to read our own memory map
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef _SELFMAP_H_
+#define _SELFMAP_H_
+
+typedef struct {
+    unsigned long start;
+    unsigned long end;
+
+    /* flags */
+    bool is_read;
+    bool is_write;
+    bool is_exec;
+    bool is_priv;
+
+    unsigned long offset;
+    gchar *dev;
+    uint64_t inode;
+    gchar *path;
+} MapInfo;
+
+
+/**
+ * read_self_maps:
+ *
+ * Read /proc/self/maps and return a list of MapInfo structures.
+ */
+GSList *read_self_maps(void);
+
+/**
+ * free_self_maps:
+ * @info: a GSlist
+ *
+ * Free a list of MapInfo structures.
+ */
+void free_self_maps(GSList *info);
+
+#endif /* _SELFMAP_H_ */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b679bc6b136..5f117872947 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -117,6 +117,7 @@
 
 #include "qemu.h"
 #include "qemu/guest-random.h"
+#include "qemu/selfmap.h"
 #include "user/syscall-trace.h"
 #include "qapi/error.h"
 #include "fd-trans.h"
@@ -7232,45 +7233,45 @@ static int open_self_maps(void *cpu_env, int fd)
 {
     CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
     TaskState *ts = cpu->opaque;
-    FILE *fp;
-    char *line = NULL;
-    size_t len = 0;
-    ssize_t read;
+    GSList *map_info = read_self_maps();
+    GSList *s;
 
-    fp = fopen("/proc/self/maps", "r");
-    if (fp == NULL) {
-        return -1;
-    }
+    for (s = map_info; s; s = g_slist_next(s)) {
+        MapInfo *e = (MapInfo *) s->data;
 
-    while ((read = getline(&line, &len, fp)) != -1) {
-        int fields, dev_maj, dev_min, inode;
-        uint64_t min, max, offset;
-        char flag_r, flag_w, flag_x, flag_p;
-        char path[512] = "";
-        fields = sscanf(line, "%"PRIx64"-%"PRIx64" %c%c%c%c %"PRIx64" %x:%x %d"
-                        " %512s", &min, &max, &flag_r, &flag_w, &flag_x,
-                        &flag_p, &offset, &dev_maj, &dev_min, &inode, path);
-
-        if ((fields < 10) || (fields > 11)) {
-            continue;
-        }
-        if (h2g_valid(min)) {
+        if (h2g_valid(e->start)) {
+            unsigned long min = e->start;
+            unsigned long max = e->end;
             int flags = page_get_flags(h2g(min));
-            max = h2g_valid(max - 1) ? max : (uintptr_t)g2h(GUEST_ADDR_MAX) + 1;
+            const char *path;
+
+            max = h2g_valid(max - 1) ?
+                max : (uintptr_t) g2h(GUEST_ADDR_MAX) + 1;
+
             if (page_check_range(h2g(min), max - min, flags) == -1) {
                 continue;
             }
+
             if (h2g(min) == ts->info->stack_limit) {
-                pstrcpy(path, sizeof(path), "      [stack]");
+                path = "      [stack]";
+            } else {
+                path = e->path;
             }
+
             dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
-                    " %c%c%c%c %08" PRIx64 " %02x:%02x %d %s%s\n",
-                    h2g(min), h2g(max - 1) + 1, flag_r, flag_w,
-                    flag_x, flag_p, offset, dev_maj, dev_min, inode,
-                    path[0] ? "         " : "", path);
+                    " %c%c%c%c %08" PRIx64 " %s %"PRId64" %s%s\n",
+                    h2g(min), h2g(max - 1) + 1,
+                    e->is_read ? 'r' : '-',
+                    e->is_write ? 'w' : '-',
+                    e->is_exec ? 'x' : '-',
+                    e->is_priv ? 'p' : '-',
+                    (uint64_t) e->offset, e->dev, e->inode,
+                    path ? "         " : "", path ? path : "");
         }
     }
 
+    free_self_maps(map_info);
+
 #ifdef TARGET_VSYSCALL_PAGE
     /*
      * We only support execution from the vsyscall page.
@@ -7281,9 +7282,6 @@ static int open_self_maps(void *cpu_env, int fd)
             TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
 #endif
 
-    free(line);
-    fclose(fp);
-
     return 0;
 }
 
diff --git a/util/selfmap.c b/util/selfmap.c
new file mode 100644
index 00000000000..2ec99dfdda1
--- /dev/null
+++ b/util/selfmap.c
@@ -0,0 +1,78 @@
+/*
+ * Utility function to get QEMU's own process map
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qemu/selfmap.h"
+
+GSList *read_self_maps(void)
+{
+    gchar *maps;
+    GSList *map_info = NULL;
+
+    if (g_file_get_contents("/proc/self/maps", &maps, NULL, NULL)) {
+        gchar **lines = g_strsplit(maps, "\n", 0);
+        int i, entries = g_strv_length(lines);
+
+        for (i = 0; i < entries; i++) {
+            gchar **fields = g_strsplit(lines[i], " ", 6);
+            if (g_strv_length(fields) > 4) {
+                MapInfo *e = g_new0(MapInfo, 1);
+                int errors;
+                const char *end;
+
+                errors  = qemu_strtoul(fields[0], &end, 16, &e->start);
+                errors += qemu_strtoul(end + 1, NULL, 16, &e->end);
+
+                e->is_read  = fields[1][0] == 'r';
+                e->is_write = fields[1][1] == 'w';
+                e->is_exec  = fields[1][2] == 'x';
+                e->is_priv  = fields[1][3] == 'p';
+
+                errors += qemu_strtoul(fields[2], NULL, 16, &e->offset);
+                e->dev = g_strdup(fields[3]);
+                errors += qemu_strtou64(fields[4], NULL, 10, &e->inode);
+
+                /*
+                 * The last field may have leading spaces which we
+                 * need to strip.
+                 */
+                if (g_strv_length(fields) == 6) {
+                    e->path = g_strdup(g_strchug(fields[5]));
+                }
+                map_info = g_slist_prepend(map_info, e);
+            }
+
+            g_strfreev(fields);
+        }
+        g_strfreev(lines);
+        g_free(maps);
+    }
+
+    /* ensure the map data is in the same order we collected it */
+    return g_slist_reverse(map_info);
+}
+
+/**
+ * free_self_maps:
+ * @info: a GSlist
+ *
+ * Free a list of MapInfo structures.
+ */
+static void free_info(gpointer data)
+{
+    MapInfo *e = (MapInfo *) data;
+    g_free(e->dev);
+    g_free(e->path);
+    g_free(e);
+}
+
+void free_self_maps(GSList *info)
+{
+    g_slist_free_full(info, &free_info);
+}
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 6718a38b616..fe339c2636b 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -63,3 +63,4 @@ util-obj-y += guest-random.o
 util-obj-$(CONFIG_GIO) += dbus.o
 dbus.o-cflags = $(GIO_CFLAGS)
 dbus.o-libs = $(GIO_LIBS)
+util-obj-$(CONFIG_USER_ONLY) += selfmap.o
-- 
2.20.1

[PULL 10/13] linux-user: clean-up padding on /proc/self/maps

[Alex Bennée]

Don't use magic spaces, calculate the justification for the file
field like the kernel does with seq_pad.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200403191150.863-10-alex.bennee@linaro.org>

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5f117872947..6495ddc4cda 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7235,6 +7235,7 @@ static int open_self_maps(void *cpu_env, int fd)
     TaskState *ts = cpu->opaque;
     GSList *map_info = read_self_maps();
     GSList *s;
+    int count;
 
     for (s = map_info; s; s = g_slist_next(s)) {
         MapInfo *e = (MapInfo *) s->data;
@@ -7253,20 +7254,24 @@ static int open_self_maps(void *cpu_env, int fd)
             }
 
             if (h2g(min) == ts->info->stack_limit) {
-                path = "      [stack]";
+                path = "[stack]";
             } else {
                 path = e->path;
             }
 
-            dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
-                    " %c%c%c%c %08" PRIx64 " %s %"PRId64" %s%s\n",
-                    h2g(min), h2g(max - 1) + 1,
-                    e->is_read ? 'r' : '-',
-                    e->is_write ? 'w' : '-',
-                    e->is_exec ? 'x' : '-',
-                    e->is_priv ? 'p' : '-',
-                    (uint64_t) e->offset, e->dev, e->inode,
-                    path ? "         " : "", path ? path : "");
+            count = dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
+                            " %c%c%c%c %08" PRIx64 " %s %"PRId64,
+                            h2g(min), h2g(max - 1) + 1,
+                            e->is_read ? 'r' : '-',
+                            e->is_write ? 'w' : '-',
+                            e->is_exec ? 'x' : '-',
+                            e->is_priv ? 'p' : '-',
+                            (uint64_t) e->offset, e->dev, e->inode);
+            if (path) {
+                dprintf(fd, "%*s%s\n", 73 - count, "", path);
+            } else {
+                dprintf(fd, "\n");
+            }
         }
     }
 
@@ -7277,9 +7282,10 @@ static int open_self_maps(void *cpu_env, int fd)
      * We only support execution from the vsyscall page.
      * This is as if CONFIG_LEGACY_VSYSCALL_XONLY=y from v5.3.
      */
-    dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx
-            " --xp 00000000 00:00 0 [vsyscall]\n",
-            TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
+    count = dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx
+                    " --xp 00000000 00:00 0",
+                    TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
+    dprintf(fd, "%*s%s\n", 73 - count, "",  "[vsyscall]");
 #endif
 
     return 0;
-- 
2.20.1

[PULL 11/13] hw/core: properly terminate loading .hex on EOF record

[Alex Bennée]

The https://makecode.microbit.org/#editor generates slightly weird
.hex files which work fine on a real microbit but causes QEMU to
choke. The reason is extraneous data after the EOF record which causes
the loader to attempt to write a bigger file than it should to the
"rom". According to the HEX file spec an EOF really should be the last
thing we process so lets do that.

Reported-by: Ursula Bennée <alex.bennee@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200403191150.863-12-alex.bennee@linaro.org>

diff --git a/hw/core/loader.c b/hw/core/loader.c
index eeef6da9a1b..8bbb1797a4c 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1447,6 +1447,7 @@ typedef struct {
     uint32_t current_rom_index;
     uint32_t rom_start_address;
     AddressSpace *as;
+    bool complete;
 } HexParser;
 
 /* return size or -1 if error */
@@ -1484,6 +1485,7 @@ static int handle_record_type(HexParser *parser)
                                   parser->current_rom_index,
                                   parser->rom_start_address, parser->as);
         }
+        parser->complete = true;
         return parser->total_size;
     case EXT_SEG_ADDR_RECORD:
     case EXT_LINEAR_ADDR_RECORD:
@@ -1548,11 +1550,12 @@ static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob,
         .bin_buf = g_malloc(hex_blob_size),
         .start_addr = addr,
         .as = as,
+        .complete = false
     };
 
     rom_transaction_begin();
 
-    for (; hex_blob < end; ++hex_blob) {
+    for (; hex_blob < end && !parser.complete; ++hex_blob) {
         switch (*hex_blob) {
         case '\r':
         case '\n':
-- 
2.20.1

[PULL 12/13] configure: Add -Werror to PIE probe

[Alex Bennée]

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

Without -Werror, the probe may succeed, but then compilation fails
later when -Werror is added for other reasons.  Shows up on windows,
where the compiler complains about -fPIC.

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>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200401214756.6559-1-richard.henderson@linaro.org>
Message-Id: <20200403191150.863-13-alex.bennee@linaro.org>

diff --git a/configure b/configure
index 22870f38672..233c671aaa9 100755
--- a/configure
+++ b/configure
@@ -2119,7 +2119,7 @@ if compile_prog "-Werror -fno-pie" "-no-pie"; then
 fi
 
 if test "$static" = "yes"; then
-  if test "$pie" != "no" && compile_prog "-fPIE -DPIE" "-static-pie"; then
+  if test "$pie" != "no" && compile_prog "-Werror -fPIE -DPIE" "-static-pie"; then
     QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
     QEMU_LDFLAGS="-static-pie $QEMU_LDFLAGS"
     pie="yes"
@@ -2132,7 +2132,7 @@ if test "$static" = "yes"; then
 elif test "$pie" = "no"; then
   QEMU_CFLAGS="$CFLAGS_NOPIE $QEMU_CFLAGS"
   QEMU_LDFLAGS="$LDFLAGS_NOPIE $QEMU_LDFLAGS"
-elif compile_prog "-fPIE -DPIE" "-pie"; then
+elif compile_prog "-Werror -fPIE -DPIE" "-pie"; then
   QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
   QEMU_LDFLAGS="-pie $QEMU_LDFLAGS"
   pie="yes"
-- 
2.20.1

[PULL 13/13] tcg/i386: Fix %r12 guest_base initialization

[Alex Bennée]

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

When %gs cannot be used, we use register offset addressing.
This path is almost never used, so it was clearly not tested.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200406174803.8192-1-richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 7f61eeedd09..ec083bddcfb 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -3737,7 +3737,7 @@ static void tcg_target_qemu_prologue(TCGContext *s)
         } else {
             /* Choose R12 because, as a base, it requires a SIB byte. */
             x86_guest_base_index = TCG_REG_R12;
-            tcg_out_mov(s, TCG_TYPE_PTR, x86_guest_base_index, guest_base);
+            tcg_out_movi(s, TCG_TYPE_PTR, x86_guest_base_index, guest_base);
             tcg_regset_set_reg(s->reserved_regs, x86_guest_base_index);
         }
     }
-- 
2.20.1

Re: [PULL 12/13] configure: Add -Werror to PIE probe

[Mark Cave-Ayland]

On 07/04/2020 16:51, Alex Bennée wrote:

> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Without -Werror, the probe may succeed, but then compilation fails
> later when -Werror is added for other reasons.  Shows up on windows,
> where the compiler complains about -fPIC.
> 
> 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>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20200401214756.6559-1-richard.henderson@linaro.org>
> Message-Id: <20200403191150.863-13-alex.bennee@linaro.org>
> 
> diff --git a/configure b/configure
> index 22870f38672..233c671aaa9 100755
> --- a/configure
> +++ b/configure
> @@ -2119,7 +2119,7 @@ if compile_prog "-Werror -fno-pie" "-no-pie"; then
>  fi
>  
>  if test "$static" = "yes"; then
> -  if test "$pie" != "no" && compile_prog "-fPIE -DPIE" "-static-pie"; then
> +  if test "$pie" != "no" && compile_prog "-Werror -fPIE -DPIE" "-static-pie"; then
>      QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
>      QEMU_LDFLAGS="-static-pie $QEMU_LDFLAGS"
>      pie="yes"
> @@ -2132,7 +2132,7 @@ if test "$static" = "yes"; then
>  elif test "$pie" = "no"; then
>    QEMU_CFLAGS="$CFLAGS_NOPIE $QEMU_CFLAGS"
>    QEMU_LDFLAGS="$LDFLAGS_NOPIE $QEMU_LDFLAGS"
> -elif compile_prog "-fPIE -DPIE" "-pie"; then
> +elif compile_prog "-Werror -fPIE -DPIE" "-pie"; then
>    QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
>    QEMU_LDFLAGS="-pie $QEMU_LDFLAGS"
>    pie="yes"

Note that even with this build fix, the native and cross-compiled Windows binaries
crash upon startup unless --disable-pie is explicitly passed to configure (this seems
like an unexpected side-effect of d2cd29e307: "configure: Do not force pie=no for
non-x86").

Given that users such as Howard and https://bugs.launchpad.net/bugs/1870911 are
starting to notice, would it be best to set this configure option as the default for
Windows builds for now?


ATB,

Mark.

Re: [PULL 12/13] configure: Add -Werror to PIE probe

[Richard Henderson]

On 4/7/20 9:08 AM, Mark Cave-Ayland wrote:
> Given that users such as Howard and https://bugs.launchpad.net/bugs/1870911 are
> starting to notice, would it be best to set this configure option as the default for
> Windows builds for now?

I guess so.

I'm mildly annoyed that Howard's compiler is silently accepting -fPIE and then
producing incorrect code, rather than rejecting -fPIE as the docker win32
cross-compiler is doing.


r~

Re: [PULL for 5.0-rc2 00/13] various fixes

[Peter Maydell]

On Tue, 7 Apr 2020 at 16:51, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The following changes since commit 53ef8a92eb04ee19640f5aad3bff36cd4a36c250:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200406' into staging (2020-04-06 12:36:45 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/stsquad/qemu.git tags/pull-misc-fixes-070420-1
>
> for you to fetch changes up to cce743abbf398a324879039cd582349b36da0ea6:
>
>   tcg/i386: Fix %r12 guest_base initialization (2020-04-07 16:19:49 +0100)
>
> ----------------------------------------------------------------
> Various fixes:
>
>   - add .github repo lockdown config
>   - better handle missing symbols in elf-ops
>   - protect fcntl64 with #ifdef
>   - remove unused macros from test
>   - fix handling of /proc/self/maps
>   - avoid BAD_SHIFT in x80 softfloat
>   - properly terminate on .hex EOF
>   - fix configure probe on windows cross build
>   - fix %r12 guest_base initialization



Applied, thanks.

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

-- PMM

Re: [PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Aleksandar Markovic]

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

17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла:
>
> From: Richard Henderson <richard.henderson@linaro.org>
>
> All other calls to normalize*Subnormal detect zero input before
> the call -- this is the only outlier.  This case can happen with
> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of
> the correct sign.
>
> Reported-by: Coverity (CID 1421991)
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org>
> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org>
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 301ce3b537b..ae6ba718540 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a,
floatx80 b, flag zSign,
>          zSig1 = 0;
>          zSig0 = aSig + bSig;
>          if ( aExp == 0 ) {
> +            if (zSig0 == 0) {
> +                return packFloatx80(zSign, 0, 0);
> +            }
>              normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 );
>              goto roundAndPack;
>          }
> --
> 2.20.1
>
>

We in MIPS have extensive FP tests, that certainly include many cases of
operations with +0 and -0. And they are all correct even before this patch.

Unfortunately, because of my current remote work, I don't havecthese tests
with me, and can't confirm if they work correctly, or perhaps are
unaffected at all.

Alex, from the commit message, it not clear if this is a fix of a bug (in
which case a test example would be useful to have, and the assesment on
what scenarios could be affected), or just a correction for some rare
condition that practically for all intents and purposes was never
triggered, or perhaps something third.

Alex, please explain this in more detail to me.

Secondly, and not related to this patch only, I see more and more patches
integrated into the main tree without "Reviewed-by:" tag. I don't think
this is the best way an open source community works. In my personal
opinion, this must stop.

Regards,
Aleksandar

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

Re: [PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Richard Henderson]

On 4/10/20 2:38 AM, Aleksandar Markovic wrote:
> 17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org
> <mailto:alex.bennee@linaro.org>> је написао/ла:
>>
>> From: Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>>
>>
>> All other calls to normalize*Subnormal detect zero input before
>> the call -- this is the only outlier.  This case can happen with
>> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of
>> the correct sign.
>>
>> Reported-by: Coverity (CID 1421991)
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org
> <mailto:alex.bennee@linaro.org>>
>> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org
> <mailto:20200327232042.10008-1-richard.henderson@linaro.org>>
>> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org
> <mailto:20200403191150.863-8-alex.bennee@linaro.org>>
>>
>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> index 301ce3b537b..ae6ba718540 100644
>> --- a/fpu/softfloat.c
>> +++ b/fpu/softfloat.c
>> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b,
> flag zSign,
>>          zSig1 = 0;
>>          zSig0 = aSig + bSig;
>>          if ( aExp == 0 ) {
>> +            if (zSig0 == 0) {
>> +                return packFloatx80(zSign, 0, 0);
>> +            }
>>              normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 );
>>              goto roundAndPack;
>>          }
>> --
>> 2.20.1
>>
>>
> 
> We in MIPS have extensive FP tests, that certainly include many cases of
> operations with +0 and -0. And they are all correct even before this patch.

This is for the 80-bit extended-double type, only used on x86 and m68k.  You
will not execute this path using MIPS.

> Alex, from the commit message, it not clear if this is a fix of a bug (in which
> case a test example would be useful to have, and the assesment on what
> scenarios could be affected), or just a correction for some rare condition that
> practically for all intents and purposes was never triggered, or perhaps
> something third.

This only avoids a Coverity out-of-range shift warning.

Beforehand, we executed 0 << 64, got 0 as the result (regardless of whether or
not the host truncates the shift count), and constructed the correctly signed
fp zero in the end.

There was more discussion about this in an earlier thread, associated with a
different patch for this same problem:

https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg08278.html


> Secondly, and not related to this patch only, I see more and more patches
> integrated into the main tree without "Reviewed-by:" tag. I don't think this is
> the best way an open source community works. In my personal opinion, this must
> stop.

The only way to avoid this is to have more developers review code outside their
own bailiwick.  The patch has been on the list for two weeks and was pinged
twice.

Although why Alex didn't add his own R-b to my patch when merging it to his
branch, I don't know.


r~

Re: [PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Aleksandar Markovic]

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

17:17 Pet, 10.04.2020. Richard Henderson <richard.henderson@linaro.org> је
написао/ла:
>
> On 4/10/20 2:38 AM, Aleksandar Markovic wrote:
> > 17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org
> > <mailto:alex.bennee@linaro.org>> је написао/ла:
> >>
> >> From: Richard Henderson <richard.henderson@linaro.org
> > <mailto:richard.henderson@linaro.org>>
> >>
> >> All other calls to normalize*Subnormal detect zero input before
> >> the call -- this is the only outlier.  This case can happen with
> >> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of
> >> the correct sign.
> >>
> >> Reported-by: Coverity (CID 1421991)
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org
> > <mailto:richard.henderson@linaro.org>>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org
> > <mailto:alex.bennee@linaro.org>>
> >> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org
> > <mailto:20200327232042.10008-1-richard.henderson@linaro.org>>
> >> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org
> > <mailto:20200403191150.863-8-alex.bennee@linaro.org>>
> >>
> >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> >> index 301ce3b537b..ae6ba718540 100644
> >> --- a/fpu/softfloat.c
> >> +++ b/fpu/softfloat.c
> >> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a,
floatx80 b,
> > flag zSign,
> >>          zSig1 = 0;
> >>          zSig0 = aSig + bSig;
> >>          if ( aExp == 0 ) {
> >> +            if (zSig0 == 0) {
> >> +                return packFloatx80(zSign, 0, 0);
> >> +            }
> >>              normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 );
> >>              goto roundAndPack;
> >>          }
> >> --
> >> 2.20.1
> >>
> >>
> >
> > We in MIPS have extensive FP tests, that certainly include many cases of
> > operations with +0 and -0. And they are all correct even before this
patch.
>
> This is for the 80-bit extended-double type, only used on x86 and m68k.
You
> will not execute this path using MIPS.
>

Thanks, Richard!

First and foremost, may health be with you and all people of United States
and all Americas!!

Yes, the fact that m68k also uses 80-bit FP arithmetic was known to me.
Though probably many people think 80-bit FP is limited to x86.

I was just afraid of some strange way that other targets may end up using
the function in question. But again, thanks for reassurances!

> > Alex, from the commit message, it not clear if this is a fix of a bug
(in which
> > case a test example would be useful to have, and the assesment on what
> > scenarios could be affected), or just a correction for some rare
condition that
> > practically for all intents and purposes was never triggered, or perhaps
> > something third.
>
> This only avoids a Coverity out-of-range shift warning.
>
> Beforehand, we executed 0 << 64, got 0 as the result (regardless of
whether or
> not the host truncates the shift count), and constructed the correctly
signed
> fp zero in the end.
>
> There was more discussion about this in an earlier thread, associated
with a
> different patch for this same problem:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg08278.html
>

OK. I didn't have a chance to read this pattivular thread. Thanks for the
pointer!

>
> > Secondly, and not related to this patch only, I see more and more
patches
> > integrated into the main tree without "Reviewed-by:" tag. I don't think
this is
> > the best way an open source community works. In my personal opinion,
this must
> > stop.
>
> The only way to avoid this is to have more developers review code outside
their
> own bailiwick.  The patch has been on the list for two weeks and was
pinged
> twice.
>
> Although why Alex didn't add his own R-b to my patch when merging it to
his
> branch, I don't know.
>

I also have a very similar impression as yours, that Alex in fact reviewed
the patch (as if he implicitely gave R-B, but forgot to insert it in a
hurry, given these hectic days around 5.0 final release).

Best regards, and best wishes to all Sietlans, or wherever you happen to
live!

Aleksandar

>
> r~

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

Re: [PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Peter Maydell]

On Fri, 10 Apr 2020 at 16:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Although why Alex didn't add his own R-b to my patch when merging it to his
> branch, I don't know.

I think this is one of those areas where different submaintainers
have different work practices. Personally I distinguish "did I
actually review this" from "did I just put this into my tree and
rely on others doing the review" and use r-by for the former
and not on the latter (although obviously everything I put in
my tree I will have at least very very briefly looked over).
But I think some submaintainers don't bother to add r-by tags
for things they review in the process of assembling their
tree because they see it as implicit in the process.

thanks
-- PMM

Re: [PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Aleksandar Markovic]

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

18:14 Pet, 10.04.2020. Peter Maydell <peter.maydell@linaro.org> је
написао/ла:

> But I think some submaintainers don't bother to add r-by tags
> for things they review in the process of assembling their
> tree because they see it as implicit in the process.
>

I think that was precisely the case in this patch.

May I wish you, Peter, the best health, and thanks UK for giving the world
Dr. John Campbell from northern England, whose videos I watch every day
with the closest possible attention, and the highest admiration.

Aleksandar

> thanks
> -- PMM

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

Re: [PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Alex Bennée]

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

> On Fri, 10 Apr 2020 at 16:17, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Although why Alex didn't add his own R-b to my patch when merging it to his
>> branch, I don't know.
>
> I think this is one of those areas where different submaintainers
> have different work practices. Personally I distinguish "did I
> actually review this" from "did I just put this into my tree and
> rely on others doing the review" and use r-by for the former
> and not on the latter (although obviously everything I put in
> my tree I will have at least very very briefly looked over).
> But I think some submaintainers don't bother to add r-by tags
> for things they review in the process of assembling their
> tree because they see it as implicit in the process.

It was exactly this - pulling in via my tree and adding my own s-o-b
implies I'm happy enough with it. Typically for longer series that
gestate on the list the total number of r-b tags grows with each re-roll
until the series gets pulled into a maintainer branch. This PR is
atypical in that regard because it's a fairly random collection of fixes.

I think the only patches we should be wary of are those with only a
single s-o-b tag from the author. I have to admit there was one such
patch in this PR:

  Subject: [PULL 09/13] linux-user: factor out reading of /proc/self/maps
  Date: Tue,  7 Apr 2020 16:51:14 +0100
  Message-Id: <20200407155118.20139-10-alex.bennee@linaro.org>

I made an executive decision to include it as it was part of the bug fix
for patch 10 and as we approach RC releases I wanted to get it merged.
If you follow the msg-id in the patch you will see the changes in the
patch are purely in response to review comments so while missing a r-b
tag it's not like it's not been on the list and had some scrutiny.
However we should certainly aim for most patches to be fully reviewed
even if we never achieve that level of perfection.

>
> thanks
> -- PMM


-- 
Alex Bennée