[PATCH for 5.0 v1 0/7] A selection of sanitiser fixes

[PATCH for 5.0 v1 0/7] A selection of sanitiser fixes

[Alex Bennée]

Hi,

I gave the rc0 a spin on the clang sanitiser and found a number of
small issues. One issue is that init_guest_space doesn't play nice
with the sanitiser for some guests but that is going to be a more
involved fix. For now I've just enhanced the debug output a little. I
also didn't attempt to fix the memory leak in xtensa as the code is
pretty unfamiliar to me. Please review and NACK anything you don't
think should be 5.0 material.

Alex Bennée (6):
  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
  fpu/softfloat: avoid undefined behaviour when normalising empty sigs
  target/xtensa: add FIXME for translation memory leak

Denis Plotnikov (1):
  gdbstub: fix compiler complaining

 include/hw/elf_ops.h           |  7 ++++++-
 fpu/softfloat.c                | 11 ++++++++---
 gdbstub.c                      |  4 ++--
 linux-user/elfload.c           |  8 +++++++-
 linux-user/syscall.c           |  8 ++++----
 target/xtensa/translate.c      |  5 +++++
 tests/tcg/x86_64/system/boot.S |  5 +----
 7 files changed, 33 insertions(+), 15 deletions(-)

-- 
2.20.1

