GCC 8 added a -Wcast-function-type warning to -Wextra by
default. This complains if you try to explicitly cast
one function signature to another function signature
with incompatible args. This is something we do many
times in libvirt especially with event callbacks. The
hack to silence the warning requires casting via a
void(*)(void) signature before casting to the desired
type. This gross, so we just disable this new warning.

GCC 8 also became more fussy about detecting switch
fallthroughs. First it doesn't like it if you have
a fallthrough attribute that is not before a case
statement. e.g.

   FOO:
   BAR:
   WIZZ:
      ATTRIBUTE_FALLTHROUGH;

Is unacceptable as there's no final case statement,
so while FOO & BAR are falling through, WIZZ is
not falling through. IOW, GCC wants us to write

  FOO:
  BAR:
    ATTRIBUTE_FALLTHROUGH;
  WIZZ:

Second, it will report risk of fallthrough even if you
have a case statement for every single enum value, but
only if the switch is nested inside another switch and
the outer case statement has no final break. This is
is arguably valid because despite the fact that we have
cast from "int" to the enum typedef, nothing guarantees
that the variable we're switching on only contains values
that have corresponding switch labels. e.g.

   int domstate = 87539319;
   switch ((virDomainState)domstate) {
      ...
   }

will not match enum value, but also not raise any kind
of compiler warning. So it is right to complain about
risk of fallthrough if no default: is present.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 m4/virt-compile-warnings.m4    |  2 ++
 src/qemu/qemu_domain.c         | 14 +++++++-------
 src/qemu/qemu_domain_address.c | 11 +++++++++++
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
index b9c974842..4b35a6f6b 100644
--- a/m4/virt-compile-warnings.m4
+++ b/m4/virt-compile-warnings.m4
@@ -177,6 +177,8 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
     # with gl_MANYWARN_COMPLEMENT
     # So we have -W enabled, and then have to explicitly turn off...
     wantwarn="$wantwarn -Wno-sign-compare"
+    # We do "bad" function casts all the time for event callbacks
+    wantwarn="$wantwarn -Wno-cast-function-type"
 
     # GNULIB expects this to be part of -Wc++-compat, but we turn
     # that one off, so we need to manually enable this again
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 178ec24ae..560436f8e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -175,9 +175,9 @@ qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
     case QEMU_ASYNC_JOB_NONE:
     case QEMU_ASYNC_JOB_LAST:
         ATTRIBUTE_FALLTHROUGH;
+    default:
+        return "none";
     }
-
-    return "none";
 }
 
 int
@@ -199,12 +199,12 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
     case QEMU_ASYNC_JOB_NONE:
     case QEMU_ASYNC_JOB_LAST:
         ATTRIBUTE_FALLTHROUGH;
+    default:
+        if (STREQ(phase, "none"))
+            return 0;
+        else
+            return -1;
     }
-
-    if (STREQ(phase, "none"))
-        return 0;
-    else
-        return -1;
 }
 
 
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e28119e4b..42c7c0b12 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -532,6 +532,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
             case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2: /* xen only */
             case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
             case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
+            default:
                 return 0;
             }
 
@@ -553,6 +554,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
                 return pciFlags;
 
             case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
+            default:
                 return 0;
             }
 
@@ -562,6 +564,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
         case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
         case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+        default:
             /* should be 0 */
             return pciFlags;
         }
@@ -605,6 +608,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_SOUND_MODEL_PCSPK:
         case VIR_DOMAIN_SOUND_MODEL_USB:
         case VIR_DOMAIN_SOUND_MODEL_LAST:
+        default:
             return 0;
         }
 
@@ -622,6 +626,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_DISK_BUS_SATA:
         case VIR_DOMAIN_DISK_BUS_SD:
         case VIR_DOMAIN_DISK_BUS_LAST:
+        default:
             return 0;
         }
 
@@ -734,6 +739,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_MEMBALLOON_MODEL_XEN:
         case VIR_DOMAIN_MEMBALLOON_MODEL_NONE:
         case VIR_DOMAIN_MEMBALLOON_MODEL_LAST:
+        default:
             return 0;
         }
 
@@ -743,6 +749,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
             return virtioFlags;
 
         case VIR_DOMAIN_RNG_MODEL_LAST:
+        default:
             return 0;
         }
 
@@ -755,6 +762,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_WATCHDOG_MODEL_IB700:
         case VIR_DOMAIN_WATCHDOG_MODEL_DIAG288:
         case VIR_DOMAIN_WATCHDOG_MODEL_LAST:
+        default:
             return 0;
         }
 
