GCC 9 gives pages of errors like:
qemumonitorjsontest.c: In function 'mymain':
qemumonitorjsontest.c:2904:9: error: jump skips variable initialization [-Werror=jump-misses-init]
2904 | goto cleanup;
| ^~~~
qemumonitorjsontest.c:3111:2: note: label 'cleanup' defined here
3111 | cleanup:
| ^~~~~~~
qemumonitorjsontest.c:2920:54: note: '({anonymous})' declared here
2920 | simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt, \
| ^
qemumonitorjsontest.c:3008:5: note: in expansion of macro 'DO_TEST_GEN'
3008 | DO_TEST_GEN(qemuMonitorJSONBlockdevMediumInsert);
| ^~~~~~~~~~~
By moving the cleanup section up near the top of the function we can
avoid this. I think a better way might be to disable the warning.
---
tests/qemumonitorjsontest.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 1a8a31717f..299c5f0cbe 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2900,8 +2900,12 @@ mymain(void)
if (!(qapiData.schema = testQEMUSchemaLoad())) {
VIR_TEST_VERBOSE("failed to load qapi schema\n");
- ret = -1;
- goto cleanup;
+ cleanup:
+ VIR_FREE(metaschemastr);
+ virJSONValueFree(metaschema);
+ virHashFree(qapiData.schema);
+ qemuTestDriverFree(&driver);
+ return -1;
}
#define DO_TEST(name) \
@@ -3098,7 +3102,6 @@ mymain(void)
if (!(metaschema = testQEMUSchemaGetLatest()) ||
!(metaschemastr = virJSONValueToString(metaschema, false))) {
VIR_TEST_VERBOSE("failed to load latest qapi schema\n");
- ret = -1;
goto cleanup;
}
@@ -3108,7 +3111,6 @@ mymain(void)
#undef DO_TEST_QAPI_SCHEMA
- cleanup:
VIR_FREE(metaschemastr);
virJSONValueFree(metaschema);
virHashFree(qapiData.schema);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 21, 2019 at 03:13:21PM +0000, Richard W.M. Jones wrote:
> GCC 9 gives pages of errors like:
>
> qemumonitorjsontest.c: In function 'mymain':
> qemumonitorjsontest.c:2904:9: error: jump skips variable initialization [-Werror=jump-misses-init]
> 2904 | goto cleanup;
> | ^~~~
> qemumonitorjsontest.c:3111:2: note: label 'cleanup' defined here
> 3111 | cleanup:
> | ^~~~~~~
> qemumonitorjsontest.c:2920:54: note: '({anonymous})' declared here
> 2920 | simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt, \
> | ^
> qemumonitorjsontest.c:3008:5: note: in expansion of macro 'DO_TEST_GEN'
> 3008 | DO_TEST_GEN(qemuMonitorJSONBlockdevMediumInsert);
> | ^~~~~~~~~~~
Ok, it is complaining about "simpleFunc", because for the 5 locally
declared variables, we've done explicit initialization before any
goto's are encountered. The fun thing is that the cleanup: block
does not reference "simpleFunc" in any way - it is only ever used
immediately after it is initialized.
So GCC is warning about something that can't happen here.
The definition of -Wjump-misses-init says
[quote]
Warn if a 'goto' statement or a 'switch' statement jumps forward
across the initialization of a variable, or jumps backward to a
label after the variable has been initialized. This only warns
about variables that are initialized when they are declared.
[/quote]
we're jumping forwards here, but the last sentance suggests it would
only warn if you are jumping over the variable declaration. ie if we
had
testQemuMonitorJSONSimpleFuncData simpleFunc;
in the middle of the method, instead of top of the method. So either
they've intentionally expanded the scope of the warning, or this is
a bug in GCC.
Either way the simple solution is probably just:
testQemuMonitorJSONSimpleFuncData simpleFunc = NULL;
>
> By moving the cleanup section up near the top of the function we can
> avoid this. I think a better way might be to disable the warning.
> ---
> tests/qemumonitorjsontest.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 1a8a31717f..299c5f0cbe 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -2900,8 +2900,12 @@ mymain(void)
>
> if (!(qapiData.schema = testQEMUSchemaLoad())) {
> VIR_TEST_VERBOSE("failed to load qapi schema\n");
> - ret = -1;
> - goto cleanup;
> + cleanup:
> + VIR_FREE(metaschemastr);
> + virJSONValueFree(metaschema);
> + virHashFree(qapiData.schema);
> + qemuTestDriverFree(&driver);
> + return -1;
> }
>
> #define DO_TEST(name) \
> @@ -3098,7 +3102,6 @@ mymain(void)
> if (!(metaschema = testQEMUSchemaGetLatest()) ||
> !(metaschemastr = virJSONValueToString(metaschema, false))) {
> VIR_TEST_VERBOSE("failed to load latest qapi schema\n");
> - ret = -1;
> goto cleanup;
> }
>
> @@ -3108,7 +3111,6 @@ mymain(void)
>
> #undef DO_TEST_QAPI_SCHEMA
>
> - cleanup:
> VIR_FREE(metaschemastr);
> virJSONValueFree(metaschema);
> virHashFree(qapiData.schema);
> --
> 2.20.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 21, 2019 at 15:13:21 +0000, Richard W.M. Jones wrote:
> GCC 9 gives pages of errors like:
>
> qemumonitorjsontest.c: In function 'mymain':
> qemumonitorjsontest.c:2904:9: error: jump skips variable initialization [-Werror=jump-misses-init]
> 2904 | goto cleanup;
> | ^~~~
> qemumonitorjsontest.c:3111:2: note: label 'cleanup' defined here
> 3111 | cleanup:
> | ^~~~~~~
> qemumonitorjsontest.c:2920:54: note: '({anonymous})' declared here
> 2920 | simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt, \
> | ^
> qemumonitorjsontest.c:3008:5: note: in expansion of macro 'DO_TEST_GEN'
> 3008 | DO_TEST_GEN(qemuMonitorJSONBlockdevMediumInsert);
> | ^~~~~~~~~~~
>
> By moving the cleanup section up near the top of the function we can
> avoid this. I think a better way might be to disable the warning.
> ---
> tests/qemumonitorjsontest.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 1a8a31717f..299c5f0cbe 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -2900,8 +2900,12 @@ mymain(void)
>
> if (!(qapiData.schema = testQEMUSchemaLoad())) {
> VIR_TEST_VERBOSE("failed to load qapi schema\n");
> - ret = -1;
> - goto cleanup;
> + cleanup:
> + VIR_FREE(metaschemastr);
> + virJSONValueFree(metaschema);
> + virHashFree(qapiData.schema);
> + qemuTestDriverFree(&driver);
> + return -1;
> }
>
> #define DO_TEST(name) \
> @@ -3098,7 +3102,6 @@ mymain(void)
> if (!(metaschema = testQEMUSchemaGetLatest()) ||
> !(metaschemastr = virJSONValueToString(metaschema, false))) {
> VIR_TEST_VERBOSE("failed to load latest qapi schema\n");
> - ret = -1;
> goto cleanup;
We generally avoid jumps back. Also cleanup really should be at the end.
Wouldn't be enough just to memset the few structs at the beginning?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 21, 2019 at 04:38:25PM +0100, Peter Krempa wrote:
> On Mon, Jan 21, 2019 at 15:13:21 +0000, Richard W.M. Jones wrote:
> > GCC 9 gives pages of errors like:
> >
> > qemumonitorjsontest.c: In function 'mymain':
> > qemumonitorjsontest.c:2904:9: error: jump skips variable initialization [-Werror=jump-misses-init]
> > 2904 | goto cleanup;
> > | ^~~~
> > qemumonitorjsontest.c:3111:2: note: label 'cleanup' defined here
> > 3111 | cleanup:
> > | ^~~~~~~
> > qemumonitorjsontest.c:2920:54: note: '({anonymous})' declared here
> > 2920 | simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt, \
> > | ^
> > qemumonitorjsontest.c:3008:5: note: in expansion of macro 'DO_TEST_GEN'
> > 3008 | DO_TEST_GEN(qemuMonitorJSONBlockdevMediumInsert);
> > | ^~~~~~~~~~~
> >
> > By moving the cleanup section up near the top of the function we can
> > avoid this. I think a better way might be to disable the warning.
> > ---
> > tests/qemumonitorjsontest.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> > index 1a8a31717f..299c5f0cbe 100644
> > --- a/tests/qemumonitorjsontest.c
> > +++ b/tests/qemumonitorjsontest.c
> > @@ -2900,8 +2900,12 @@ mymain(void)
> >
> > if (!(qapiData.schema = testQEMUSchemaLoad())) {
> > VIR_TEST_VERBOSE("failed to load qapi schema\n");
> > - ret = -1;
> > - goto cleanup;
> > + cleanup:
> > + VIR_FREE(metaschemastr);
> > + virJSONValueFree(metaschema);
> > + virHashFree(qapiData.schema);
> > + qemuTestDriverFree(&driver);
> > + return -1;
> > }
> >
> > #define DO_TEST(name) \
> > @@ -3098,7 +3102,6 @@ mymain(void)
> > if (!(metaschema = testQEMUSchemaGetLatest()) ||
> > !(metaschemastr = virJSONValueToString(metaschema, false))) {
> > VIR_TEST_VERBOSE("failed to load latest qapi schema\n");
> > - ret = -1;
> > goto cleanup;
>
> We generally avoid jumps back. Also cleanup really should be at the end.
>
> Wouldn't be enough just to memset the few structs at the beginning?
The warning turns out to be insanely misleading. Here we are thinking
that it is complaining about missing initializers for "simpleFunc",
but that is not in fact what's going on.
When we have code
testQemuMonitorJSONSimpleFuncData simpleFunc = {0}
...some code with gotos..
simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt,
.schema = qapiData.schema
__VA_ARGS__ };
What GCC actually sees after parsing is
testQemuMonitorJSONSimpleFuncData simpleFunc = {0}
...some code with gotos..
testQemuMonitorJSONSimpleFuncData anonymous = {.xmlopt = driver.xmlopt,
.schema = qapiData.schema
__VA_ARGS__ };
simpleFunc = anoymous;
and the warning is complaining that we have a goto jumping over the
initialization of this "anonymous" variable, which is in fact true,
but horrible because we have no idea this "anonymous" variable even
exists !
Libvirt's usage is safe, because this anonymous variable is only
used once (immediately) and so would never be relevant later in
the target of the goto.
The reason GCC warns is that you can apparently take the address of
an anonymous compound initializer, in which case the anonymous
variable is alive for the rest of the function !
eg it is possible to write
testQemuMonitorJSONSimpleFuncData *simpleFunc = NULL;
...some code with gotos..
simpleFunc = &(testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt,
.schema = qapiData.schema
__VA_ARGS__ };
which GCC would see as
testQemuMonitorJSONSimpleFuncData *simpleFunc = NULL;
...some code with gotos..
testQemuMonitorJSONSimpleFuncData anonymous = {.xmlopt = driver.xmlopt,
.schema = qapiData.schema
__VA_ARGS__ };
simpleFunc = &anoymous;
and so anoymous remains live for the remainder of the code block.
GCC devs are investigating whether its possible to make GCC more
intelligent and only warn in the latter case where there's a genuine
risk, and so avoid complaints in libvirt's case which is safe.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 21, 2019 at 04:38:25PM +0100, Peter Krempa wrote:
> On Mon, Jan 21, 2019 at 15:13:21 +0000, Richard W.M. Jones wrote:
> > GCC 9 gives pages of errors like:
> >
> > qemumonitorjsontest.c: In function 'mymain':
> > qemumonitorjsontest.c:2904:9: error: jump skips variable initialization [-Werror=jump-misses-init]
> > 2904 | goto cleanup;
> > | ^~~~
> > qemumonitorjsontest.c:3111:2: note: label 'cleanup' defined here
> > 3111 | cleanup:
> > | ^~~~~~~
> > qemumonitorjsontest.c:2920:54: note: '({anonymous})' declared here
> > 2920 | simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt, \
> > | ^
> > qemumonitorjsontest.c:3008:5: note: in expansion of macro 'DO_TEST_GEN'
> > 3008 | DO_TEST_GEN(qemuMonitorJSONBlockdevMediumInsert);
> > | ^~~~~~~~~~~
> >
> > By moving the cleanup section up near the top of the function we can
> > avoid this. I think a better way might be to disable the warning.
> > ---
> > tests/qemumonitorjsontest.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> > index 1a8a31717f..299c5f0cbe 100644
> > --- a/tests/qemumonitorjsontest.c
> > +++ b/tests/qemumonitorjsontest.c
> > @@ -2900,8 +2900,12 @@ mymain(void)
> >
> > if (!(qapiData.schema = testQEMUSchemaLoad())) {
> > VIR_TEST_VERBOSE("failed to load qapi schema\n");
> > - ret = -1;
> > - goto cleanup;
> > + cleanup:
> > + VIR_FREE(metaschemastr);
> > + virJSONValueFree(metaschema);
> > + virHashFree(qapiData.schema);
> > + qemuTestDriverFree(&driver);
> > + return -1;
> > }
> >
> > #define DO_TEST(name) \
> > @@ -3098,7 +3102,6 @@ mymain(void)
> > if (!(metaschema = testQEMUSchemaGetLatest()) ||
> > !(metaschemastr = virJSONValueToString(metaschema, false))) {
> > VIR_TEST_VERBOSE("failed to load latest qapi schema\n");
> > - ret = -1;
> > goto cleanup;
>
> We generally avoid jumps back. Also cleanup really should be at the end.
>
> Wouldn't be enough just to memset the few structs at the beginning?
No, this appears to be another GCC regression. IMHO the following should
not warn, yet it does:
$ cat demo.c
#include <stdlib.h>
struct demo {
const char *cmd;
};
int main(void)
{
struct demo demo = {0};
if ((demo.cmd = getenv("FOO")) == NULL) {
goto cleanup;
}
demo = (struct demo) { .cmd = "foo" };
cleanup:
return 0;
}
$ gcc -Wjump-misses-init -o demo demo.c
demo.c: In function ‘main’:
demo.c:13:5: warning: jump skips variable initialization [-Wjump-misses-init]
13 | goto cleanup;
| ^~~~
demo.c:18:2: note: label ‘cleanup’ defined here
18 | cleanup:
| ^~~~~~~
demo.c:16:24: note: ‘({anonymous})’ declared here
16 | demo = (struct demo) { .cmd = "foo" };
| ^
This doesn't appear to be reported to GCC upstream yet
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jan 25, 2019 at 12:54:13PM +0000, Daniel P. Berrangé wrote:
> On Mon, Jan 21, 2019 at 04:38:25PM +0100, Peter Krempa wrote:
> > On Mon, Jan 21, 2019 at 15:13:21 +0000, Richard W.M. Jones wrote:
> > > GCC 9 gives pages of errors like:
> > >
> > > qemumonitorjsontest.c: In function 'mymain':
> > > qemumonitorjsontest.c:2904:9: error: jump skips variable initialization [-Werror=jump-misses-init]
> > > 2904 | goto cleanup;
> > > | ^~~~
> > > qemumonitorjsontest.c:3111:2: note: label 'cleanup' defined here
> > > 3111 | cleanup:
> > > | ^~~~~~~
> > > qemumonitorjsontest.c:2920:54: note: '({anonymous})' declared here
> > > 2920 | simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt, \
> > > | ^
> > > qemumonitorjsontest.c:3008:5: note: in expansion of macro 'DO_TEST_GEN'
> > > 3008 | DO_TEST_GEN(qemuMonitorJSONBlockdevMediumInsert);
> > > | ^~~~~~~~~~~
> > >
> > > By moving the cleanup section up near the top of the function we can
> > > avoid this. I think a better way might be to disable the warning.
> > > ---
> > > tests/qemumonitorjsontest.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> > > index 1a8a31717f..299c5f0cbe 100644
> > > --- a/tests/qemumonitorjsontest.c
> > > +++ b/tests/qemumonitorjsontest.c
> > > @@ -2900,8 +2900,12 @@ mymain(void)
> > >
> > > if (!(qapiData.schema = testQEMUSchemaLoad())) {
> > > VIR_TEST_VERBOSE("failed to load qapi schema\n");
> > > - ret = -1;
> > > - goto cleanup;
> > > + cleanup:
> > > + VIR_FREE(metaschemastr);
> > > + virJSONValueFree(metaschema);
> > > + virHashFree(qapiData.schema);
> > > + qemuTestDriverFree(&driver);
> > > + return -1;
> > > }
> > >
> > > #define DO_TEST(name) \
> > > @@ -3098,7 +3102,6 @@ mymain(void)
> > > if (!(metaschema = testQEMUSchemaGetLatest()) ||
> > > !(metaschemastr = virJSONValueToString(metaschema, false))) {
> > > VIR_TEST_VERBOSE("failed to load latest qapi schema\n");
> > > - ret = -1;
> > > goto cleanup;
> >
> > We generally avoid jumps back. Also cleanup really should be at the end.
> >
> > Wouldn't be enough just to memset the few structs at the beginning?
>
> No, this appears to be another GCC regression. IMHO the following should
> not warn, yet it does:
{snip}
> This doesn't appear to be reported to GCC upstream yet
I've filed
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89061
https://bugzilla.redhat.com/show_bug.cgi?id=1669489
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.