[PATCH v1 1/7] 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.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/elf_ops.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index a1411bfcab6..b5d4074d1e3 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -170,8 +170,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) {
+        goto fail;
+    }
+
+    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) {
-- 
2.20.1

[PATCH v1 2/7] 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>
---
 linux-user/syscall.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 49395dcea97..a3da46d69f9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11223,11 +11223,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;
 
@@ -11238,7 +11238,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

[PATCH v1 3/7] 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>
---
 tests/tcg/x86_64/system/boot.S | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

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

[PATCH v1 4/7] 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>
---
 linux-user/elfload.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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

[PATCH v1 5/7] fpu/softfloat: avoid undefined behaviour when normalising empty sigs

[Alex Bennée]

The undefined behaviour checker pointed out that a shift of 64 would
lead to undefined behaviour.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 fpu/softfloat.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 301ce3b537b..444d35920dd 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,
 {
     int8_t shiftCount;
 
-    shiftCount = clz64(aSig);
-    *zSigPtr = aSig<<shiftCount;
-    *zExpPtr = 1 - shiftCount;
+    if (aSig) {
+        shiftCount = clz64(aSig);
+        *zSigPtr = aSig << shiftCount;
+        *zExpPtr = 1 - shiftCount;
+    } else {
+        *zSigPtr = 0;
+        *zExpPtr = 1 - 64;
+    }
 }
 
 /*----------------------------------------------------------------------------
-- 
2.20.1

[PATCH v1 6/7] 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>
Cc: Max Filippov <jcmvbkbc@gmail.com>
---
 target/xtensa/translate.c | 5 +++++
 1 file changed, 5 insertions(+)

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

[PATCH v1 7/7] 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: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

Re: [PATCH v1 5/7] fpu/softfloat: avoid undefined behaviour when normalising empty sigs

[Peter Maydell]

On Fri, 27 Mar 2020 at 09:49, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The undefined behaviour checker pointed out that a shift of 64 would
> lead to undefined behaviour.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  fpu/softfloat.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 301ce3b537b..444d35920dd 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,
>  {
>      int8_t shiftCount;
>
> -    shiftCount = clz64(aSig);
> -    *zSigPtr = aSig<<shiftCount;
> -    *zExpPtr = 1 - shiftCount;
> +    if (aSig) {
> +        shiftCount = clz64(aSig);
> +        *zSigPtr = aSig << shiftCount;
> +        *zExpPtr = 1 - shiftCount;
> +    } else {
> +        *zSigPtr = 0;
> +        *zExpPtr = 1 - 64;
> +    }
>  }

Ah yes, I saw this one in Coverity: CID 1421991.

RTH marked the Coverity issue as a false positive with the rationale
"We assume an out-of-range shift count is merely IMPLEMENTATION DEFINED
 and not UNDEFINED (in the Arm ARM sense), and so cannot turn a 0 value
 into a non-zero value."
but I think I disagree with that. We can assume that for the TCG IR
where we get to define shift semantics because we're doing the codegen,
but we can't assume it in C code, because it's not included in the set
of extended guarantees provided by -fwrapv as far as I know.

That said, is it valid for this function to be called with a zero
aSig value ? I think all these normalizeFloat*Subnormal() functions
assume non-zero sig input, and the only callsite where it's not clearly
obvious that this is obvious that the sig input is non-zero is the call to
normalizeFloatx80Subnormal() from addFloatx80Sigs(). So perhaps we
just need to check and fix that callsite ??

thanks
-- PMM

Re: [PATCH v1 5/7] fpu/softfloat: avoid undefined behaviour when normalising empty sigs

[Aleksandar Markovic]

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

11:53 Pet, 27.03.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла:
>
> The undefined behaviour checker

Alex, what exactly is "undefined behaviour checker"? If this is a test, can
you give us a link?

Sincerely,
Aleksandar

> pointed out that a shift of 64 would
> lead to undefined behaviour.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  fpu/softfloat.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 301ce3b537b..444d35920dd 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig,
int32_t *zExpPtr,
>  {
>      int8_t shiftCount;
>
> -    shiftCount = clz64(aSig);
> -    *zSigPtr = aSig<<shiftCount;
> -    *zExpPtr = 1 - shiftCount;
> +    if (aSig) {
> +        shiftCount = clz64(aSig);
> +        *zSigPtr = aSig << shiftCount;
> +        *zExpPtr = 1 - shiftCount;
> +    } else {
> +        *zSigPtr = 0;
> +        *zExpPtr = 1 - 64;
> +    }
>  }
>
>
 /*----------------------------------------------------------------------------
> --
> 2.20.1
>
>

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

Re: [PATCH v1 5/7] fpu/softfloat: avoid undefined behaviour when normalising empty sigs

[Alex Bennée]

Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> writes:

> 11:53 Pet, 27.03.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла:
>>
>> The undefined behaviour checker
>
> Alex, what exactly is "undefined behaviour checker"? If this is a test, can
> you give us a link?

It's enabled by our sanitizers build:

  ../../configure --cc=clang-8 --cxx=clang++-8 --enable-sanitizers

>
> Sincerely,
> Aleksandar
>
>> pointed out that a shift of 64 would
>> lead to undefined behaviour.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  fpu/softfloat.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> index 301ce3b537b..444d35920dd 100644
>> --- a/fpu/softfloat.c
>> +++ b/fpu/softfloat.c
>> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig,
> int32_t *zExpPtr,
>>  {
>>      int8_t shiftCount;
>>
>> -    shiftCount = clz64(aSig);
>> -    *zSigPtr = aSig<<shiftCount;
>> -    *zExpPtr = 1 - shiftCount;
>> +    if (aSig) {
>> +        shiftCount = clz64(aSig);
>> +        *zSigPtr = aSig << shiftCount;
>> +        *zExpPtr = 1 - shiftCount;
>> +    } else {
>> +        *zSigPtr = 0;
>> +        *zExpPtr = 1 - 64;
>> +    }
>>  }
>>
>>
>  /*----------------------------------------------------------------------------
>> --
>> 2.20.1
>>
>>


-- 
Alex Bennée

Re: [PATCH v1 2/7] linux-user: protect fcntl64 with an #ifdef

[Laurent Vivier]

Le 27/03/2020 à 10:49, Alex Bennée a écrit :
> 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>
> ---
>  linux-user/syscall.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 49395dcea97..a3da46d69f9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11223,11 +11223,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;
>  
> @@ -11238,7 +11238,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;
>          }
> 

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

Re: [PATCH v1 4/7] linux-user: more debug for init_guest_space

[Laurent Vivier]

Le 27/03/2020 à 10:49, Alex Bennée a écrit :
> 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>
> ---
>  linux-user/elfload.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> 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
> 

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

Re: [PATCH for 5.0 v1 0/7] A selection of sanitiser fixes

[no-reply]

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



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===




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

Re: [PATCH v1 1/7] elf-ops: bail out if we have no function symbols

[Philippe Mathieu-Daudé]

On 3/27/20 10:49 AM, Alex Bennée wrote:
> 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.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/elf_ops.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index a1411bfcab6..b5d4074d1e3 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -170,8 +170,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) {
> +        goto fail;
> +    }
> +
> +    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) {
> 

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

Re: [PATCH v1 3/7] tests/tcg: remove extraneous pasting macros

[Philippe Mathieu-Daudé]

On 3/27/20 10:49 AM, Alex Bennée wrote:
> We are not using them and they just get in the way.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/tcg/x86_64/system/boot.S | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> 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)
> 

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

Re: [PATCH v1 7/7] gdbstub: fix compiler complaining

[Philippe Mathieu-Daudé]

On 3/27/20 10:49 AM, Alex Bennée wrote:
> 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: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   gdbstub.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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 {
> 

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

Re: [PATCH v1 1/7] elf-ops: bail out if we have no function symbols

[Philippe Mathieu-Daudé]

On 3/27/20 11:53 AM, Philippe Mathieu-Daudé wrote:
> On 3/27/20 10:49 AM, Alex Bennée wrote:
>> 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.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   include/hw/elf_ops.h | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index a1411bfcab6..b5d4074d1e3 100644
>> --- a/include/hw/elf_ops.h
>> +++ b/include/hw/elf_ops.h
>> @@ -170,8 +170,13 @@ static int glue(load_symbols, SZ)(struct elfhdr 
>> *ehdr, int fd, int must_swab,
>>           }
>>           i++;
>>       }
>> -    syms = g_realloc(syms, nsyms * sizeof(*syms));

Something was bugging me why looking at this line, now I remembered: 
another patch from 2 years ago :)

https://www.mail-archive.com/qemu-devel@nongnu.org/msg528713.html

Is this the same emitted warning? It seems.

   $ qemu-system-xtensa -M kc705 -m 128M -semihosting -nographic 
-monitor null -kernel Image.elf
   include/hw/elf_ops.h:179:5: runtime error: null pointer passed as 
argument 1, which is declared to never be null

If so, can you add it to the commit description?

Thanks!

>> +    /* check we have symbols left */
>> +    if (nsyms == 0) {
>> +        goto fail;
>> +    }
>> +
>> +    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) {
>>
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v1 1/7] elf-ops: bail out if we have no function symbols