@@ -775,6 +783,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
         case VIR_DOMAIN_VIDEO_TYPE_GOP:
         case VIR_DOMAIN_VIDEO_TYPE_LAST:
+        default:
             return 0;
         }
 
@@ -791,6 +800,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_INPUT_BUS_XEN:
         case VIR_DOMAIN_INPUT_BUS_PARALLELS:
         case VIR_DOMAIN_INPUT_BUS_LAST:
+        default:
             return 0;
         }
 
@@ -806,6 +816,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
         case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
         case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
         case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+        default:
             return 0;
         }
 
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Feb 13, 2018 at 12:21:11 +0000, Daniel P. Berrangé wrote:
> GCC 8 added a -Wcast-function-type warning to -Wextra by
> default. This complains if you try to explicitly cast
> one function signature to another function signature
> with incompatible args. This is something we do many
> times in libvirt especially with event callbacks. The
> hack to silence the warning requires casting via a
> void(*)(void) signature before casting to the desired
> type. This gross, so we just disable this new warning.
> 
> GCC 8 also became more fussy about detecting switch
> fallthroughs. First it doesn't like it if you have
> a fallthrough attribute that is not before a case
> statement. e.g.
> 
>    FOO:
>    BAR:
>    WIZZ:
>       ATTRIBUTE_FALLTHROUGH;
> 
> Is unacceptable as there's no final case statement,
> so while FOO & BAR are falling through, WIZZ is
> not falling through. IOW, GCC wants us to write
> 
>   FOO:
>   BAR:
>     ATTRIBUTE_FALLTHROUGH;
>   WIZZ:
> 
> Second, it will report risk of fallthrough even if you
> have a case statement for every single enum value, but
> only if the switch is nested inside another switch and
> the outer case statement has no final break. This is
> is arguably valid because despite the fact that we have
> cast from "int" to the enum typedef, nothing guarantees
> that the variable we're switching on only contains values
> that have corresponding switch labels. e.g.
> 
>    int domstate = 87539319;
>    switch ((virDomainState)domstate) {
>       ...
>    }
> 
> will not match enum value, but also not raise any kind
> of compiler warning. So it is right to complain about
> risk of fallthrough if no default: is present.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  m4/virt-compile-warnings.m4    |  2 ++
>  src/qemu/qemu_domain.c         | 14 +++++++-------
>  src/qemu/qemu_domain_address.c | 11 +++++++++++
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
> index b9c974842..4b35a6f6b 100644
> --- a/m4/virt-compile-warnings.m4
> +++ b/m4/virt-compile-warnings.m4
> @@ -177,6 +177,8 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
>      # with gl_MANYWARN_COMPLEMENT
>      # So we have -W enabled, and then have to explicitly turn off...
>      wantwarn="$wantwarn -Wno-sign-compare"
> +    # We do "bad" function casts all the time for event callbacks
> +    wantwarn="$wantwarn -Wno-cast-function-type"
>  
>      # GNULIB expects this to be part of -Wc++-compat, but we turn
>      # that one off, so we need to manually enable this again
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 178ec24ae..560436f8e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -175,9 +175,9 @@ qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
>      case QEMU_ASYNC_JOB_NONE:
>      case QEMU_ASYNC_JOB_LAST:
>          ATTRIBUTE_FALLTHROUGH;
> +    default:
> +        return "none";
>      }
> -
> -    return "none";
>  }
>  
>  int

