[PATCH 00/19] -Werror=maybe-uninitialized fixes

[PATCH 00/19] -Werror=maybe-uninitialized fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Depending on -Doptimization=<value>, GCC (13.2.1 here) produces different
maybe-uninitialized warnings:
- g: produces -Werror=maybe-uninitialized errors
- 0: clean build
- 1: produces -Werror=maybe-uninitialized errors
- 2: clean build
- 3: produces few -Werror=maybe-uninitialized errors
- s: produces -Werror=maybe-uninitialized errors

Most are false-positive, because prior LOCK_GUARD should guarantee an
initialization path. Few of them are a bit trickier. Finally, I found
a potential related memory leak.

thanks

Marc-André Lureau (19):
  util/coroutine: fix -Werror=maybe-uninitialized false-positive
  util/timer: with -Werror=maybe-uninitialized false-positive
  hw/qxl: fix -Werror=maybe-uninitialized false-positives
  nbd: with -Werror=maybe-uninitialized false-positive
  block/mirror: fix -Werror=maybe-uninitialized false-positive
  block/stream: fix -Werror=maybe-uninitialized false-positives
  hw/ahci: fix -Werror=maybe-uninitialized false-positive
  hw/vhost-scsi: fix -Werror=maybe-uninitialized
  hw/sdhci: fix -Werror=maybe-uninitialized false-positive
  hw/rdma: fix -Werror=maybe-uninitialized false-positive
  migration/block: fix -Werror=maybe-uninitialized false-positive
  migration: fix -Werror=maybe-uninitialized false-positives
  hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive
  plugins: fix -Werror=maybe-uninitialized false-positive
  migration: fix -Werror=maybe-uninitialized false-positive
  tests: fix -Werror=maybe-uninitialized
  hw/nvme: fix -Werror=maybe-uninitialized
  hw/virtio: fix -Werror=maybe-uninitialized
  RFC: hw/virtio: a potential leak fix

 block/mirror.c                     | 2 +-
 block/stream.c                     | 6 +++---
 hw/block/virtio-blk.c              | 2 +-
 hw/display/qxl.c                   | 4 ++--
 hw/ide/ahci.c                      | 3 ++-
 hw/nvme/ctrl.c                     | 2 +-
 hw/rdma/rdma_backend.c             | 2 +-
 hw/scsi/vhost-scsi.c               | 2 +-
 hw/sd/sdhci.c                      | 2 +-
 hw/virtio/vhost-shadow-virtqueue.c | 6 ++++--
 migration/block.c                  | 2 +-
 migration/dirtyrate.c              | 4 ++--
 migration/migration.c              | 2 +-
 migration/ram.c                    | 2 +-
 nbd/client-connection.c            | 2 +-
 plugins/loader.c                   | 2 +-
 tests/unit/test-bdrv-drain.c       | 2 +-
 tests/unit/test-block-iothread.c   | 2 +-
 util/qemu-coroutine.c              | 2 +-
 util/qemu-timer.c                  | 6 +++---
 20 files changed, 30 insertions(+), 27 deletions(-)

-- 
2.44.0

[PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../util/qemu-coroutine.c:150:8: error: ‘batch’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/qemu-coroutine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index eb4eebefdf..64d6264fc7 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -136,7 +136,7 @@ static Coroutine *coroutine_pool_get_local(void)
 static void coroutine_pool_refill_local(void)
 {
     CoroutinePool *local_pool = get_ptr_local_pool();
-    CoroutinePoolBatch *batch;
+    CoroutinePoolBatch *batch = NULL;
 
     WITH_QEMU_LOCK_GUARD(&global_pool_lock) {
         batch = QSLIST_FIRST(&global_pool);
-- 
2.44.0

Re: [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive

[Stefan Hajnoczi]

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

On Thu, Mar 28, 2024 at 02:20:34PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../util/qemu-coroutine.c:150:8: error: ‘batch’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  util/qemu-coroutine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[PATCH 02/19] util/timer: with -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../util/qemu-timer.c:198:24: error: ‘expire_time’ may be used uninitialized [-Werror=maybe-uninitialized]
../util/qemu-timer.c:476:8: error: ‘rearm’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/qemu-timer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 6a0de33dd2..12b22cf69b 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -182,7 +182,7 @@ bool qemu_clock_has_timers(QEMUClockType type)
 
 bool timerlist_expired(QEMUTimerList *timer_list)
 {
-    int64_t expire_time;
+    int64_t expire_time = 0;
 
     if (!qatomic_read(&timer_list->active_timers)) {
         return false;
@@ -212,7 +212,7 @@ bool qemu_clock_expired(QEMUClockType type)
 int64_t timerlist_deadline_ns(QEMUTimerList *timer_list)
 {
     int64_t delta;
-    int64_t expire_time;
+    int64_t expire_time = 0;
 
     if (!qatomic_read(&timer_list->active_timers)) {
         return -1;
@@ -461,7 +461,7 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
 void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time)
 {
     QEMUTimerList *timer_list = ts->timer_list;
-    bool rearm;
+    bool rearm = false;
 
     WITH_QEMU_LOCK_GUARD(&timer_list->active_timers_lock) {
         if (ts->expire_time == -1 || ts->expire_time > expire_time) {
-- 
2.44.0

[PATCH 03/19] hw/qxl: fix -Werror=maybe-uninitialized false-positives

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/display/qxl.c:1352:5: error: ‘pci_region’ may be used uninitialized [-Werror=maybe-uninitialized]
../hw/display/qxl.c:1365:22: error: ‘pci_start’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/qxl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 7178dec85d..cfea4e7af5 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1301,8 +1301,8 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
     };
     uint64_t guest_start;
     uint64_t guest_end;
-    int pci_region;
-    pcibus_t pci_start;
+    int pci_region = -1;
+    pcibus_t pci_start = -1;
     pcibus_t pci_end;
     MemoryRegion *mr;
     intptr_t virt_start;
-- 
2.44.0

[PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../nbd/client-connection.c:419:8: error: ‘wait_co’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 nbd/client-connection.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index f9da67c87e..b11e266807 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -410,7 +410,7 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
  */
 void nbd_co_establish_connection_cancel(NBDClientConnection *conn)
 {
-    Coroutine *wait_co;
+    Coroutine *wait_co = NULL;
 
     WITH_QEMU_LOCK_GUARD(&conn->mutex) {
         wait_co = g_steal_pointer(&conn->wait_co);
-- 
2.44.0

Re: [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive

[Eric Blake]

On Thu, Mar 28, 2024 at 02:20:37PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../nbd/client-connection.c:419:8: error: ‘wait_co’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  nbd/client-connection.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> index f9da67c87e..b11e266807 100644
> --- a/nbd/client-connection.c
> +++ b/nbd/client-connection.c
> @@ -410,7 +410,7 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
>   */
>  void nbd_co_establish_connection_cancel(NBDClientConnection *conn)
>  {
> -    Coroutine *wait_co;
> +    Coroutine *wait_co = NULL;
>  
>      WITH_QEMU_LOCK_GUARD(&conn->mutex) {
>          wait_co = g_steal_pointer(&conn->wait_co);
> -- 
> 2.44.0
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

[PATCH 05/19] block/mirror: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../block/mirror.c:1066:22: error: ‘iostatus’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 1bdce3b657..53dd7332ee 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -926,7 +926,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
     MirrorBDSOpaque *mirror_top_opaque = s->mirror_top_bs->opaque;
     BlockDriverState *target_bs = blk_bs(s->target);
     bool need_drain = true;
-    BlockDeviceIoStatus iostatus;
+    BlockDeviceIoStatus iostatus = BLOCK_DEVICE_IO_STATUS__MAX;
     int64_t length;
     int64_t target_length;
     BlockDriverInfo bdi;
-- 
2.44.0

Re: [PATCH 05/19] block/mirror: fix -Werror=maybe-uninitialized false-positive

[Vladimir Sementsov-Ogievskiy]

On 28.03.24 13:20, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../block/mirror.c:1066:22: error: ‘iostatus’ may be used uninitialized [-Werror=maybe-uninitialized]

Actually that's a false-positive.. Compiler can't believe that body of WITH_JOB_LOCK_GUARD() will be executed unconditionally. Probably we should mention this in a comment.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   block/mirror.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 1bdce3b657..53dd7332ee 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -926,7 +926,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>       MirrorBDSOpaque *mirror_top_opaque = s->mirror_top_bs->opaque;
>       BlockDriverState *target_bs = blk_bs(s->target);
>       bool need_drain = true;
> -    BlockDeviceIoStatus iostatus;
> +    BlockDeviceIoStatus iostatus = BLOCK_DEVICE_IO_STATUS__MAX;
>       int64_t length;
>       int64_t target_length;
>       BlockDriverInfo bdi;

-- 
Best regards,
Vladimir

[PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized [-Werror=maybe-uninitialized]
../block/stream.c:176:5: error: ‘len’ may be used uninitialized [-Werror=maybe-uninitialized]
trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/stream.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 7031eef12b..9076203193 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -155,8 +155,8 @@ static void stream_clean(Job *job)
 static int coroutine_fn stream_run(Job *job, Error **errp)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-    BlockDriverState *unfiltered_bs;
-    int64_t len;
+    BlockDriverState *unfiltered_bs = NULL;
+    int64_t len = -1;
     int64_t offset = 0;
     int error = 0;
     int64_t n = 0; /* bytes */
@@ -177,7 +177,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
 
     for ( ; offset < len; offset += n) {
         bool copy;
-        int ret;
+        int ret = -1;
 
         /* Note that even when no rate limit is applied we need to yield
          * with no pending I/O here so that bdrv_drain_all() returns.
-- 
2.44.0

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Vladimir Sementsov-Ogievskiy]

On 28.03.24 13:20, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized [-Werror=maybe-uninitialized]
> ../block/stream.c:176:5: error: ‘len’ may be used uninitialized [-Werror=maybe-uninitialized]
> trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..

Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?

Actually, "unused variable initialization" is bad thing too.

Anyway, if no better solution for now:
Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   block/stream.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 7031eef12b..9076203193 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -155,8 +155,8 @@ static void stream_clean(Job *job)
>   static int coroutine_fn stream_run(Job *job, Error **errp)
>   {
>       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> -    BlockDriverState *unfiltered_bs;
> -    int64_t len;
> +    BlockDriverState *unfiltered_bs = NULL;
> +    int64_t len = -1;
>       int64_t offset = 0;
>       int error = 0;
>       int64_t n = 0; /* bytes */
> @@ -177,7 +177,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>   
>       for ( ; offset < len; offset += n) {
>           bool copy;
> -        int ret;
> +        int ret = -1;
>   
>           /* Note that even when no rate limit is applied we need to yield
>            * with no pending I/O here so that bdrv_drain_all() returns.

-- 
Best regards,
Vladimir

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Marc-André Lureau]

Hi

On Fri, Mar 29, 2024 at 12:35 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 28.03.24 13:20, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > ../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized [-Werror=maybe-uninitialized]
> > ../block/stream.c:176:5: error: ‘len’ may be used uninitialized [-Werror=maybe-uninitialized]
> > trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
>
> Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?
>


#define WITH_QEMU_LOCK_GUARD_(x, var) \
    for (g_autoptr(QemuLockable) var = \
                qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
         var; \
         qemu_lockable_auto_unlock(var), var = NULL)

I can't think of a clever way to rewrite this. The compiler probably
thinks the loop may not run, due to the "var" condition. But how to
convince it otherwise? it's hard to introduce another variable too..

> Actually, "unused variable initialization" is bad thing too.
>
> Anyway, if no better solution for now:
> Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> > ---
> >   block/stream.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/stream.c b/block/stream.c
> > index 7031eef12b..9076203193 100644
> > --- a/block/stream.c
> > +++ b/block/stream.c
> > @@ -155,8 +155,8 @@ static void stream_clean(Job *job)
> >   static int coroutine_fn stream_run(Job *job, Error **errp)
> >   {
> >       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> > -    BlockDriverState *unfiltered_bs;
> > -    int64_t len;
> > +    BlockDriverState *unfiltered_bs = NULL;
> > +    int64_t len = -1;
> >       int64_t offset = 0;
> >       int error = 0;
> >       int64_t n = 0; /* bytes */
> > @@ -177,7 +177,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
> >
> >       for ( ; offset < len; offset += n) {
> >           bool copy;
> > -        int ret;
> > +        int ret = -1;
> >
> >           /* Note that even when no rate limit is applied we need to yield
> >            * with no pending I/O here so that bdrv_drain_all() returns.
>
> --
> Best regards,
> Vladimir
>
>


-- 
Marc-André Lureau

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Vladimir Sementsov-Ogievskiy]

On 02.04.24 12:12, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Mar 29, 2024 at 12:35 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> On 28.03.24 13:20, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> ../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized [-Werror=maybe-uninitialized]
>>> ../block/stream.c:176:5: error: ‘len’ may be used uninitialized [-Werror=maybe-uninitialized]
>>> trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
>>
>> Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?
>>
> 
> 
> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>      for (g_autoptr(QemuLockable) var = \
>                  qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
>           var; \
>           qemu_lockable_auto_unlock(var), var = NULL)
> 
> I can't think of a clever way to rewrite this. The compiler probably
> thinks the loop may not run, due to the "var" condition. But how to
> convince it otherwise? it's hard to introduce another variable too..


hmm. maybe like this?

#define WITH_QEMU_LOCK_GUARD_(x, var) \
     for (g_autoptr(QemuLockable) var = \
                 qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
          var2 = (void *)(true); \
          var2; \
          qemu_lockable_auto_unlock(var), var2 = NULL)


probably, it would be simpler for compiler to understand the logic this way. Could you check?


(actually, will also need to construct var2 name same way as for var)

> 
>> Actually, "unused variable initialization" is bad thing too.
>>
>> Anyway, if no better solution for now:
>> Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>
>>> ---
>>>    block/stream.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/stream.c b/block/stream.c
>>> index 7031eef12b..9076203193 100644
>>> --- a/block/stream.c
>>> +++ b/block/stream.c
>>> @@ -155,8 +155,8 @@ static void stream_clean(Job *job)
>>>    static int coroutine_fn stream_run(Job *job, Error **errp)
>>>    {
>>>        StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>>> -    BlockDriverState *unfiltered_bs;
>>> -    int64_t len;
>>> +    BlockDriverState *unfiltered_bs = NULL;
>>> +    int64_t len = -1;
>>>        int64_t offset = 0;
>>>        int error = 0;
>>>        int64_t n = 0; /* bytes */
>>> @@ -177,7 +177,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>>>
>>>        for ( ; offset < len; offset += n) {
>>>            bool copy;
>>> -        int ret;
>>> +        int ret = -1;
>>>
>>>            /* Note that even when no rate limit is applied we need to yield
>>>             * with no pending I/O here so that bdrv_drain_all() returns.
>>
>> --
>> Best regards,
>> Vladimir
>>
>>
> 
> 

-- 
Best regards,
Vladimir

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Eric Blake]

On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
> > > 
> > > Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?
> > > 
> > 
> > 
> > #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >      for (g_autoptr(QemuLockable) var = \
> >                  qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
> >           var; \
> >           qemu_lockable_auto_unlock(var), var = NULL)
> > 
> > I can't think of a clever way to rewrite this. The compiler probably
> > thinks the loop may not run, due to the "var" condition. But how to
> > convince it otherwise? it's hard to introduce another variable too..
> 
> 
> hmm. maybe like this?
> 
> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>     for (g_autoptr(QemuLockable) var = \
>                 qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
>          var2 = (void *)(true); \
>          var2; \
>          qemu_lockable_auto_unlock(var), var2 = NULL)
> 
> 
> probably, it would be simpler for compiler to understand the logic this way. Could you check?

Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
point we could cause the compiler to call xxx((void*)(true)) if the
user does an early return inside the lock guard, with disastrous
consequences?  Or is the __attribute__ applied only to the first out
of two declarations in a list?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Vladimir Sementsov-Ogievskiy]

On 02.04.24 18:34, Eric Blake wrote:
> On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
>>>>
>>>> Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?
>>>>
>>>
>>>
>>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>>>       for (g_autoptr(QemuLockable) var = \
>>>                   qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
>>>            var; \
>>>            qemu_lockable_auto_unlock(var), var = NULL)
>>>
>>> I can't think of a clever way to rewrite this. The compiler probably
>>> thinks the loop may not run, due to the "var" condition. But how to
>>> convince it otherwise? it's hard to introduce another variable too..
>>
>>
>> hmm. maybe like this?
>>
>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>>      for (g_autoptr(QemuLockable) var = \
>>                  qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
>>           var2 = (void *)(true); \
>>           var2; \
>>           qemu_lockable_auto_unlock(var), var2 = NULL)
>>
>>
>> probably, it would be simpler for compiler to understand the logic this way. Could you check?
> 
> Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
> point we could cause the compiler to call xxx((void*)(true)) if the
> user does an early return inside the lock guard, with disastrous
> consequences?  Or is the __attribute__ applied only to the first out
> of two declarations in a list?
> 

Oh, most probably you are right, seems g_autoptr apply it to both variables. Also, we don't need qemu_lockable_auto_unlock(var) separate call, if we zero-out another variable. So, me fixing:

#define WITH_QEMU_LOCK_GUARD_(x, var) \
     for (QemuLockable *var __attribute__((cleanup(qemu_lockable_auto_unlock))) = qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
          *var2 = (void *)(true); \
          var2; \
          var2 = NULL)

(and we'll need to modify qemu_lockable_auto_unlock() to take "QemuLockable **x" argument)

-- 
Best regards,
Vladimir

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Marc-André Lureau]

Hi

On Tue, Apr 2, 2024 at 11:24 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 02.04.24 18:34, Eric Blake wrote:
> > On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >>>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
> >>>>
> >>>> Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?
> >>>>
> >>>
> >>>
> >>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>>       for (g_autoptr(QemuLockable) var = \
> >>>                   qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
> >>>            var; \
> >>>            qemu_lockable_auto_unlock(var), var = NULL)
> >>>
> >>> I can't think of a clever way to rewrite this. The compiler probably
> >>> thinks the loop may not run, due to the "var" condition. But how to
> >>> convince it otherwise? it's hard to introduce another variable too..
> >>
> >>
> >> hmm. maybe like this?
> >>
> >> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>      for (g_autoptr(QemuLockable) var = \
> >>                  qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
> >>           var2 = (void *)(true); \
> >>           var2; \
> >>           qemu_lockable_auto_unlock(var), var2 = NULL)
> >>
> >>
> >> probably, it would be simpler for compiler to understand the logic this way. Could you check?
> >
> > Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
> > point we could cause the compiler to call xxx((void*)(true)) if the
> > user does an early return inside the lock guard, with disastrous
> > consequences?  Or is the __attribute__ applied only to the first out
> > of two declarations in a list?
> >
>
> Oh, most probably you are right, seems g_autoptr apply it to both variables. Also, we don't need qemu_lockable_auto_unlock(var) separate call, if we zero-out another variable. So, me fixing:
>
> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>      for (QemuLockable *var __attribute__((cleanup(qemu_lockable_auto_unlock))) = qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
>           *var2 = (void *)(true); \
>           var2; \
>           var2 = NULL)
>
> (and we'll need to modify qemu_lockable_auto_unlock() to take "QemuLockable **x" argument)
>

That's almost good enough. I fixed a few things to generate var2.

I applied a similar approach to WITH_GRAPH_RDLOCK_GUARD macro:

--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -224,13 +224,22 @@ graph_lockable_auto_unlock(GraphLockable *x)

 G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)

-#define WITH_GRAPH_RDLOCK_GUARD_(var)                                         \
-    for (g_autoptr(GraphLockable) var = graph_lockable_auto_lock(GML_OBJ_()); \
-         var;                                                                 \
-         graph_lockable_auto_unlock(var), var = NULL)
+static inline void TSA_NO_TSA coroutine_fn
+graph_lockable_auto_cleanup(GraphLockable **x)
+{
+    graph_lockable_auto_unlock(*x);
+}
+
+#define WITH_GRAPH_RDLOCK_GUARD__(var) \
+    GraphLockable *var \
+        __attribute__((cleanup(graph_lockable_auto_cleanup))) G_GNUC_UNUSED = \
+       graph_lockable_auto_lock(GML_OBJ_())
+
+#define WITH_GRAPH_RDLOCK_GUARD_(var, var2)                             \
+    for (WITH_GRAPH_RDLOCK_GUARD__(var), *var2 = (void *)true; var2;
var2 = NULL)

 #define WITH_GRAPH_RDLOCK_GUARD() \
-    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
+    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__),
glue(graph_lockable_auto, __COUNTER__))

Unfortunately, it doesn't work in all cases. It seems to have issues
with some guards:
../block/stream.c: In function ‘stream_run’:
../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
[-Werror=maybe-uninitialized]
  216 |         if (ret < 0) {


What should we do? change the macros + cherry-pick the missing
false-positives, or keep this series as is?





-- 
Marc-André Lureau

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Vladimir Sementsov-Ogievskiy]

On 03.04.24 11:11, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Apr 2, 2024 at 11:24 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> On 02.04.24 18:34, Eric Blake wrote:
>>> On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
>>>>>>
>>>>>> Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?
>>>>>>
>>>>>
>>>>>
>>>>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>>>>>        for (g_autoptr(QemuLockable) var = \
>>>>>                    qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
>>>>>             var; \
>>>>>             qemu_lockable_auto_unlock(var), var = NULL)
>>>>>
>>>>> I can't think of a clever way to rewrite this. The compiler probably
>>>>> thinks the loop may not run, due to the "var" condition. But how to
>>>>> convince it otherwise? it's hard to introduce another variable too..
>>>>
>>>>
>>>> hmm. maybe like this?
>>>>
>>>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>>>>       for (g_autoptr(QemuLockable) var = \
>>>>                   qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
>>>>            var2 = (void *)(true); \
>>>>            var2; \
>>>>            qemu_lockable_auto_unlock(var), var2 = NULL)
>>>>
>>>>
>>>> probably, it would be simpler for compiler to understand the logic this way. Could you check?
>>>
>>> Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
>>> point we could cause the compiler to call xxx((void*)(true)) if the
>>> user does an early return inside the lock guard, with disastrous
>>> consequences?  Or is the __attribute__ applied only to the first out
>>> of two declarations in a list?
>>>
>>
>> Oh, most probably you are right, seems g_autoptr apply it to both variables. Also, we don't need qemu_lockable_auto_unlock(var) separate call, if we zero-out another variable. So, me fixing:
>>
>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>>       for (QemuLockable *var __attribute__((cleanup(qemu_lockable_auto_unlock))) = qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
>>            *var2 = (void *)(true); \
>>            var2; \
>>            var2 = NULL)
>>
>> (and we'll need to modify qemu_lockable_auto_unlock() to take "QemuLockable **x" argument)
>>
> 
> That's almost good enough. I fixed a few things to generate var2.
> 
> I applied a similar approach to WITH_GRAPH_RDLOCK_GUARD macro:
> 
> --- a/include/block/graph-lock.h
> +++ b/include/block/graph-lock.h
> @@ -224,13 +224,22 @@ graph_lockable_auto_unlock(GraphLockable *x)
> 
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)
> 
> -#define WITH_GRAPH_RDLOCK_GUARD_(var)                                         \
> -    for (g_autoptr(GraphLockable) var = graph_lockable_auto_lock(GML_OBJ_()); \
> -         var;                                                                 \
> -         graph_lockable_auto_unlock(var), var = NULL)
> +static inline void TSA_NO_TSA coroutine_fn
> +graph_lockable_auto_cleanup(GraphLockable **x)
> +{
> +    graph_lockable_auto_unlock(*x);
> +}
> +
> +#define WITH_GRAPH_RDLOCK_GUARD__(var) \
> +    GraphLockable *var \
> +        __attribute__((cleanup(graph_lockable_auto_cleanup))) G_GNUC_UNUSED = \
> +       graph_lockable_auto_lock(GML_OBJ_())
> +
> +#define WITH_GRAPH_RDLOCK_GUARD_(var, var2)                             \
> +    for (WITH_GRAPH_RDLOCK_GUARD__(var), *var2 = (void *)true; var2;
> var2 = NULL)
> 
>   #define WITH_GRAPH_RDLOCK_GUARD() \
> -    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
> +    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__),
> glue(graph_lockable_auto, __COUNTER__))
> 
> Unfortunately, it doesn't work in all cases. It seems to have issues
> with some guards:
> ../block/stream.c: In function ‘stream_run’:
> ../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
> [-Werror=maybe-uninitialized]
>    216 |         if (ret < 0) {
> 
> 

So, updated macro helps in some cases, but doesn't help here? Intersting, why.

> What should we do? change the macros + cherry-pick the missing
> false-positives, or keep this series as is?
> 
> 

I think marco + missing is better. No reason to add dead-initializations in cases where new macros helps.
Still, would be good to understand, what's the difference, why it help on some cases and not help in another.


-- 
Best regards,
Vladimir

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Marc-André Lureau]

Hi

On Wed, Apr 3, 2024 at 12:31 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 03.04.24 11:11, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Apr 2, 2024 at 11:24 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> On 02.04.24 18:34, Eric Blake wrote:
> >>> On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >>>>>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
> >>>>>>
> >>>>>> Didn't you try to change WITH_ macros somehow, so that compiler believe in our good intentions?
> >>>>>>
> >>>>>
> >>>>>
> >>>>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>>>>        for (g_autoptr(QemuLockable) var = \
> >>>>>                    qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
> >>>>>             var; \
> >>>>>             qemu_lockable_auto_unlock(var), var = NULL)
> >>>>>
> >>>>> I can't think of a clever way to rewrite this. The compiler probably
> >>>>> thinks the loop may not run, due to the "var" condition. But how to
> >>>>> convince it otherwise? it's hard to introduce another variable too..
> >>>>
> >>>>
> >>>> hmm. maybe like this?
> >>>>
> >>>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>>>       for (g_autoptr(QemuLockable) var = \
> >>>>                   qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
> >>>>            var2 = (void *)(true); \
> >>>>            var2; \
> >>>>            qemu_lockable_auto_unlock(var), var2 = NULL)
> >>>>
> >>>>
> >>>> probably, it would be simpler for compiler to understand the logic this way. Could you check?
> >>>
> >>> Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
> >>> point we could cause the compiler to call xxx((void*)(true)) if the
> >>> user does an early return inside the lock guard, with disastrous
> >>> consequences?  Or is the __attribute__ applied only to the first out
> >>> of two declarations in a list?
> >>>
> >>
> >> Oh, most probably you are right, seems g_autoptr apply it to both variables. Also, we don't need qemu_lockable_auto_unlock(var) separate call, if we zero-out another variable. So, me fixing:
> >>
> >> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>       for (QemuLockable *var __attribute__((cleanup(qemu_lockable_auto_unlock))) = qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
> >>            *var2 = (void *)(true); \
> >>            var2; \
> >>            var2 = NULL)
> >>
> >> (and we'll need to modify qemu_lockable_auto_unlock() to take "QemuLockable **x" argument)
> >>
> >
> > That's almost good enough. I fixed a few things to generate var2.
> >
> > I applied a similar approach to WITH_GRAPH_RDLOCK_GUARD macro:
> >
> > --- a/include/block/graph-lock.h
> > +++ b/include/block/graph-lock.h
> > @@ -224,13 +224,22 @@ graph_lockable_auto_unlock(GraphLockable *x)
> >
> >   G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)
> >
> > -#define WITH_GRAPH_RDLOCK_GUARD_(var)                                         \
> > -    for (g_autoptr(GraphLockable) var = graph_lockable_auto_lock(GML_OBJ_()); \
> > -         var;                                                                 \
> > -         graph_lockable_auto_unlock(var), var = NULL)
> > +static inline void TSA_NO_TSA coroutine_fn
> > +graph_lockable_auto_cleanup(GraphLockable **x)
> > +{
> > +    graph_lockable_auto_unlock(*x);
> > +}
> > +
> > +#define WITH_GRAPH_RDLOCK_GUARD__(var) \
> > +    GraphLockable *var \
> > +        __attribute__((cleanup(graph_lockable_auto_cleanup))) G_GNUC_UNUSED = \
> > +       graph_lockable_auto_lock(GML_OBJ_())
> > +
> > +#define WITH_GRAPH_RDLOCK_GUARD_(var, var2)                             \
> > +    for (WITH_GRAPH_RDLOCK_GUARD__(var), *var2 = (void *)true; var2;
> > var2 = NULL)
> >
> >   #define WITH_GRAPH_RDLOCK_GUARD() \
> > -    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
> > +    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__),
> > glue(graph_lockable_auto, __COUNTER__))
> >
> > Unfortunately, it doesn't work in all cases. It seems to have issues
> > with some guards:
> > ../block/stream.c: In function ‘stream_run’:
> > ../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
> > [-Werror=maybe-uninitialized]
> >    216 |         if (ret < 0) {
> >
> >
>
> So, updated macro helps in some cases, but doesn't help here? Intersting, why.
>
> > What should we do? change the macros + cherry-pick the missing
> > false-positives, or keep this series as is?
> >
> >
>
> I think marco + missing is better. No reason to add dead-initializations in cases where new macros helps.

Ok

> Still, would be good to understand, what's the difference, why it help on some cases and not help in another.

I don't know, it's like if the analyzer was lazy for this particular
case, although there is nothing much different from other usages.

If I replace:
for (... *var2 = (void *)true; var2;
with:
for (... *var2 = (void *)true; var2 || true;

then it doesn't warn..

Interestingly as well, if I change:
    for (... *var2 = (void *)true; var2; var2 = NULL)
for:
    for (... *var2 = GML_OBJ_(); var2; var2 = NULL)

GML_OBJ_() simply being &(GraphLockable) { }), an empty compound
literal, then it doesn't work, in all usages.

All in all, I am not sure the trick of using var2 is really reliable either.

-- 
Marc-André Lureau

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Eric Blake]

On Wed, Apr 03, 2024 at 01:24:11PM +0400, Marc-André Lureau wrote:
> > > Unfortunately, it doesn't work in all cases. It seems to have issues
> > > with some guards:
> > > ../block/stream.c: In function ‘stream_run’:
> > > ../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
> > > [-Werror=maybe-uninitialized]
> > >    216 |         if (ret < 0) {
> > >

That one looks like:

int ret;
WITH_GRAPH_RDLOCK_GUARD() {
  ret = ...;
}
if (copy) {
  ret = ...;
}
if (ret < 0)

I suspect the compiler is seeing the uncertainty possible from the
second conditional, and letting it take priority over the certainty
that the tweaked macro provided for the first conditional.

> > >
> >
> > So, updated macro helps in some cases, but doesn't help here? Intersting, why.
> >
> > > What should we do? change the macros + cherry-pick the missing
> > > false-positives, or keep this series as is?

An uglier macro, with sufficient comments as to why it is ugly (in
order to let us have fewer false positives where we have to add
initializers) may be less churn in the code base, but I'm not
necessarily sold on the ugly macro.  Let's see if anyone else
expresses an opinion.


> > >
> > >
> >
> > I think marco + missing is better. No reason to add dead-initializations in cases where new macros helps.
> 
> Ok
> 
> > Still, would be good to understand, what's the difference, why it help on some cases and not help in another.
> 
> I don't know, it's like if the analyzer was lazy for this particular
> case, although there is nothing much different from other usages.
> 
> If I replace:
> for (... *var2 = (void *)true; var2;
> with:
> for (... *var2 = (void *)true; var2 || true;
> 
> then it doesn't warn..

but it also doesn't work.  We want the body to execute exactly once,
not infloop.


> 
> Interestingly as well, if I change:
>     for (... *var2 = (void *)true; var2; var2 = NULL)
> for:
>     for (... *var2 = GML_OBJ_(); var2; var2 = NULL)
> 
> GML_OBJ_() simply being &(GraphLockable) { }), an empty compound
> literal, then it doesn't work, in all usages.

So the compiler is not figuring out that the compound literal is
sufficient for an unconditional one time through the for loop body.

What's worse, different compiler versions will change behavior over
time.  Making the code ugly to pacify a particular compiler, when that
compiler might improve in the future, is a form of chasing windmills.

> 
> All in all, I am not sure the trick of using var2 is really reliable either.

And that's my biggest argument for not making the macro not more
complex than it already is.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Vladimir Sementsov-Ogievskiy]

On 03.04.24 20:50, Eric Blake wrote:
> On Wed, Apr 03, 2024 at 01:24:11PM +0400, Marc-André Lureau wrote:
>>>> Unfortunately, it doesn't work in all cases. It seems to have issues
>>>> with some guards:
>>>> ../block/stream.c: In function ‘stream_run’:
>>>> ../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
>>>> [-Werror=maybe-uninitialized]
>>>>     216 |         if (ret < 0) {
>>>>
> 
> That one looks like:
> 
> int ret;
> WITH_GRAPH_RDLOCK_GUARD() {
>    ret = ...;
> }
> if (copy) {
>    ret = ...;
> }
> if (ret < 0)
> 
> I suspect the compiler is seeing the uncertainty possible from the
> second conditional, and letting it take priority over the certainty
> that the tweaked macro provided for the first conditional.
> 
>>>>
>>>
>>> So, updated macro helps in some cases, but doesn't help here? Intersting, why.
>>>
>>>> What should we do? change the macros + cherry-pick the missing
>>>> false-positives, or keep this series as is?
> 
> An uglier macro, with sufficient comments as to why it is ugly (in
> order to let us have fewer false positives where we have to add
> initializers) may be less churn in the code base, but I'm not
> necessarily sold on the ugly macro.  Let's see if anyone else
> expresses an opinion.
> 
> 
>>>>
>>>>
>>>
>>> I think marco + missing is better. No reason to add dead-initializations in cases where new macros helps.
>>
>> Ok
>>
>>> Still, would be good to understand, what's the difference, why it help on some cases and not help in another.
>>
>> I don't know, it's like if the analyzer was lazy for this particular
>> case, although there is nothing much different from other usages.
>>
>> If I replace:
>> for (... *var2 = (void *)true; var2;
>> with:
>> for (... *var2 = (void *)true; var2 || true;
>>
>> then it doesn't warn..
> 
> but it also doesn't work.  We want the body to execute exactly once,
> not infloop.
> 
> 
>>
>> Interestingly as well, if I change:
>>      for (... *var2 = (void *)true; var2; var2 = NULL)
>> for:
>>      for (... *var2 = GML_OBJ_(); var2; var2 = NULL)
>>
>> GML_OBJ_() simply being &(GraphLockable) { }), an empty compound
>> literal, then it doesn't work, in all usages.
> 
> So the compiler is not figuring out that the compound literal is
> sufficient for an unconditional one time through the for loop body.
> 
> What's worse, different compiler versions will change behavior over
> time.  Making the code ugly to pacify a particular compiler, when that
> compiler might improve in the future, is a form of chasing windmills.
> 
>>
>> All in all, I am not sure the trick of using var2 is really reliable either.
> 
> And that's my biggest argument for not making the macro not more
> complex than it already is.
> 

All sounds reasonable, I'm not sure now.

I still don't like an idea to satisfy compiler false-positive warnings by extra initializations.. Interesting that older versions do have unitialized-use warnings, but don't fail here (or nobody check?). Is it necessary to fix them at all? Older versions of compiler don't produce these warnings?  Is it possible that some optimizations in new GCC version somehow breaks our WITH_ hack, so that it really lead to uninitialized behavior? And we just mask real problem with these patches?

Wouldn't it more correct to just drop WITH_ hack, and move to a bit uglier but more gcc-native and fair

{
    QEMU_LOCK_GUARD(lock);
    ...
}

?

-- 
Best regards,
Vladimir

[PATCH 07/19] hw/ahci: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/ide/ahci.c:989:58: error: ‘tbl_entry_size’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/ide/ahci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bfefad2965..e89c92b7aa 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -948,7 +948,6 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
     uint64_t sum = 0;
     int off_idx = -1;
     int64_t off_pos = -1;
-    int tbl_entry_size;
     IDEBus *bus = &ad->port;
     BusState *qbus = BUS(bus);
 
@@ -976,6 +975,8 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
     /* Get entries in the PRDT, init a qemu sglist accordingly */
     if (prdtl > 0) {
         AHCI_SG *tbl = (AHCI_SG *)prdt;
+        int tbl_entry_size = -1;
+
         sum = 0;
         for (i = 0; i < prdtl; i++) {
             tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
-- 
2.44.0

[PATCH 08/19] hw/vhost-scsi: fix -Werror=maybe-uninitialized

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/scsi/vhost-scsi.c:173:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

It can be reached when num_queues=0. It probably doesn't make much sense
to instantiate a vhost-scsi with 0 IO queues though. For now, make
vhost_scsi_set_workers() return success/0 anyway, when no workers have
been setup.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/scsi/vhost-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index ae26bc19a4..5b066df4f7 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -170,7 +170,7 @@ static int vhost_scsi_set_workers(VHostSCSICommon *vsc, bool per_virtqueue)
     struct vhost_dev *dev = &vsc->dev;
     struct vhost_vring_worker vq_worker;
     struct vhost_worker_state worker;
-    int i, ret;
+    int i, ret = 0;
 
     /* Use default worker */
     if (!per_virtqueue || dev->nvqs == VHOST_SCSI_VQ_NUM_FIXED + 1) {
-- 
2.44.0

[PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/sd/sdhci.c:846:16: error: ‘res’ may be used uninitialized [-Werror=maybe-uninitialized]

False-positive, because "length" is non-null.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/sd/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index c5e0bc018b..da5351d4e5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -745,7 +745,7 @@ static void sdhci_do_adma(SDHCIState *s)
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     const MemTxAttrs attrs = { .memory = true };
     ADMADescr dscr = {};
-    MemTxResult res;
+    MemTxResult res = MEMTX_ERROR;
     int i;
 
     if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
-- 
2.44.0

Re: [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive

[Philippe Mathieu-Daudé]

On 28/3/24 11:20, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../hw/sd/sdhci.c:846:16: error: ‘res’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> False-positive, because "length" is non-null.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/sd/sdhci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index c5e0bc018b..da5351d4e5 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -745,7 +745,7 @@ static void sdhci_do_adma(SDHCIState *s)
>       const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
>       const MemTxAttrs attrs = { .memory = true };
>       ADMADescr dscr = {};
> -    MemTxResult res;
> +    MemTxResult res = MEMTX_ERROR;

Something is indeed odd here. Due to the for() loop, the variable
initialization should be moved after the SDHC_ADMA_ATTR_ACT_TRAN
switch case.

Re: [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive

[Marc-André Lureau]

Hi

On Thu, Mar 28, 2024 at 3:31 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 28/3/24 11:20, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > ../hw/sd/sdhci.c:846:16: error: ‘res’ may be used uninitialized [-Werror=maybe-uninitialized]
> >
> > False-positive, because "length" is non-null.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   hw/sd/sdhci.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index c5e0bc018b..da5351d4e5 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -745,7 +745,7 @@ static void sdhci_do_adma(SDHCIState *s)
> >       const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
> >       const MemTxAttrs attrs = { .memory = true };
> >       ADMADescr dscr = {};
> > -    MemTxResult res;
> > +    MemTxResult res = MEMTX_ERROR;
>
> Something is indeed odd here. Due to the for() loop, the variable
> initialization should be moved after the SDHC_ADMA_ATTR_ACT_TRAN
> switch case.
>

Moving the variable initialization? What do you suggest exactly?

thanks

-- 
Marc-André Lureau

[PATCH 10/19] hw/rdma: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/rdma/rdma_backend.c:129:8: error: ‘ne’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/rdma/rdma_backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index 6dcdfbbbe2..1ca485dd0d 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -90,7 +90,7 @@ static void clean_recv_mads(RdmaBackendDev *backend_dev)
 
 static int rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, struct ibv_cq *ibcq)
 {
-    int i, ne, total_ne = 0;
+    int i, ne = -1, total_ne = 0;
     BackendCtx *bctx;
     struct ibv_wc wc[2];
     RdmaProtectedGSList *cqe_ctx_list;
-- 
2.44.0

[PATCH 11/19] migration/block: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../migration/block.c:966:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

Given that "cluster_size" must be <= BLK_MIG_BLOCK_SIZE, the previous
loop is entered at least once, so 'ret' is assigned a value in all conditions.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/block.c b/migration/block.c
index 2b9054889a..486b1bec03 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -877,7 +877,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
     uint8_t *buf;
     int64_t total_sectors = 0;
     int nr_sectors;
-    int ret;
+    int ret = -EINVAL;
     BlockDriverInfo bdi;
     int cluster_size = BLK_MIG_BLOCK_SIZE;
 
-- 
2.44.0

Re: [PATCH 11/19] migration/block: fix -Werror=maybe-uninitialized false-positive

[Peter Xu]

On Thu, Mar 28, 2024 at 02:20:44PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../migration/block.c:966:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Given that "cluster_size" must be <= BLK_MIG_BLOCK_SIZE, the previous
> loop is entered at least once, so 'ret' is assigned a value in all conditions.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../migration/dirtyrate.c:186:5: error: ‘records’ may be used uninitialized [-Werror=maybe-uninitialized]
../migration/dirtyrate.c:168:12: error: ‘gen_id’ may be used uninitialized [-Werror=maybe-uninitialized]
../migration/migration.c:2273:5: error: ‘file’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/dirtyrate.c | 4 ++--
 migration/migration.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 1d2e85746f..22dd22922c 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -144,12 +144,12 @@ int64_t vcpu_calculate_dirtyrate(int64_t calc_time_ms,
                                  unsigned int flag,
                                  bool one_shot)
 {
-    DirtyPageRecord *records;
+    DirtyPageRecord *records = NULL;
     int64_t init_time_ms;
     int64_t duration;
     int64_t dirtyrate;
     int i = 0;
-    unsigned int gen_id;
+    unsigned int gen_id = 0;
 
 retry:
     init_time_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/migration/migration.c b/migration/migration.c
index 9fe8fd2afd..412138ea94 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2247,7 +2247,7 @@ static bool migrate_handle_rp_resume_ack(MigrationState *s,
  */
 static void migration_release_dst_files(MigrationState *ms)
 {
-    QEMUFile *file;
+    QEMUFile *file = NULL;
 
     WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
         /*
-- 
2.44.0

Re: [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives

[Peter Xu]

On Thu, Mar 28, 2024 at 02:20:45PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../migration/dirtyrate.c:186:5: error: ‘records’ may be used uninitialized [-Werror=maybe-uninitialized]
> ../migration/dirtyrate.c:168:12: error: ‘gen_id’ may be used uninitialized [-Werror=maybe-uninitialized]
> ../migration/migration.c:2273:5: error: ‘file’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives

[Yong Huang]

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

On Thu, Mar 28, 2024 at 6:23 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ../migration/dirtyrate.c:186:5: error: ‘records’ may be used uninitialized
> [-Werror=maybe-uninitialized]
> ../migration/dirtyrate.c:168:12: error: ‘gen_id’ may be used uninitialized
> [-Werror=maybe-uninitialized]
> ../migration/migration.c:2273:5: error: ‘file’ may be used uninitialized
> [-Werror=maybe-uninitialized]
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  migration/dirtyrate.c | 4 ++--
>  migration/migration.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 1d2e85746f..22dd22922c 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -144,12 +144,12 @@ int64_t vcpu_calculate_dirtyrate(int64_t
> calc_time_ms,
>                                   unsigned int flag,
>                                   bool one_shot)
>  {
> -    DirtyPageRecord *records;
> +    DirtyPageRecord *records = NULL;
>      int64_t init_time_ms;
>      int64_t duration;
>      int64_t dirtyrate;
>      int i = 0;
> -    unsigned int gen_id;
> +    unsigned int gen_id = 0;
>
>  retry:
>      init_time_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> diff --git a/migration/migration.c b/migration/migration.c
> index 9fe8fd2afd..412138ea94 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2247,7 +2247,7 @@ static bool
> migrate_handle_rp_resume_ack(MigrationState *s,
>   */
>  static void migration_release_dst_files(MigrationState *ms)
>  {
> -    QEMUFile *file;
> +    QEMUFile *file = NULL;
>
>      WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>          /*
> --
> 2.44.0
>
>
Reviewed-by: Hyman Huang <yong.huang@smartx.com>

Yong

-- 
Best regards

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

[PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/block/virtio-blk.c:1212:12: error: ‘rq’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 92de315f17..ce660ff9fe 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1195,7 +1195,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
     VirtIOBlock *s = opaque;
     uint16_t num_queues = s->conf.num_queues;
     g_autofree VirtIOBlockReq **vq_rq = NULL;
-    VirtIOBlockReq *rq;
+    VirtIOBlockReq *rq = NULL;
 
     if (!running) {
         return;
-- 
2.44.0

Re: [PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive

[Stefan Hajnoczi]

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

On Thu, Mar 28, 2024 at 02:20:46PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../hw/block/virtio-blk.c:1212:12: error: ‘rq’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/block/virtio-blk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[PATCH 14/19] plugins: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../plugins/loader.c:405:15: error: ‘ctx’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 plugins/loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/loader.c b/plugins/loader.c
index 9768b78eb6..513a429c57 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -390,7 +390,7 @@ void plugin_reset_uninstall(qemu_plugin_id_t id,
                             bool reset)
 {
     struct qemu_plugin_reset_data *data;
-    struct qemu_plugin_ctx *ctx;
+    struct qemu_plugin_ctx *ctx = NULL;
 
     WITH_QEMU_LOCK_GUARD(&plugin.lock) {
         ctx = plugin_id_to_ctx_locked(id);
-- 
2.44.0

Re: [PATCH 14/19] plugins: fix -Werror=maybe-uninitialized false-positive

[Pierrick Bouvier]

On 3/28/24 14:20, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../plugins/loader.c:405:15: error: ‘ctx’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   plugins/loader.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/plugins/loader.c b/plugins/loader.c
> index 9768b78eb6..513a429c57 100644
> --- a/plugins/loader.c
> +++ b/plugins/loader.c
> @@ -390,7 +390,7 @@ void plugin_reset_uninstall(qemu_plugin_id_t id,
>                               bool reset)
>   {
>       struct qemu_plugin_reset_data *data;
> -    struct qemu_plugin_ctx *ctx;
> +    struct qemu_plugin_ctx *ctx = NULL;
>   
>       WITH_QEMU_LOCK_GUARD(&plugin.lock) {
>           ctx = plugin_id_to_ctx_locked(id);

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH 15/19] migration: fix -Werror=maybe-uninitialized false-positive

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../migration/ram.c:1873:23: error: ‘dirty’ may be used uninitialized [-Werror=maybe-uninitialized]

When 'block' != NULL, 'dirty' is initialized.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 8deb84984f..4e26bced31 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1847,7 +1847,7 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
 {
     RAMBlock  *block;
     ram_addr_t offset;
-    bool dirty;
+    bool dirty = false;
 
     do {
         block = unqueue_page(rs, &offset);
-- 
2.44.0

Re: [PATCH 15/19] migration: fix -Werror=maybe-uninitialized false-positive

[Peter Xu]

On Thu, Mar 28, 2024 at 02:20:48PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../migration/ram.c:1873:23: error: ‘dirty’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> When 'block' != NULL, 'dirty' is initialized.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 16/19] tests: fix -Werror=maybe-uninitialized

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../tests/unit/test-block-iothread.c:773:17: error: ‘job’ may be used uninitialized [-Werror=maybe-uninitialized]
/usr/include/glib-2.0/glib/gtestutils.h:73:53: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/test-bdrv-drain.c     | 2 +-
 tests/unit/test-block-iothread.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 666880472b..c112d5b189 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -722,7 +722,7 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type,
     BlockJob *job;
     TestBlockJob *tjob;
     IOThread *iothread = NULL;
-    int ret;
+    int ret = -1;
 
     src = bdrv_new_open_driver(&bdrv_test, "source", BDRV_O_RDWR,
                                &error_abort);
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 3766d5de6b..20ed54f570 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -745,7 +745,7 @@ static void test_propagate_mirror(void)
     AioContext *main_ctx = qemu_get_aio_context();
     BlockDriverState *src, *target, *filter;
     BlockBackend *blk;
-    Job *job;
+    Job *job = NULL;
     Error *local_err = NULL;
 
     /* Create src and target*/
-- 
2.44.0

[PATCH 17/19] hw/nvme: fix -Werror=maybe-uninitialized

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/nvme/ctrl.c:6081:21: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]

It's not obvious that 'result' is set in all code paths. When &result is
a returned argument, it's even less clear.

Looking at various assignments, 0 seems to be a suitable default value.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index c2b17de987..127c3d2383 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5894,7 +5894,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
     uint32_t dw10 = le32_to_cpu(cmd->cdw10);
     uint32_t dw11 = le32_to_cpu(cmd->cdw11);
     uint32_t nsid = le32_to_cpu(cmd->nsid);
-    uint32_t result;
+    uint32_t result = 0;
     uint8_t fid = NVME_GETSETFEAT_FID(dw10);
     NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
     uint16_t iv;
-- 
2.44.0

Re: [PATCH 17/19] hw/nvme: fix -Werror=maybe-uninitialized

[Klaus Jensen]

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

On Mar 28 14:20, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../hw/nvme/ctrl.c:6081:21: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> It's not obvious that 'result' is set in all code paths. When &result is
> a returned argument, it's even less clear.
> 
> Looking at various assignments, 0 seems to be a suitable default value.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/nvme/ctrl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index c2b17de987..127c3d2383 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -5894,7 +5894,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>      uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>      uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>      uint32_t nsid = le32_to_cpu(cmd->nsid);
> -    uint32_t result;
> +    uint32_t result = 0;
>      uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>      NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
>      uint16_t iv;
> -- 
> 2.44.0
> 

Thanks!

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

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

[PATCH 18/19] hw/virtio: fix -Werror=maybe-uninitialized

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used uninitialized [-Werror=maybe-uninitialized]

'&r' is not guaranteed to be assigned when calling -Werror=maybe-uninitialized.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index fc5f408f77..cd29cc795b 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
 size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
 {
     size_t len = 0;
-    uint32_t r;
+    uint32_t r = 0;
 
     while (num--) {
         int64_t start_us = g_get_monotonic_time();
-- 
2.44.0

[PATCH 19/19] RFC: hw/virtio: a potential leak fix

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost_svq_get_buf() may return a VirtQueueElement that should be freed.

It's unclear to me if the vhost_svq_get_buf() call should always return NULL.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index cd29cc795b..93742d9ddc 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
     return i;
 }
 
+G_GNUC_WARN_UNUSED_RESULT
 static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
                                            uint32_t *len)
 {
@@ -529,6 +530,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
     uint32_t r = 0;
 
     while (num--) {
+        g_autofree VirtQueueElement *elem = NULL;
         int64_t start_us = g_get_monotonic_time();
 
         do {
@@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
             }
         } while (true);
 
-        vhost_svq_get_buf(svq, &r);
+        elem = vhost_svq_get_buf(svq, &r);
         len += r;
     }
 
-- 
2.44.0

Re: [PATCH 00/19] -Werror=maybe-uninitialized fixes

[Eric Blake]

On Thu, Mar 28, 2024 at 02:20:33PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Depending on -Doptimization=<value>, GCC (13.2.1 here) produces different
> maybe-uninitialized warnings:
> - g: produces -Werror=maybe-uninitialized errors
> - 0: clean build
> - 1: produces -Werror=maybe-uninitialized errors
> - 2: clean build
> - 3: produces few -Werror=maybe-uninitialized errors
> - s: produces -Werror=maybe-uninitialized errors
> 
> Most are false-positive, because prior LOCK_GUARD should guarantee an
> initialization path. Few of them are a bit trickier. Finally, I found
> a potential related memory leak.
> 
> thanks

Couple of subject lines are inconsistent; I suggest:

> 
> Marc-André Lureau (19):
>   util/coroutine: fix -Werror=maybe-uninitialized false-positive
>   util/timer: with -Werror=maybe-uninitialized false-positive

s/with/fix/

>   hw/qxl: fix -Werror=maybe-uninitialized false-positives
>   nbd: with -Werror=maybe-uninitialized false-positive

s/with/fix/

>   block/mirror: fix -Werror=maybe-uninitialized false-positive
>   block/stream: fix -Werror=maybe-uninitialized false-positives
>   hw/ahci: fix -Werror=maybe-uninitialized false-positive
>   hw/vhost-scsi: fix -Werror=maybe-uninitialized
>   hw/sdhci: fix -Werror=maybe-uninitialized false-positive
>   hw/rdma: fix -Werror=maybe-uninitialized false-positive
>   migration/block: fix -Werror=maybe-uninitialized false-positive
>   migration: fix -Werror=maybe-uninitialized false-positives
>   hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive
>   plugins: fix -Werror=maybe-uninitialized false-positive
>   migration: fix -Werror=maybe-uninitialized false-positive
>   tests: fix -Werror=maybe-uninitialized
>   hw/nvme: fix -Werror=maybe-uninitialized
>   hw/virtio: fix -Werror=maybe-uninitialized
>   RFC: hw/virtio: a potential leak fix
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org