[PATCH 00/11] Fixes for clang-13 plus tcg/ppc

[PATCH 00/11] Fixes for clang-13 plus tcg/ppc

[Richard Henderson]

The goal here was to address Brad's report for clang vs ppc32.

Somewhere in between here and there I forgot about the ppc32 part,
needed a newer clang for gcc135, accidentally built master instead
of the clang-12 release branch, fixed a bunch of buggy looking
things, and only then remembered I was building ppc64 and wasn't
going to test what I thought I would.

So: Brad, could you double-check this fixes your problem?

Others: Only patch 7 obviously should have been using the
variable indicated as unused.  But please double-check.


r~


Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Brad Smith <brad@comstyle.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Eric Blake <eblake@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Greg Kurz <groug@kaod.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: qemu-block@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


Richard Henderson (11):
  nbd/server: Remove unused variable
  accel/tcg: Remove unused variable in cpu_exec
  util/selfmap: Discard mapping on error
  net/checksum: Remove unused variable in net_checksum_add_iov
  hw/audio/adlib: Remove unused variable in adlib_callback
  hw/ppc/spapr_events: Remove unused variable from check_exception
  hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write
  linux-user/syscall: Remove unused variable from execve
  tests/unit: Remove unused variable from test_io
  tcg/ppc: Replace TCG_TARGET_CALL_DARWIN with _CALL_DARWIN
  tcg/ppc: Ensure _CALL_SYSV is set for 32-bit ELF

 accel/tcg/cpu-exec.c     |  3 ---
 hw/audio/adlib.c         |  3 +--
 hw/pci-host/pnv_phb4.c   |  2 +-
 hw/ppc/spapr_events.c    |  5 -----
 linux-user/syscall.c     |  3 ---
 nbd/server.c             |  4 ----
 net/checksum.c           |  4 +---
 tests/unit/test-iov.c    |  5 +----
 util/selfmap.c           | 28 ++++++++++++++++------------
 tcg/ppc/tcg-target.c.inc | 25 ++++++++++++++++++++-----
 10 files changed, 40 insertions(+), 42 deletions(-)

-- 
2.25.1

[PATCH 01/11] nbd/server: Remove unused variable

[Richard Henderson]

From clang-13:
nbd/server.c:976:22: error: variable 'bitmaps' set but not used \
    [-Werror,-Wunused-but-set-variable]

Cc: qemu-block@nongnu.org
Cc: Eric Blake <eblake@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 nbd/server.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index b60ebc3ab6..721349ec00 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
 {
     int ret;
     g_autofree char *export_name = NULL;
-    g_autofree bool *bitmaps = NULL;
     NBDExportMetaContexts local_meta = {0};
     uint32_t nb_queries;
     size_t i;
@@ -1007,9 +1006,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
                             "export '%s' not present", sane_name);
     }
     meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps);