Please don't do this. I hope there's a better way of silencing the
warnings. The reason for not providing a default value is to make sure
gcc emits a warning one the corresponding enum is changed. This change
will prevent gcc from reporting all places where the new item needs to
be explicitly handled.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Feb 13, 2018 at 01:58:44PM +0100, Jiri Denemark wrote:
> On Tue, Feb 13, 2018 at 12:21:11 +0000, Daniel P. Berrangé wrote:
> > GCC 8 added a -Wcast-function-type warning to -Wextra by
> > default. This complains if you try to explicitly cast
> > one function signature to another function signature
> > with incompatible args. This is something we do many
> > times in libvirt especially with event callbacks. The
> > hack to silence the warning requires casting via a
> > void(*)(void) signature before casting to the desired
> > type. This gross, so we just disable this new warning.
> > 
> > GCC 8 also became more fussy about detecting switch
> > fallthroughs. First it doesn't like it if you have
> > a fallthrough attribute that is not before a case
> > statement. e.g.
> > 
> >    FOO:
> >    BAR:
> >    WIZZ:
> >       ATTRIBUTE_FALLTHROUGH;
> > 
> > Is unacceptable as there's no final case statement,
> > so while FOO & BAR are falling through, WIZZ is
> > not falling through. IOW, GCC wants us to write
> > 
> >   FOO:
> >   BAR:
> >     ATTRIBUTE_FALLTHROUGH;
> >   WIZZ:
> > 
> > Second, it will report risk of fallthrough even if you
> > have a case statement for every single enum value, but
> > only if the switch is nested inside another switch and
> > the outer case statement has no final break. This is
> > is arguably valid because despite the fact that we have
> > cast from "int" to the enum typedef, nothing guarantees
> > that the variable we're switching on only contains values
> > that have corresponding switch labels. e.g.
> > 
> >    int domstate = 87539319;
> >    switch ((virDomainState)domstate) {
> >       ...
> >    }
> > 
> > will not match enum value, but also not raise any kind
> > of compiler warning. So it is right to complain about
> > risk of fallthrough if no default: is present.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  m4/virt-compile-warnings.m4    |  2 ++
> >  src/qemu/qemu_domain.c         | 14 +++++++-------
> >  src/qemu/qemu_domain_address.c | 11 +++++++++++
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
> > index b9c974842..4b35a6f6b 100644
> > --- a/m4/virt-compile-warnings.m4
> > +++ b/m4/virt-compile-warnings.m4
> > @@ -177,6 +177,8 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
> >      # with gl_MANYWARN_COMPLEMENT
> >      # So we have -W enabled, and then have to explicitly turn off...
> >      wantwarn="$wantwarn -Wno-sign-compare"
> > +    # We do "bad" function casts all the time for event callbacks
> > +    wantwarn="$wantwarn -Wno-cast-function-type"
> >  
> >      # GNULIB expects this to be part of -Wc++-compat, but we turn
> >      # that one off, so we need to manually enable this again
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 178ec24ae..560436f8e 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -175,9 +175,9 @@ qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
> >      case QEMU_ASYNC_JOB_NONE:
> >      case QEMU_ASYNC_JOB_LAST:
> >          ATTRIBUTE_FALLTHROUGH;
> > +    default:
> > +        return "none";
> >      }
> > -
> > -    return "none";
> >  }
> >  
> >  int
> 
> Please don't do this. I hope there's a better way of silencing the
> warnings. The reason for not providing a default value is to make sure
> gcc emits a warning one the corresponding enum is changed. This change
> will prevent gcc from reporting all places where the new item needs to
> be explicitly handled.

That's because we're currently relying on -Wswitch to report missing
enums. That explicitly doesn't warn if you have  default.  If we
enable -Wswitch-enum, then we get a warning about missing cases
regardless of whether a default is present. I'll repost with that
warning flag changed to address this.

Looking at our code though, I think we ought to turn on
-Wswitch-default too, and thus ensure we have all enums
cases covered and a default in everything.

Relying on just listing known enum values is dangerous if there are
bugs in the code which don't initialize the field based from an
enum constant, but instead use an plain int value. It is rare but
we do have some such places, as well as potential for code bugs
where we use the wrong enum type to initialize a field which wont
be caught as all struct fields are ints not the typed enum.

I've notice our handling of the _LAST value is flawed in a huge
number of cases too - it should almost always result in an error
being reported because we should never see that value. In places
which do report an error, the _LAST value is sometimes being fed
to a fooToString() method which will fail because _LAST is out of
range for printable strings.

IOW, most of our enums should really end with

   FOO_LAST:
   default:
      virReportError(VIR_ERR_INTERNAL_ERROR,
                     _("Unexpected foo type %d"), type);
      return -1;

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 Tue, 2018-02-13 at 12:21 +0000, Daniel P. Berrangé wrote:
> @@ -175,9 +175,9 @@ qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
>      case QEMU_ASYNC_JOB_NONE:
>      case QEMU_ASYNC_JOB_LAST:
>          ATTRIBUTE_FALLTHROUGH;
> +    default:
> +        return "none";
>      }
> -
> -    return "none";
>  }

Can't we just replace ATTRIBUTE_FALLTHROUGH with a break? Or even
just duplicate the return statement, without adding the default
label, if that doesn't help? Same for the next hunk.

> @@ -532,6 +532,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>              case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2: /* xen only */
>              case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
>              case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> +            default:
>                  return 0;
>              }

Adding the default label here and in the following hunks will
forfeit the advantage of having the compiler catch for us cases
where we introduced a new model but didn't update all the code
that needs to deal with it accordingly. IMHO that should only be
considered as a very last resort if we can't possibly otherwise
restructure the code in a way that makes the compiler happy.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list