[libvirt] [PATCH 2/2] tests: Avoid "jump skips variable initialization" with GCC 9.

[Daniel P. Berrangé]

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 :|