-    if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
-        bitmaps = meta->bitmaps;
-    }
 
     ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), false, errp);
     if (ret <= 0) {
-- 
2.25.1

Re: [PATCH 01/11] nbd/server: Remove unused variable

[Vladimir Sementsov-Ogievskiy]

13.07.2021 00:55, Richard Henderson wrote:
>  From clang-13:
> nbd/server.c:976:22: error: variable 'bitmaps' set but not used \
>      [-Werror,-Wunused-but-set-variable]
> 
> Cc: qemu-block@nongnu.org
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   nbd/server.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index b60ebc3ab6..721349ec00 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>   {
>       int ret;
>       g_autofree char *export_name = NULL;
> -    g_autofree bool *bitmaps = NULL;
>       NBDExportMetaContexts local_meta = {0};
>       uint32_t nb_queries;
>       size_t i;
> @@ -1007,9 +1006,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>                               "export '%s' not present", sane_name);
>       }
>       meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps);
> -    if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
> -        bitmaps = meta->bitmaps;
> -    }
>   
>       ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), false, errp);
>       if (ret <= 0) {
> 


Hm. I'm afraid, this way meta->bitmaps will be leaked in NBD_OPT_LIST_META_CONTEXT case.

Actually, "bitmaps" _is_ used, in cleanup handler, setup by g_autofree. So it's a false positive.


-- 
Best regards,
Vladimir

Re: [PATCH 01/11] nbd/server: Remove unused variable

[Eric Blake]

On Tue, Jul 13, 2021 at 12:27:48PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 13.07.2021 00:55, Richard Henderson wrote:
> >  From clang-13:
> > nbd/server.c:976:22: error: variable 'bitmaps' set but not used \
> >      [-Werror,-Wunused-but-set-variable]
> > 

> > +++ b/nbd/server.c
> > @@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
> >   {
> >       int ret;
> >       g_autofree char *export_name = NULL;
> > -    g_autofree bool *bitmaps = NULL;
> >       NBDExportMetaContexts local_meta = {0};
> >       uint32_t nb_queries;
> >       size_t i;
> > @@ -1007,9 +1006,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
> >                               "export '%s' not present", sane_name);
> >       }
> >       meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps);
> > -    if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
> > -        bitmaps = meta->bitmaps;
> > -    }
> >       ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), false, errp);
> >       if (ret <= 0) {
> > 
> 
> 
> Hm. I'm afraid, this way meta->bitmaps will be leaked in NBD_OPT_LIST_META_CONTEXT case.
> 
> Actually, "bitmaps" _is_ used, in cleanup handler, setup by g_autofree. So it's a false positive.
>

Correct; this patch is wrong, and would cause a memory leak. This is a
false positive in clang, and a known issue that clang is in general
unable to see that g_autofree variables are used, sometimes for their
intentional side effects such as easier memory cleanup as done here.

I suspect that the definition of g_autofree already uses
__attribute__((unused)) to work around clang's oddities, which means
I'm not sure how to silence clang on this one.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 01/11] nbd/server: Remove unused variable

[Eric Blake]

On Tue, Jul 13, 2021 at 08:01:34AM -0500, Eric Blake wrote:
> > > @@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
> > >   {
> > >       int ret;
> > >       g_autofree char *export_name = NULL;
> > > -    g_autofree bool *bitmaps = NULL;
> > >       NBDExportMetaContexts local_meta = {0};
> > Actually, "bitmaps" _is_ used, in cleanup handler, setup by g_autofree. So it's a false positive.
> >
> 
> Correct; this patch is wrong, and would cause a memory leak. This is a
> false positive in clang, and a known issue that clang is in general
> unable to see that g_autofree variables are used, sometimes for their
> intentional side effects such as easier memory cleanup as done here.
> 
> I suspect that the definition of g_autofree already uses
> __attribute__((unused)) to work around clang's oddities, which means
> I'm not sure how to silence clang on this one.

Hmm; in glib 2.68.2 (on Fedora 34), g_autofree does NOT include an
attribute unused.  Thus, does this silence the compiler?  (Even cooler
would be making the comment a link to an actual bug in the clang
database, but I couldn't quickly find one)

diff --git i/nbd/server.c w/nbd/server.c
index b60ebc3ab6ac..393cbd81c57a 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -973,7 +973,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
 {
     int ret;
     g_autofree char *export_name = NULL;
-    g_autofree bool *bitmaps = NULL;
+    /* G_GNUC_UNUSED needed to work around a clang bug */
+    g_autofree G_GNUC_UNUSED bool *bitmaps = NULL;
     NBDExportMetaContexts local_meta = {0};
     uint32_t nb_queries;
     size_t i;


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 01/11] nbd/server: Remove unused variable

[Richard Henderson]

On 7/13/21 6:14 AM, Eric Blake wrote:
> Hmm; in glib 2.68.2 (on Fedora 34), g_autofree does NOT include an
> attribute unused.  Thus, does this silence the compiler?  (Even cooler
> would be making the comment a link to an actual bug in the clang
> database, but I couldn't quickly find one)
> 
> diff --git i/nbd/server.c w/nbd/server.c
> index b60ebc3ab6ac..393cbd81c57a 100644
> --- i/nbd/server.c
> +++ w/nbd/server.c
> @@ -973,7 +973,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>   {
>       int ret;
>       g_autofree char *export_name = NULL;
> -    g_autofree bool *bitmaps = NULL;
> +    /* G_GNUC_UNUSED needed to work around a clang bug */
> +    g_autofree G_GNUC_UNUSED bool *bitmaps = NULL;

That works.  I found

   https://bugs.llvm.org/show_bug.cgi?id=3888

and gave it a nudge.


r~

[PATCH 02/11] accel/tcg: Remove unused variable in cpu_exec

[Richard Henderson]

From clang-13:
accel/tcg/cpu-exec.c:783:15: error: variable 'cc' set but not used \
    [-Werror,-Wunused-but-set-variable]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index e22bcb99f7..a8d8cea586 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -780,7 +780,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
 
 int cpu_exec(CPUState *cpu)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
     int ret;
     SyncClocks sc = { 0 };
 
@@ -819,14 +818,12 @@ int cpu_exec(CPUState *cpu)
          * so we only perform the workaround for clang.
          */
         cpu = current_cpu;
-        cc = CPU_GET_CLASS(cpu);
 #else
         /*
          * Non-buggy compilers preserve these locals; assert that
          * they have the correct value.
          */
         g_assert(cpu == current_cpu);
-        g_assert(cc == CPU_GET_CLASS(cpu));
 #endif
 
 #ifndef CONFIG_SOFTMMU
-- 
2.25.1

Re: [PATCH 02/11] accel/tcg: Remove unused variable in cpu_exec

[Philippe Mathieu-Daudé]

On 7/12/21 11:55 PM, Richard Henderson wrote:
> From clang-13:
> accel/tcg/cpu-exec.c:783:15: error: variable 'cc' set but not used \
>     [-Werror,-Wunused-but-set-variable]
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/cpu-exec.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index e22bcb99f7..a8d8cea586 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -780,7 +780,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
>  
>  int cpu_exec(CPUState *cpu)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>      int ret;
>      SyncClocks sc = { 0 };
>  
> @@ -819,14 +818,12 @@ int cpu_exec(CPUState *cpu)
>           * so we only perform the workaround for clang.
>           */
>          cpu = current_cpu;
> -        cc = CPU_GET_CLASS(cpu);
>  #else
>          /*
>           * Non-buggy compilers preserve these locals; assert that
>           * they have the correct value.

Maybe update comment to singular, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>           */
>          g_assert(cpu == current_cpu);
> -        g_assert(cc == CPU_GET_CLASS(cpu));
>  #endif
>  
>  #ifndef CONFIG_SOFTMMU
> 

[PATCH 03/11] util/selfmap: Discard mapping on error

[Richard Henderson]

From clang-13:
util/selfmap.c:26:21: error: variable 'errors' set but not used \
    [-Werror,-Wunused-but-set-variable]

Quite right of course, but there's no reason not to check errors.

First, incrementing errors is incorrect, because qemu_strtoul
returns an errno not a count -- just or them together so that
we have a non-zero value at the end.

Second, if we have an error, do not add the struct to the list,
but free it instead.

Cc: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 util/selfmap.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/util/selfmap.c b/util/selfmap.c
index 2ec99dfdda..0178c2ff8b 100644
--- a/util/selfmap.c
+++ b/util/selfmap.c
@@ -23,29 +23,33 @@ GSList *read_self_maps(void)
             gchar **fields = g_strsplit(lines[i], " ", 6);
             if (g_strv_length(fields) > 4) {
                 MapInfo *e = g_new0(MapInfo, 1);
-                int errors;
+                int errors = 0;
                 const char *end;
 
-                errors  = qemu_strtoul(fields[0], &end, 16, &e->start);
-                errors += qemu_strtoul(end + 1, NULL, 16, &e->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);
+                errors |= qemu_strtoul(fields[2], NULL, 16, &e->offset);
                 e->dev = g_strdup(fields[3]);
-                errors += qemu_strtou64(fields[4], NULL, 10, &e->inode);
+                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]));
+                if (!errors) {
+                    /*
+                     * 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);
+                } else {
+                    g_free(e);
                 }
-                map_info = g_slist_prepend(map_info, e);
             }
 
             g_strfreev(fields);
-- 
2.25.1

Re: [PATCH 03/11] util/selfmap: Discard mapping on error

[Eric Blake]

On Mon, Jul 12, 2021 at 02:55:27PM -0700, Richard Henderson wrote:
> From clang-13:
> util/selfmap.c:26:21: error: variable 'errors' set but not used \
>     [-Werror,-Wunused-but-set-variable]
> 
> Quite right of course, but there's no reason not to check errors.
> 
> First, incrementing errors is incorrect, because qemu_strtoul
> returns an errno not a count -- just or them together so that
> we have a non-zero value at the end.
> 
> Second, if we have an error, do not add the struct to the list,
> but free it instead.
> 
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  util/selfmap.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)

> 
> diff --git a/util/selfmap.c b/util/selfmap.c
> index 2ec99dfdda..0178c2ff8b 100644
> --- a/util/selfmap.c
> +++ b/util/selfmap.c
> @@ -23,29 +23,33 @@ GSList *read_self_maps(void)
>              gchar **fields = g_strsplit(lines[i], " ", 6);
>              if (g_strv_length(fields) > 4) {
>                  MapInfo *e = g_new0(MapInfo, 1);
> -                int errors;
> +                int errors = 0;
>                  const char *end;
>  
> -                errors  = qemu_strtoul(fields[0], &end, 16, &e->start);
> -                errors += qemu_strtoul(end + 1, NULL, 16, &e->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);
> +                errors |= qemu_strtoul(fields[2], NULL, 16, &e->offset);
>                  e->dev = g_strdup(fields[3]);

e->dev now contains malloc'd memory...

> -                errors += qemu_strtou64(fields[4], NULL, 10, &e->inode);
> +                errors |= qemu_strtou64(fields[4], NULL, 10, &e->inode);

...and if this qemu_strtou64 fails...

>  
> -                /*
> -                 * 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]));
> +                if (!errors) {
> +                    /*
> +                     * 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);
> +                } else {
> +                    g_free(e);

...you've now leaked it.  Oops.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 03/11] util/selfmap: Discard mapping on error

[Richard Henderson]

On 7/13/21 9:06 AM, Eric Blake wrote:
>>                   e->dev = g_strdup(fields[3]);
> 
> e->dev now contains malloc'd memory...
> 
>> -                errors += qemu_strtou64(fields[4], NULL, 10, &e->inode);
>> +                errors |= qemu_strtou64(fields[4], NULL, 10, &e->inode);
> 
> ...and if this qemu_strtou64 fails...
> 
>>   
>> -                /*
>> -                 * 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]));
>> +                if (!errors) {
>> +                    /*
>> +                     * 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);
>> +                } else {
>> +                    g_free(e);
> 
> ...you've now leaked it.  Oops.

Yep, thanks.


r~

[PATCH 04/11] net/checksum: Remove unused variable in net_checksum_add_iov

[Richard Henderson]

From clang-13:
../qemu/net/checksum.c:189:23: error: variable 'buf_off' set but not used \
    [-Werror,-Wunused-but-set-variable]

Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 net/checksum.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/checksum.c b/net/checksum.c
index 70f4eaeb3a..68245fd748 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -186,12 +186,11 @@ uint32_t
 net_checksum_add_iov(const struct iovec *iov, const unsigned int iov_cnt,
                      uint32_t iov_off, uint32_t size, uint32_t csum_offset)
 {
-    size_t iovec_off, buf_off;
+    size_t iovec_off;
     unsigned int i;
     uint32_t res = 0;
 
     iovec_off = 0;
-    buf_off = 0;
     for (i = 0; i < iov_cnt && size; i++) {
         if (iov_off < (iovec_off + iov[i].iov_len)) {
             size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off , size);
@@ -200,7 +199,6 @@ net_checksum_add_iov(const struct iovec *iov, const unsigned int iov_cnt,
             res += net_checksum_add_cont(len, chunk_buf, csum_offset);
             csum_offset += len;
 
-            buf_off += len;
             iov_off += len;
             size -= len;
         }
-- 
2.25.1

[PATCH 05/11] hw/audio/adlib: Remove unused variable in adlib_callback

[Richard Henderson]

From clang-13:
hw/audio/adlib.c:189:18: error: variable 'net' set but not used \
    [-Werror,-Wunused-but-set-variable]

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/audio/adlib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
index 42d50d2fdc..5f979b1487 100644
--- a/hw/audio/adlib.c
+++ b/hw/audio/adlib.c
@@ -186,7 +186,7 @@ static int write_audio (AdlibState *s, int samples)
 static void adlib_callback (void *opaque, int free)
 {
     AdlibState *s = opaque;
-    int samples, net = 0, to_play, written;
+    int samples, to_play, written;
 
     samples = free >> SHIFT;
     if (!(s->active && s->enabled) || !samples) {
@@ -219,7 +219,6 @@ static void adlib_callback (void *opaque, int free)
         written = write_audio (s, samples);
 
         if (written) {
-            net += written;
             samples -= written;
             s->pos = (s->pos + written) % s->samples;
         }
-- 
2.25.1

Re: [PATCH 05/11] hw/audio/adlib: Remove unused variable in adlib_callback

[Philippe Mathieu-Daudé]

On 7/12/21 11:55 PM, Richard Henderson wrote:
> From clang-13:
> hw/audio/adlib.c:189:18: error: variable 'net' set but not used \
>     [-Werror,-Wunused-but-set-variable]
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/audio/adlib.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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

[PATCH 06/11] hw/ppc/spapr_events: Remove unused variable from check_exception

[Richard Henderson]

From clang-13:
hw/ppc/spapr_events.c:937:14: error: variable 'xinfo' set but not used \
    [-Werror,-Wunused-but-set-variable]

Cc: qemu-ppc@nongnu.org
Cc: Greg Kurz <groug@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/ppc/spapr_events.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 0cfc19be19..23e2e2fff1 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -934,7 +934,6 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
                             uint32_t nret, target_ulong rets)
 {
     uint32_t mask, buf, len, event_len;
-    uint64_t xinfo;
     SpaprEventLogEntry *event;
     struct rtas_error_log header;
     int i;
@@ -944,13 +943,9 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
         return;
     }
 
-    xinfo = rtas_ld(args, 1);
     mask = rtas_ld(args, 2);
     buf = rtas_ld(args, 4);
     len = rtas_ld(args, 5);
-    if (nargs == 7) {
-        xinfo |= (uint64_t)rtas_ld(args, 6) << 32;
-    }
 
     event = rtas_event_log_dequeue(spapr, mask);
     if (!event) {
-- 
2.25.1

Re: [PATCH 06/11] hw/ppc/spapr_events: Remove unused variable from check_exception

[David Gibson]

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

On Mon, Jul 12, 2021 at 02:55:30PM -0700, Richard Henderson wrote:
> >From clang-13:
> hw/ppc/spapr_events.c:937:14: error: variable 'xinfo' set but not used \
>     [-Werror,-Wunused-but-set-variable]
> 
> Cc: qemu-ppc@nongnu.org
> Cc: Greg Kurz <groug@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr_events.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 0cfc19be19..23e2e2fff1 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -934,7 +934,6 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
>                              uint32_t nret, target_ulong rets)
>  {
>      uint32_t mask, buf, len, event_len;
> -    uint64_t xinfo;
>      SpaprEventLogEntry *event;
>      struct rtas_error_log header;
>      int i;
> @@ -944,13 +943,9 @@ static void check_exception(PowerPCCPU *cpu, SpaprMachineState *spapr,
>          return;
>      }
>  
> -    xinfo = rtas_ld(args, 1);
>      mask = rtas_ld(args, 2);
>      buf = rtas_ld(args, 4);
>      len = rtas_ld(args, 5);
> -    if (nargs == 7) {
> -        xinfo |= (uint64_t)rtas_ld(args, 6) << 32;
> -    }
>  
>      event = rtas_event_log_dequeue(spapr, mask);
>      if (!event) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 07/11] hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write

[Richard Henderson]

From clang-13:
hw/pci-host/pnv_phb4.c:375:18: error: variable 'v' set but not used \
    [-Werror,-Wunused-but-set-variable]

It's pretty clear that we meant to write back 'v' after
all that computation and not 'val'.

Cc: qemu-ppc@nongnu.org
Cc: Greg Kurz <groug@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/pci-host/pnv_phb4.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 54f57c660a..5c375a9f28 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -392,7 +392,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t val)
             v &= 0xffffffffffff0000ull;
             v |= 0x000000000000cfffull & val;
         }
-        *tptr = val;
+        *tptr = v;
         break;
     }
     case IODA3_TBL_MBT:
-- 
2.25.1

Re: [PATCH 07/11] hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write

[David Gibson]

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

On Mon, Jul 12, 2021 at 02:55:31PM -0700, Richard Henderson wrote:
> >From clang-13:
> hw/pci-host/pnv_phb4.c:375:18: error: variable 'v' set but not used \
>     [-Werror,-Wunused-but-set-variable]
> 
> It's pretty clear that we meant to write back 'v' after
> all that computation and not 'val'.
> 
> Cc: qemu-ppc@nongnu.org
> Cc: Greg Kurz <groug@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/pci-host/pnv_phb4.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 54f57c660a..5c375a9f28 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -392,7 +392,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t val)
>              v &= 0xffffffffffff0000ull;
>              v |= 0x000000000000cfffull & val;
>          }
> -        *tptr = val;
> +        *tptr = v;
>          break;
>      }
>      case IODA3_TBL_MBT:

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 08/11] linux-user/syscall: Remove unused variable from execve

[Richard Henderson]

From clang-13:
linux-user/syscall.c:8503:17: error: variable 'total_size' set but not used \
    [-Werror,-Wunused-but-set-variable]

Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2e826206d2..c27debba8c 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8500,7 +8500,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
             abi_ulong guest_envp;
             abi_ulong addr;
             char **q;
-            int total_size = 0;
 
             argc = 0;
             guest_argp = arg2;
@@ -8532,7 +8531,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                     break;
                 if (!(*q = lock_user_string(addr)))
                     goto execve_efault;
-                total_size += strlen(*q) + 1;
             }
             *q = NULL;
 
@@ -8544,7 +8542,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                     break;
                 if (!(*q = lock_user_string(addr)))
                     goto execve_efault;
-                total_size += strlen(*q) + 1;
             }
             *q = NULL;
 
-- 
2.25.1

Re: [PATCH 08/11] linux-user/syscall: Remove unused variable from execve

[Laurent Vivier]

Le 12/07/2021 à 23:55, Richard Henderson a écrit :
> From clang-13:
> linux-user/syscall.c:8503:17: error: variable 'total_size' set but not used \
>     [-Werror,-Wunused-but-set-variable]
> 
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/syscall.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2e826206d2..c27debba8c 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8500,7 +8500,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>              abi_ulong guest_envp;
>              abi_ulong addr;
>              char **q;
> -            int total_size = 0;
>  
>              argc = 0;
>              guest_argp = arg2;
> @@ -8532,7 +8531,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>                      break;
>                  if (!(*q = lock_user_string(addr)))
>                      goto execve_efault;
> -                total_size += strlen(*q) + 1;
>              }
>              *q = NULL;
>  
> @@ -8544,7 +8542,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>                      break;
>                  if (!(*q = lock_user_string(addr)))
>                      goto execve_efault;
> -                total_size += strlen(*q) + 1;
>              }
>              *q = NULL;
>  
> 

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

[PATCH 09/11] tests/unit: Remove unused variable from test_io

[Richard Henderson]

From clang-13:
tests/unit/test-iov.c:161:26: error: variable 't' set but not used \
    [-Werror,-Wunused-but-set-variable]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/unit/test-iov.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/unit/test-iov.c b/tests/unit/test-iov.c
index 9c415e2f1f..5371066fb6 100644
--- a/tests/unit/test-iov.c
+++ b/tests/unit/test-iov.c
@@ -158,7 +158,7 @@ static void test_io(void)
 
     int sv[2];
     int r;
-    unsigned i, j, k, s, t;
+    unsigned i, j, k, s;
     fd_set fds;
     unsigned niov;
     struct iovec *iov, *siov;
@@ -182,7 +182,6 @@ static void test_io(void)
 
     FD_ZERO(&fds);
 
-    t = 0;
     if (fork() == 0) {
        /* writer */
 
@@ -201,7 +200,6 @@ static void test_io(void)
                    g_assert(memcmp(iov, siov, sizeof(*iov)*niov) == 0);
                    if (r >= 0) {
                        k += r;
-                       t += r;
                        usleep(g_test_rand_int_range(0, 30));
                    } else if (errno == EAGAIN) {
                        select(sv[1]+1, NULL, &fds, NULL, NULL);
@@ -238,7 +236,6 @@ static void test_io(void)
                    g_assert(memcmp(iov, siov, sizeof(*iov)*niov) == 0);
                    if (r > 0) {
                        k += r;
-                       t += r;
                    } else if (!r) {
                        if (s) {
                            break;
-- 
2.25.1

Re: [PATCH 09/11] tests/unit: Remove unused variable from test_io

[Philippe Mathieu-Daudé]

On 7/12/21 11:55 PM, Richard Henderson wrote:
> From clang-13:
> tests/unit/test-iov.c:161:26: error: variable 't' set but not used \
>     [-Werror,-Wunused-but-set-variable]
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tests/unit/test-iov.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

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

[PATCH 10/11] tcg/ppc: Replace TCG_TARGET_CALL_DARWIN with _CALL_DARWIN

[Richard Henderson]

If __APPLE__, ensure that _CALL_DARWIN is set, then remove
our local TCG_TARGET_CALL_DARWIN.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/ppc/tcg-target.c.inc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc
index e0f4665213..2202ce017e 100644
--- a/tcg/ppc/tcg-target.c.inc
+++ b/tcg/ppc/tcg-target.c.inc
@@ -25,8 +25,8 @@
 #include "elf.h"
 #include "../tcg-pool.c.inc"
 
-#if defined _CALL_DARWIN || defined __APPLE__
-#define TCG_TARGET_CALL_DARWIN
+#if !defined _CALL_DARWIN && defined __APPLE__
+#define _CALL_DARWIN 1
 #endif
 #ifdef _CALL_SYSV
 # define TCG_TARGET_CALL_ALIGN_ARGS   1
@@ -169,7 +169,7 @@ static const int tcg_target_call_oarg_regs[] = {
 };
 
 static const int tcg_target_callee_save_regs[] = {
-#ifdef TCG_TARGET_CALL_DARWIN
+#ifdef _CALL_DARWIN
     TCG_REG_R11,
 #endif
     TCG_REG_R14,
@@ -2372,7 +2372,7 @@ static void tcg_out_nop_fill(tcg_insn_unit *p, int count)
 # define LINK_AREA_SIZE                (6 * SZR)
 # define LR_OFFSET                     (1 * SZR)
 # define TCG_TARGET_CALL_STACK_OFFSET  (LINK_AREA_SIZE + 8 * SZR)
-#elif defined(TCG_TARGET_CALL_DARWIN)
+#elif defined(_CALL_DARWIN)
 # define LINK_AREA_SIZE                (6 * SZR)
 # define LR_OFFSET                     (2 * SZR)
 #elif TCG_TARGET_REG_BITS == 64
-- 
2.25.1

[PATCH 11/11] tcg/ppc: Ensure _CALL_SYSV is set for 32-bit ELF

[Richard Henderson]

Clang only sets _CALL_ELF for ppc64, and nothing at all to specify
the ABI for ppc32.  Make a good guess based on other symbols.

Reported-by: Brad Smith <brad@comstyle.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/ppc/tcg-target.c.inc | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc
index 2202ce017e..5e1fac914a 100644
--- a/tcg/ppc/tcg-target.c.inc
+++ b/tcg/ppc/tcg-target.c.inc
@@ -25,9 +25,24 @@
 #include "elf.h"
 #include "../tcg-pool.c.inc"
 
-#if !defined _CALL_DARWIN && defined __APPLE__
-#define _CALL_DARWIN 1
-#endif
+/*
+ * Standardize on the _CALL_FOO symbols used by GCC:
+ * Apple XCode does not define _CALL_DARWIN.
+ * Clang defines _CALL_ELF (64-bit) but not _CALL_SYSV (32-bit).
+ */
+#if !defined(_CALL_SYSV) && \
+    !defined(_CALL_DARWIN) && \
+    !defined(_CALL_AIX) && \
+    !defined(_CALL_ELF)
+# if defined(__APPLE__)
+#  define _CALL_DARWIN
+# elif defined(__ELF__) && TCG_TARGET_REG_BITS == 32
+#  define _CALL_SYSV
+# else
+#  error "Unknown ABI"
+# endif
+#endif 
+
 #ifdef _CALL_SYSV
 # define TCG_TARGET_CALL_ALIGN_ARGS   1
 #endif
-- 
2.25.1

Re: [PATCH 00/11] Fixes for clang-13 plus tcg/ppc

[Brad Smith]

On 7/12/2021 5:55 PM, Richard Henderson wrote:
> The goal here was to address Brad's report for clang vs ppc32.
>
> Somewhere in between here and there I forgot about the ppc32 part,
> needed a newer clang for gcc135, accidentally built master instead
> of the clang-12 release branch, fixed a bunch of buggy looking
> things, and only then remembered I was building ppc64 and wasn't
> going to test what I thought I would.
>
> So: Brad, could you double-check this fixes your problem?

Yes, this does. Thank you.

> Others: Only patch 7 obviously should have been using the
> variable indicated as unused.  But please double-check.
>
>
> r~
>
>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Brad Smith <brad@comstyle.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Greg Kurz <groug@kaod.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: qemu-block@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
> Richard Henderson (11):
>    nbd/server: Remove unused variable
>    accel/tcg: Remove unused variable in cpu_exec
>    util/selfmap: Discard mapping on error
>    net/checksum: Remove unused variable in net_checksum_add_iov
>    hw/audio/adlib: Remove unused variable in adlib_callback
>    hw/ppc/spapr_events: Remove unused variable from check_exception
>    hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write
>    linux-user/syscall: Remove unused variable from execve
>    tests/unit: Remove unused variable from test_io
>    tcg/ppc: Replace TCG_TARGET_CALL_DARWIN with _CALL_DARWIN
>    tcg/ppc: Ensure _CALL_SYSV is set for 32-bit ELF
>
>   accel/tcg/cpu-exec.c     |  3 ---
>   hw/audio/adlib.c         |  3 +--
>   hw/pci-host/pnv_phb4.c   |  2 +-
>   hw/ppc/spapr_events.c    |  5 -----
>   linux-user/syscall.c     |  3 ---
>   nbd/server.c             |  4 ----
>   net/checksum.c           |  4 +---
>   tests/unit/test-iov.c    |  5 +----
>   util/selfmap.c           | 28 ++++++++++++++++------------
>   tcg/ppc/tcg-target.c.inc | 25 ++++++++++++++++++++-----
>   10 files changed, 40 insertions(+), 42 deletions(-)
>