[Peter Maydell]

On Fri, 27 Mar 2020 at 09:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/elf_ops.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index a1411bfcab6..b5d4074d1e3 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -170,8 +170,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) {
> +        goto fail;
> +    }
> +
> +    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) {

If "ELF file has no symbols" is valid, it's a bit odd for
load_symbols to report it as a failure by returning -1.
This only works because load_elf (the only caller) just
ignores the return value entirely. OTOH I suppose you
could argue that we can just ignore any oddity in the
attempt to load symbols (eg bogus/malformad symtab section).
If so, we should probably drop the return value from
load_symbols().

thanks
-- PMM

Re: [PATCH v1 1/7] elf-ops: bail out if we have no function symbols

[Richard Henderson]

On 3/27/20 2:49 AM, Alex Bennée wrote:
> 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.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/elf_ops.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

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

r~

Re: [PATCH v1 2/7] linux-user: protect fcntl64 with an #ifdef

[Richard Henderson]

On 3/27/20 2:49 AM, Alex Bennée wrote:
> 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>
> ---
>  linux-user/syscall.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

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

r~

Re: [PATCH v1 3/7] tests/tcg: remove extraneous pasting macros

[Richard Henderson]

On 3/27/20 2:49 AM, Alex Bennée wrote:
> We are not using them and they just get in the way.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/x86_64/system/boot.S | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

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

r~

Re: [PATCH v1 5/7] fpu/softfloat: avoid undefined behaviour when normalising empty sigs

[Richard Henderson]

On 3/27/20 3:09 AM, Peter Maydell wrote:
> On Fri, 27 Mar 2020 at 09:49, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> The undefined behaviour checker pointed out that a shift of 64 would
>> lead to undefined behaviour.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  fpu/softfloat.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> index 301ce3b537b..444d35920dd 100644
>> --- a/fpu/softfloat.c
>> +++ b/fpu/softfloat.c
>> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,
>>  {
>>      int8_t shiftCount;
>>
>> -    shiftCount = clz64(aSig);
>> -    *zSigPtr = aSig<<shiftCount;
>> -    *zExpPtr = 1 - shiftCount;
>> +    if (aSig) {
>> +        shiftCount = clz64(aSig);
>> +        *zSigPtr = aSig << shiftCount;
>> +        *zExpPtr = 1 - shiftCount;
>> +    } else {
>> +        *zSigPtr = 0;
>> +        *zExpPtr = 1 - 64;
>> +    }
>>  }
> 
> Ah yes, I saw this one in Coverity: CID 1421991.
> 
> RTH marked the Coverity issue as a false positive with the rationale
> "We assume an out-of-range shift count is merely IMPLEMENTATION DEFINED
>  and not UNDEFINED (in the Arm ARM sense), and so cannot turn a 0 value
>  into a non-zero value."
> but I think I disagree with that. We can assume that for the TCG IR
> where we get to define shift semantics because we're doing the codegen,
> but we can't assume it in C code, because it's not included in the set
> of extended guarantees provided by -fwrapv as far as I know.

Perhaps.  Of course we also know from our broad knowledge of architectures,
that a compiler would really have to go out of its way for this to happen.

I really hate C in this way, sometimes.

I wonder if I have the energy to petition the committee to drop, for C202? all
of the "undefined" nonsense that only applies to sign-magnitute and
ones-compliment computers, which haven't been seen since the 70's...

> That said, is it valid for this function to be called with a zero
> aSig value ? I think all these normalizeFloat*Subnormal() functions
> assume non-zero sig input, and the only callsite where it's not clearly
> obvious that this is obvious that the sig input is non-zero is the call to
> normalizeFloatx80Subnormal() from addFloatx80Sigs(). So perhaps we
> just need to check and fix that callsite ??

You're right -- addFloatx80Sigs is the only use out of 26 that doesn't have a
preceding check for 0.


r~

Re: [PATCH v1 5/7] fpu/softfloat: avoid undefined behaviour when normalising empty sigs

[Peter Maydell]

On Fri, 27 Mar 2020 at 22:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
> I wonder if I have the energy to petition the committee to drop, for C202? all
> of the "undefined" nonsense that only applies to sign-magnitute and
> ones-compliment computers, which haven't been seen since the 70's...

There was certainly a proposal to do that (I think from a Google
engineer) for C++, I forget whether the equivalent C change has
also been proposed.

> > That said, is it valid for this function to be called with a zero
> > aSig value ? I think all these normalizeFloat*Subnormal() functions
> > assume non-zero sig input, and the only callsite where it's not clearly
> > obvious that this is obvious that the sig input is non-zero is the call to
> > normalizeFloatx80Subnormal() from addFloatx80Sigs(). So perhaps we
> > just need to check and fix that callsite ??
>
> You're right -- addFloatx80Sigs is the only use out of 26 that doesn't have a
> preceding check for 0.

Mmm. My vote is for fixing addFloatx80Sigs -- now we just need
to figure out what the desired behaviour is.

thanks
-- PMM