* [PATCH 00/19] -Werror=maybe-uninitialized fixes
@ 2024-03-28 10:20 marcandre.lureau
2024-03-28 10:20 ` [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
` (19 more replies)
0 siblings, 20 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-04-02 20:17 ` Stefan Hajnoczi
2024-03-28 10:20 ` [PATCH 02/19] util/timer: with " marcandre.lureau
` (18 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 02/19] util/timer: with -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
2024-03-28 10:20 ` [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:20 ` [PATCH 03/19] hw/qxl: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
` (17 subsequent siblings)
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 03/19] hw/qxl: fix -Werror=maybe-uninitialized false-positives
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
2024-03-28 10:20 ` [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
2024-03-28 10:20 ` [PATCH 02/19] util/timer: with " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:20 ` [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive marcandre.lureau
` (16 subsequent siblings)
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (2 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 03/19] hw/qxl: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 14:30 ` Eric Blake
2024-03-28 10:20 ` [PATCH 05/19] block/mirror: fix " marcandre.lureau
` (15 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 05/19] block/mirror: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (3 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-29 8:22 ` Vladimir Sementsov-Ogievskiy
2024-03-28 10:20 ` [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
` (14 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (4 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 05/19] block/mirror: fix " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-29 8:34 ` Vladimir Sementsov-Ogievskiy
2024-03-28 10:20 ` [PATCH 07/19] hw/ahci: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
` (13 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 07/19] hw/ahci: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (5 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:20 ` [PATCH 08/19] hw/vhost-scsi: fix -Werror=maybe-uninitialized marcandre.lureau
` (12 subsequent siblings)
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 08/19] hw/vhost-scsi: fix -Werror=maybe-uninitialized
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (6 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 07/19] hw/ahci: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:20 ` [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
` (11 subsequent siblings)
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (7 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 08/19] hw/vhost-scsi: fix -Werror=maybe-uninitialized marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 11:30 ` Philippe Mathieu-Daudé
2024-03-28 10:20 ` [PATCH 10/19] hw/rdma: " marcandre.lureau
` (10 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 10/19] hw/rdma: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (8 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:20 ` [PATCH 11/19] migration/block: " marcandre.lureau
` (9 subsequent siblings)
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 11/19] migration/block: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (9 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 10/19] hw/rdma: " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 19:40 ` Peter Xu
2024-03-28 10:20 ` [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
` (8 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (10 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 11/19] migration/block: " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 19:40 ` Peter Xu
2024-03-29 1:14 ` Yong Huang
2024-03-28 10:20 ` [PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
` (7 subsequent siblings)
19 siblings, 2 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (11 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-04-02 20:18 ` Stefan Hajnoczi
2024-03-28 10:20 ` [PATCH 14/19] plugins: " marcandre.lureau
` (6 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 14/19] plugins: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (12 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:35 ` Pierrick Bouvier
2024-03-28 10:20 ` [PATCH 15/19] migration: " marcandre.lureau
` (5 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 15/19] migration: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (13 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 14/19] plugins: " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 19:40 ` Peter Xu
2024-03-28 10:20 ` [PATCH 16/19] tests: fix -Werror=maybe-uninitialized marcandre.lureau
` (4 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 16/19] tests: fix -Werror=maybe-uninitialized
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (14 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 15/19] migration: " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:20 ` [PATCH 17/19] hw/nvme: " marcandre.lureau
` (3 subsequent siblings)
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 17/19] hw/nvme: fix -Werror=maybe-uninitialized
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (15 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 16/19] tests: fix -Werror=maybe-uninitialized marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-04-02 10:40 ` Klaus Jensen
2024-03-28 10:20 ` [PATCH 18/19] hw/virtio: " marcandre.lureau
` (2 subsequent siblings)
19 siblings, 1 reply; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 18/19] hw/virtio: fix -Werror=maybe-uninitialized
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (16 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 17/19] hw/nvme: " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 10:20 ` [PATCH 19/19] RFC: hw/virtio: a potential leak fix marcandre.lureau
2024-03-28 14:31 ` [PATCH 00/19] -Werror=maybe-uninitialized fixes Eric Blake
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 19/19] RFC: hw/virtio: a potential leak fix
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (17 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 18/19] hw/virtio: " marcandre.lureau
@ 2024-03-28 10:20 ` marcandre.lureau
2024-03-28 14:31 ` [PATCH 00/19] -Werror=maybe-uninitialized fixes Eric Blake
19 siblings, 0 replies; 43+ messages in thread
From: marcandre.lureau @ 2024-03-28 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu,
Marc-André 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 14/19] plugins: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 14/19] plugins: " marcandre.lureau
@ 2024-03-28 10:35 ` Pierrick Bouvier
0 siblings, 0 replies; 43+ messages in thread
From: Pierrick Bouvier @ 2024-03-28 10:35 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Keith Busch,
Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-03-28 11:30 ` Philippe Mathieu-Daudé
2024-04-02 9:21 ` Marc-André Lureau
0 siblings, 1 reply; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-03-28 11:30 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Hyman Huang, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Yuval Shaia,
Alex Bennée, Jesper Devantier, Pierrick Bouvier, Keith Busch,
Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-03-28 14:30 ` Eric Blake
0 siblings, 0 replies; 43+ messages in thread
From: Eric Blake @ 2024-03-28 14:30 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 00/19] -Werror=maybe-uninitialized fixes
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
` (18 preceding siblings ...)
2024-03-28 10:20 ` [PATCH 19/19] RFC: hw/virtio: a potential leak fix marcandre.lureau
@ 2024-03-28 14:31 ` Eric Blake
19 siblings, 0 replies; 43+ messages in thread
From: Eric Blake @ 2024-03-28 14:31 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 11/19] migration/block: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 11/19] migration/block: " marcandre.lureau
@ 2024-03-28 19:40 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2024-03-28 19:40 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives
2024-03-28 10:20 ` [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
@ 2024-03-28 19:40 ` Peter Xu
2024-03-29 1:14 ` Yong Huang
1 sibling, 0 replies; 43+ messages in thread
From: Peter Xu @ 2024-03-28 19:40 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 15/19] migration: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 15/19] migration: " marcandre.lureau
@ 2024-03-28 19:40 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2024-03-28 19:40 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives
2024-03-28 10:20 ` [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
2024-03-28 19:40 ` Peter Xu
@ 2024-03-29 1:14 ` Yong Huang
1 sibling, 0 replies; 43+ messages in thread
From: Yong Huang @ 2024-03-29 1:14 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Vladimir Sementsov-Ogievskiy, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu
[-- 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 --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 05/19] block/mirror: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 05/19] block/mirror: fix " marcandre.lureau
@ 2024-03-29 8:22 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 43+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-03-29 8:22 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Hyman Huang, Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-03-28 10:20 ` [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
@ 2024-03-29 8:34 ` Vladimir Sementsov-Ogievskiy
2024-04-02 9:12 ` Marc-André Lureau
0 siblings, 1 reply; 43+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-03-29 8:34 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Hyman Huang, Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-03-29 8:34 ` Vladimir Sementsov-Ogievskiy
@ 2024-04-02 9:12 ` Marc-André Lureau
2024-04-02 9:58 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 43+ messages in thread
From: Marc-André Lureau @ 2024-04-02 9:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-devel, Hyman Huang, Paolo Bonzini, Gerd Hoffmann, qemu-block,
Kevin Wolf, Fabiano Rosas, Mahmoud Mandour, John Snow,
Klaus Jensen, Fam Zheng, Eugenio Pérez, Bin Meng,
Hanna Reitz, Eric Blake, Michael S. Tsirkin, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Yuval Shaia, Alex Bennée,
Jesper Devantier, Pierrick Bouvier, Keith Busch, Marcel Apfelbaum,
Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive
2024-03-28 11:30 ` Philippe Mathieu-Daudé
@ 2024-04-02 9:21 ` Marc-André Lureau
0 siblings, 0 replies; 43+ messages in thread
From: Marc-André Lureau @ 2024-04-02 9:21 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Yuval Shaia,
Alex Bennée, Jesper Devantier, Pierrick Bouvier, Keith Busch,
Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-02 9:12 ` Marc-André Lureau
@ 2024-04-02 9:58 ` Vladimir Sementsov-Ogievskiy
2024-04-02 15:34 ` Eric Blake
0 siblings, 1 reply; 43+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-02 9:58 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Hyman Huang, Paolo Bonzini, Gerd Hoffmann, qemu-block,
Kevin Wolf, Fabiano Rosas, Mahmoud Mandour, John Snow,
Klaus Jensen, Fam Zheng, Eugenio Pérez, Bin Meng,
Hanna Reitz, Eric Blake, Michael S. Tsirkin, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Yuval Shaia, Alex Bennée,
Jesper Devantier, Pierrick Bouvier, Keith Busch, Marcel Apfelbaum,
Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 17/19] hw/nvme: fix -Werror=maybe-uninitialized
2024-03-28 10:20 ` [PATCH 17/19] hw/nvme: " marcandre.lureau
@ 2024-04-02 10:40 ` Klaus Jensen
0 siblings, 0 replies; 43+ messages in thread
From: Klaus Jensen @ 2024-04-02 10:40 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu
[-- 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 --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-02 9:58 ` Vladimir Sementsov-Ogievskiy
@ 2024-04-02 15:34 ` Eric Blake
2024-04-02 19:24 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 43+ messages in thread
From: Eric Blake @ 2024-04-02 15:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Marc-André Lureau, qemu-devel, Hyman Huang, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Michael S. Tsirkin,
Stefan Hajnoczi, Philippe Mathieu-Daudé, Yuval Shaia,
Alex Bennée, Jesper Devantier, Pierrick Bouvier, Keith Busch,
Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-02 15:34 ` Eric Blake
@ 2024-04-02 19:24 ` Vladimir Sementsov-Ogievskiy
2024-04-03 8:11 ` Marc-André Lureau
0 siblings, 1 reply; 43+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-02 19:24 UTC (permalink / raw)
To: Eric Blake
Cc: Marc-André Lureau, qemu-devel, Hyman Huang, Paolo Bonzini,
Gerd Hoffmann, qemu-block, Kevin Wolf, Fabiano Rosas,
Mahmoud Mandour, John Snow, Klaus Jensen, Fam Zheng,
Eugenio Pérez, Bin Meng, Hanna Reitz, Michael S. Tsirkin,
Stefan Hajnoczi, Philippe Mathieu-Daudé, Yuval Shaia,
Alex Bennée, Jesper Devantier, Pierrick Bouvier, Keith Busch,
Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-04-02 20:17 ` Stefan Hajnoczi
0 siblings, 0 replies; 43+ messages in thread
From: Stefan Hajnoczi @ 2024-04-02 20:17 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Philippe Mathieu-Daudé, Yuval Shaia,
Alex Bennée, Jesper Devantier, Pierrick Bouvier, Keith Busch,
Marcel Apfelbaum, Alexandre Iooss, Peter Xu
[-- 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 --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive
2024-03-28 10:20 ` [PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
@ 2024-04-02 20:18 ` Stefan Hajnoczi
0 siblings, 0 replies; 43+ messages in thread
From: Stefan Hajnoczi @ 2024-04-02 20:18 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Hyman Huang, Vladimir Sementsov-Ogievskiy,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz, Eric Blake,
Michael S. Tsirkin, Philippe Mathieu-Daudé, Yuval Shaia,
Alex Bennée, Jesper Devantier, Pierrick Bouvier, Keith Busch,
Marcel Apfelbaum, Alexandre Iooss, Peter Xu
[-- 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 --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-02 19:24 ` Vladimir Sementsov-Ogievskiy
@ 2024-04-03 8:11 ` Marc-André Lureau
2024-04-03 8:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 43+ messages in thread
From: Marc-André Lureau @ 2024-04-03 8:11 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Eric Blake, qemu-devel, Hyman Huang, Paolo Bonzini, Gerd Hoffmann,
qemu-block, Kevin Wolf, Fabiano Rosas, Mahmoud Mandour, John Snow,
Klaus Jensen, Fam Zheng, Eugenio Pérez, Bin Meng,
Hanna Reitz, Michael S. Tsirkin, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Yuval Shaia, Alex Bennée,
Jesper Devantier, Pierrick Bouvier, Keith Busch, Marcel Apfelbaum,
Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-03 8:11 ` Marc-André Lureau
@ 2024-04-03 8:31 ` Vladimir Sementsov-Ogievskiy
2024-04-03 9:24 ` Marc-André Lureau
0 siblings, 1 reply; 43+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-03 8:31 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Eric Blake, qemu-devel, Hyman Huang, Paolo Bonzini, Gerd Hoffmann,
qemu-block, Kevin Wolf, Fabiano Rosas, Mahmoud Mandour, John Snow,
Klaus Jensen, Fam Zheng, Eugenio Pérez, Bin Meng,
Hanna Reitz, Michael S. Tsirkin, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Yuval Shaia, Alex Bennée,
Jesper Devantier, Pierrick Bouvier, Keith Busch, Marcel Apfelbaum,
Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-03 8:31 ` Vladimir Sementsov-Ogievskiy
@ 2024-04-03 9:24 ` Marc-André Lureau
2024-04-03 17:50 ` Eric Blake
0 siblings, 1 reply; 43+ messages in thread
From: Marc-André Lureau @ 2024-04-03 9:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Eric Blake, qemu-devel, Hyman Huang, Paolo Bonzini, Gerd Hoffmann,
qemu-block, Kevin Wolf, Fabiano Rosas, Mahmoud Mandour, John Snow,
Klaus Jensen, Fam Zheng, Eugenio Pérez, Bin Meng,
Hanna Reitz, Michael S. Tsirkin, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Yuval Shaia, Alex Bennée,
Jesper Devantier, Pierrick Bouvier, Keith Busch, Marcel Apfelbaum,
Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-03 9:24 ` Marc-André Lureau
@ 2024-04-03 17:50 ` Eric Blake
2024-04-03 21:28 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 43+ messages in thread
From: Eric Blake @ 2024-04-03 17:50 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, Hyman Huang,
Paolo Bonzini, Gerd Hoffmann, qemu-block, Kevin Wolf,
Fabiano Rosas, Mahmoud Mandour, John Snow, Klaus Jensen,
Fam Zheng, Eugenio Pérez, Bin Meng, Hanna Reitz,
Michael S. Tsirkin, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Yuval Shaia, Alex Bennée, Jesper Devantier, Pierrick Bouvier,
Keith Busch, Marcel Apfelbaum, Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
2024-04-03 17:50 ` Eric Blake
@ 2024-04-03 21:28 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 43+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-03 21:28 UTC (permalink / raw)
To: Eric Blake, Marc-André Lureau
Cc: qemu-devel, Hyman Huang, Paolo Bonzini, Gerd Hoffmann, qemu-block,
Kevin Wolf, Fabiano Rosas, Mahmoud Mandour, John Snow,
Klaus Jensen, Fam Zheng, Eugenio Pérez, Bin Meng,
Hanna Reitz, Michael S. Tsirkin, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Yuval Shaia, Alex Bennée,
Jesper Devantier, Pierrick Bouvier, Keith Busch, Marcel Apfelbaum,
Alexandre Iooss, Peter Xu
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
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2024-04-03 21:29 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-28 10:20 [PATCH 00/19] -Werror=maybe-uninitialized fixes marcandre.lureau
2024-03-28 10:20 ` [PATCH 01/19] util/coroutine: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
2024-04-02 20:17 ` Stefan Hajnoczi
2024-03-28 10:20 ` [PATCH 02/19] util/timer: with " marcandre.lureau
2024-03-28 10:20 ` [PATCH 03/19] hw/qxl: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
2024-03-28 10:20 ` [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive marcandre.lureau
2024-03-28 14:30 ` Eric Blake
2024-03-28 10:20 ` [PATCH 05/19] block/mirror: fix " marcandre.lureau
2024-03-29 8:22 ` Vladimir Sementsov-Ogievskiy
2024-03-28 10:20 ` [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
2024-03-29 8:34 ` Vladimir Sementsov-Ogievskiy
2024-04-02 9:12 ` Marc-André Lureau
2024-04-02 9:58 ` Vladimir Sementsov-Ogievskiy
2024-04-02 15:34 ` Eric Blake
2024-04-02 19:24 ` Vladimir Sementsov-Ogievskiy
2024-04-03 8:11 ` Marc-André Lureau
2024-04-03 8:31 ` Vladimir Sementsov-Ogievskiy
2024-04-03 9:24 ` Marc-André Lureau
2024-04-03 17:50 ` Eric Blake
2024-04-03 21:28 ` Vladimir Sementsov-Ogievskiy
2024-03-28 10:20 ` [PATCH 07/19] hw/ahci: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
2024-03-28 10:20 ` [PATCH 08/19] hw/vhost-scsi: fix -Werror=maybe-uninitialized marcandre.lureau
2024-03-28 10:20 ` [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
2024-03-28 11:30 ` Philippe Mathieu-Daudé
2024-04-02 9:21 ` Marc-André Lureau
2024-03-28 10:20 ` [PATCH 10/19] hw/rdma: " marcandre.lureau
2024-03-28 10:20 ` [PATCH 11/19] migration/block: " marcandre.lureau
2024-03-28 19:40 ` Peter Xu
2024-03-28 10:20 ` [PATCH 12/19] migration: fix -Werror=maybe-uninitialized false-positives marcandre.lureau
2024-03-28 19:40 ` Peter Xu
2024-03-29 1:14 ` Yong Huang
2024-03-28 10:20 ` [PATCH 13/19] hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive marcandre.lureau
2024-04-02 20:18 ` Stefan Hajnoczi
2024-03-28 10:20 ` [PATCH 14/19] plugins: " marcandre.lureau
2024-03-28 10:35 ` Pierrick Bouvier
2024-03-28 10:20 ` [PATCH 15/19] migration: " marcandre.lureau
2024-03-28 19:40 ` Peter Xu
2024-03-28 10:20 ` [PATCH 16/19] tests: fix -Werror=maybe-uninitialized marcandre.lureau
2024-03-28 10:20 ` [PATCH 17/19] hw/nvme: " marcandre.lureau
2024-04-02 10:40 ` Klaus Jensen
2024-03-28 10:20 ` [PATCH 18/19] hw/virtio: " marcandre.lureau
2024-03-28 10:20 ` [PATCH 19/19] RFC: hw/virtio: a potential leak fix marcandre.lureau
2024-03-28 14:31 ` [PATCH 00/19] -Werror=maybe-uninitialized fixes Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.