| Message ID | 20210712215535.1471256-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for clang-13 plus tcg/ppc | expand |
13.07.2021 00:55, Richard Henderson wrote: > From clang-13: > nbd/server.c:976:22: error: variable 'bitmaps' set but not used \ > [-Werror,-Wunused-but-set-variable] > > Cc: qemu-block@nongnu.org > Cc: Eric Blake <eblake@redhat.com> > Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > nbd/server.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index b60ebc3ab6..721349ec00 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > { > int ret; > g_autofree char *export_name = NULL; > - g_autofree bool *bitmaps = NULL; > NBDExportMetaContexts local_meta = {0}; > uint32_t nb_queries; > size_t i; > @@ -1007,9 +1006,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > "export '%s' not present", sane_name); > } > meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps); > - if (client->opt == NBD_OPT_LIST_META_CONTEXT) { > - bitmaps = meta->bitmaps; > - } > > ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), false, errp); > if (ret <= 0) { > Hm. I'm afraid, this way meta->bitmaps will be leaked in NBD_OPT_LIST_META_CONTEXT case. Actually, "bitmaps" _is_ used, in cleanup handler, setup by g_autofree. So it's a false positive. -- Best regards, Vladimir
On Tue, Jul 13, 2021 at 12:27:48PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 13.07.2021 00:55, Richard Henderson wrote: > > From clang-13: > > nbd/server.c:976:22: error: variable 'bitmaps' set but not used \ > > [-Werror,-Wunused-but-set-variable] > > > > +++ b/nbd/server.c > > @@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > > { > > int ret; > > g_autofree char *export_name = NULL; > > - g_autofree bool *bitmaps = NULL; > > NBDExportMetaContexts local_meta = {0}; > > uint32_t nb_queries; > > size_t i; > > @@ -1007,9 +1006,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > > "export '%s' not present", sane_name); > > } > > meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps); > > - if (client->opt == NBD_OPT_LIST_META_CONTEXT) { > > - bitmaps = meta->bitmaps; > > - } > > ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), false, errp); > > if (ret <= 0) { > > > > > Hm. I'm afraid, this way meta->bitmaps will be leaked in NBD_OPT_LIST_META_CONTEXT case. > > Actually, "bitmaps" _is_ used, in cleanup handler, setup by g_autofree. So it's a false positive. > Correct; this patch is wrong, and would cause a memory leak. This is a false positive in clang, and a known issue that clang is in general unable to see that g_autofree variables are used, sometimes for their intentional side effects such as easier memory cleanup as done here. I suspect that the definition of g_autofree already uses __attribute__((unused)) to work around clang's oddities, which means I'm not sure how to silence clang on this one. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Jul 13, 2021 at 08:01:34AM -0500, Eric Blake wrote: > > > @@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > > > { > > > int ret; > > > g_autofree char *export_name = NULL; > > > - g_autofree bool *bitmaps = NULL; > > > NBDExportMetaContexts local_meta = {0}; > > Actually, "bitmaps" _is_ used, in cleanup handler, setup by g_autofree. So it's a false positive. > > > > Correct; this patch is wrong, and would cause a memory leak. This is a > false positive in clang, and a known issue that clang is in general > unable to see that g_autofree variables are used, sometimes for their > intentional side effects such as easier memory cleanup as done here. > > I suspect that the definition of g_autofree already uses > __attribute__((unused)) to work around clang's oddities, which means > I'm not sure how to silence clang on this one. Hmm; in glib 2.68.2 (on Fedora 34), g_autofree does NOT include an attribute unused. Thus, does this silence the compiler? (Even cooler would be making the comment a link to an actual bug in the clang database, but I couldn't quickly find one) diff --git i/nbd/server.c w/nbd/server.c index b60ebc3ab6ac..393cbd81c57a 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -973,7 +973,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client, { int ret; g_autofree char *export_name = NULL; - g_autofree bool *bitmaps = NULL; + /* G_GNUC_UNUSED needed to work around a clang bug */ + g_autofree G_GNUC_UNUSED bool *bitmaps = NULL; NBDExportMetaContexts local_meta = {0}; uint32_t nb_queries; size_t i; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 7/13/21 6:14 AM, Eric Blake wrote: > Hmm; in glib 2.68.2 (on Fedora 34), g_autofree does NOT include an > attribute unused. Thus, does this silence the compiler? (Even cooler > would be making the comment a link to an actual bug in the clang > database, but I couldn't quickly find one) > > diff --git i/nbd/server.c w/nbd/server.c > index b60ebc3ab6ac..393cbd81c57a 100644 > --- i/nbd/server.c > +++ w/nbd/server.c > @@ -973,7 +973,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > { > int ret; > g_autofree char *export_name = NULL; > - g_autofree bool *bitmaps = NULL; > + /* G_GNUC_UNUSED needed to work around a clang bug */ > + g_autofree G_GNUC_UNUSED bool *bitmaps = NULL; That works. I found https://bugs.llvm.org/show_bug.cgi?id=3888 and gave it a nudge. r~
diff --git a/nbd/server.c b/nbd/server.c index b60ebc3ab6..721349ec00 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -973,7 +973,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client, { int ret; g_autofree char *export_name = NULL; - g_autofree bool *bitmaps = NULL; NBDExportMetaContexts local_meta = {0}; uint32_t nb_queries; size_t i; @@ -1007,9 +1006,6 @@ static int nbd_negotiate_meta_queries(NBDClient *client, "export '%s' not present", sane_name); } meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps); - if (client->opt == NBD_OPT_LIST_META_CONTEXT) { - bitmaps = meta->bitmaps; - } ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), false, errp); if (ret <= 0) {
From clang-13: nbd/server.c:976:22: error: variable 'bitmaps' set but not used \ [-Werror,-Wunused-but-set-variable] Cc: qemu-block@nongnu.org Cc: Eric Blake <eblake@redhat.com> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- nbd/server.c | 4 ---- 1 file changed, 4 deletions(-) -- 2.25.1