diff mbox series

[v8,04/87] target/mips: Mark switch fallthroughs with interpretable comments

Message ID 1534182832-554-5-git-send-email-aleksandar.markovic@rt-rk.com
State New
Headers show
Series Add nanoMIPS support to QEMU | expand

Commit Message

Aleksandar Markovic Aug. 13, 2018, 5:52 p.m. UTC 
From: Aleksandar Markovic <amarkovic@wavecomp.com>

Mark switch fallthroughs with comments, in cases fallthroughs
are intentional.

The comments "/* fall through */" are interpreted by compilers and
other tools, and they will not issue warnings in such cases. For gcc,
the warning is turnend on by -Wimplicit-fallthrough. With this patch,
there will be no such warnings in target/mips directory. If such
warning appears in future, it should be checked if it is intentional,
and, if yes, marked with a comment similar to those from this patch.

The comment must be just before next "case", otherwise gcc won't
understand it.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 target/mips/translate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Stefan Weil July 7, 2019, 8:26 p.m. UTC | #1 
Am 13.08.18 um 19:52 schrieb Aleksandar Markovic:

> From: Aleksandar Markovic <amarkovic@wavecomp.com>
>
> Mark switch fallthroughs with comments, in cases fallthroughs
> are intentional.


This is a general problem all over the QEMU code. I usually compile with 
nearly all warnings enabled and get now lots of errors with the latest 
code and after updating to gcc-8.3.0 (Debian buster). It should be 
reproducible by enabling -Werror=implicit-fallthrough.

The current situation is like this:

- Some code has fallthrough comments which are accepted by the compiler.

- Other code has fallthrough comments which are not accepted (resulting 
in a compiler error).

- Some code is correct, but has no indication that the fallthrough is 
intentional.

- There is also fallthrough code which is obviously not correct (even in 
target/mips/translate.c).


I suggest to enable -Werror=implicit-fallthrough by default and add a 
new macro to mark all fallthrough locations which are correct, but not 
accepted by the compiler.

This can be done with a definition for GCC compatible compilers in 
include/qemu/compiler.h:

#define QEMU_FALLTHROUGH __attribute__ ((fallthrough))

Then fallthrough code would look like this:

     case 1:
         do_something();
         QEMU_FALLTHROUGH;

     case 2:


VIXL_FALLTHROUGH also needs a similar definition to work with gcc-8.3.0.

Please comment. Would you prefer another macro name or a macro with 
parentheses like this:

#define QEMU_FALLTHROUGH() __attribute__ ((fallthrough))


As soon as there is consensus on the macro name and form, I can send a 
patch which adds it (but would not mind if someone else adds it).

Then I suggest that the maintainers build with the fallthrough warning 
enabled and fix all warnings, either by using the new macro or by adding 
the missing break.

Finally we can enforce the warning by default.


Another macro which is currently missing is a scanf variant of GCC_FMT_ATTR.

I suggest to add and use a GCC_SCANF_ATTR macro:

#define GCC_SCANF_ATTR(n, m) __attribute__((format(gnu_scanf, n, m)))

A more regular solution would require renaming GCC_FMT_ATTR to 
GCC_FMT_PRINTF and use GCC_FMT_SCANF for the new macro.


Regards
Stefan Weil
Markus Armbruster July 8, 2019, 4:40 a.m. UTC | #2 
Stefan Weil <sw@weilnetz.de> writes:

> Am 13.08.18 um 19:52 schrieb Aleksandar Markovic:
>
>> From: Aleksandar Markovic <amarkovic@wavecomp.com>
>>
>> Mark switch fallthroughs with comments, in cases fallthroughs
>> are intentional.
>
>
> This is a general problem all over the QEMU code. I usually compile
> with nearly all warnings enabled and get now lots of errors with the
> latest code and after updating to gcc-8.3.0 (Debian buster). It should
> be reproducible by enabling -Werror=implicit-fallthrough.
>
> The current situation is like this:
>
> - Some code has fallthrough comments which are accepted by the compiler.
>
> - Other code has fallthrough comments which are not accepted
> (resulting in a compiler error).
>
> - Some code is correct, but has no indication that the fallthrough is
> intentional.

I'd treat that as a bug.

> - There is also fallthrough code which is obviously not correct (even
> in target/mips/translate.c).

Bug.

> I suggest to enable -Werror=implicit-fallthrough by default and add a
> new macro to mark all fallthrough locations which are correct, but not
> accepted by the compiler.
>
> This can be done with a definition for GCC compatible compilers in
> include/qemu/compiler.h:
>
> #define QEMU_FALLTHROUGH __attribute__ ((fallthrough))
>
> Then fallthrough code would look like this:
>
>     case 1:
>         do_something();
>         QEMU_FALLTHROUGH;
>
>     case 2:
>
>
> VIXL_FALLTHROUGH also needs a similar definition to work with gcc-8.3.0.
>
> Please comment. Would you prefer another macro name or a macro with
> parentheses like this:
>
> #define QEMU_FALLTHROUGH() __attribute__ ((fallthrough))

In my opinion, the macro is no clearer than proper comments.

I'd prefer -Wimplicit-fallthrough=1 or 2.  The former makes gcc accept
any comment.  The latter makes it accept '.*falls?[ \t-]*thr(ough|u).*',
which should still match the majority of our comments.  Less churn than
the macro.

> As soon as there is consensus on the macro name and form, I can send a
> patch which adds it (but would not mind if someone else adds it).
>
> Then I suggest that the maintainers build with the fallthrough warning
> enabled and fix all warnings, either by using the new macro or by
> adding the missing break.
>
> Finally we can enforce the warning by default.
>
>
> Another macro which is currently missing is a scanf variant of GCC_FMT_ATTR.
>
> I suggest to add and use a GCC_SCANF_ATTR macro:
>
> #define GCC_SCANF_ATTR(n, m) __attribute__((format(gnu_scanf, n, m)))

Do we define our own scanf()-like functions?  If yes, decorating them
with the attribute is a good idea.

However, the gnu_ in gnu_scanf tells the compiler we're linking with the
GNU C Library, which seems unwise.  Hmm, we already use gnu_printf.
Commit 9c9e7d51bf0:

    Newer gcc versions support format gnu_printf which is
    better suited for use in QEMU than format printf
    (QEMU always uses standard format strings (even with mingw32)).

Should we limit the use of gnu_printf to #ifdef _WIN32?

> A more regular solution would require renaming GCC_FMT_ATTR to
> GCC_FMT_PRINTF and use GCC_FMT_SCANF for the new macro.

Quite some churn, but regularity matters.
Stefan Weil July 8, 2019, 4:52 a.m. UTC | #3 
Am 08.07.19 um 06:40 schrieb Markus Armbruster:

> Stefan Weil <sw@weilnetz.de> writes:
>
>> - Some code is correct, but has no indication that the fallthrough is
>> intentional.
> I'd treat that as a bug.


Sure.


>
>> - There is also fallthrough code which is obviously not correct (even
>> in target/mips/translate.c).
> Bug.


Yes, of course.


>
>> I suggest to enable -Werror=implicit-fallthrough by default and add a
>> new macro to mark all fallthrough locations which are correct, but not
>> accepted by the compiler.
>>
>> This can be done with a definition for GCC compatible compilers in
>> include/qemu/compiler.h:
>>
>> #define QEMU_FALLTHROUGH __attribute__ ((fallthrough))
>>
>> Then fallthrough code would look like this:
>>
>>      case 1:
>>          do_something();
>>          QEMU_FALLTHROUGH;
>>
>>      case 2:
>>
>>
>> VIXL_FALLTHROUGH also needs a similar definition to work with gcc-8.3.0.
>>
>> Please comment. Would you prefer another macro name or a macro with
>> parentheses like this:
>>
>> #define QEMU_FALLTHROUGH() __attribute__ ((fallthrough))
> In my opinion, the macro is no clearer than proper comments.
>
> I'd prefer -Wimplicit-fallthrough=1 or 2.  The former makes gcc accept
> any comment.  The latter makes it accept '.*falls?[ \t-]*thr(ough|u).*',
> which should still match the majority of our comments.  Less churn than
> the macro.
[...]
> Another macro which is currently missing is a scanf variant of GCC_FMT_ATTR.
>
> I suggest to add and use a GCC_SCANF_ATTR macro:
>
> #define GCC_SCANF_ATTR(n, m) __attribute__((format(gnu_scanf, n, m)))
> Do we define our own scanf()-like functions?  If yes, decorating them
> with the attribute is a good idea.


xen_device_backend_scanf, xs_node_vscanf, xs_node_scanf, 
xen_device_frontend_scanf

Maybe more. The compiler can tell you missing attributes.


>
> However, the gnu_ in gnu_scanf tells the compiler we're linking with the
> GNU C Library, which seems unwise.  Hmm, we already use gnu_printf.
> Commit 9c9e7d51bf0:
>
>      Newer gcc versions support format gnu_printf which is
>      better suited for use in QEMU than format printf
>      (QEMU always uses standard format strings (even with mingw32)).
>
> Should we limit the use of gnu_printf to #ifdef _WIN32?


No, because we don't want lots of conditional code with different format 
strings for POSIX and Windows (I made that commit 9 years ago).


>> A more regular solution would require renaming GCC_FMT_ATTR to
>> GCC_FMT_PRINTF and use GCC_FMT_SCANF for the new macro.
> Quite some churn, but regularity matters.


I could do that when adding the new macro, but would like to hear more 
opinions on that.

Thank you,

Stefan
Aleksandar Markovic July 8, 2019, 8:14 a.m. UTC | #4 
On Jul 7, 2019 10:26 PM, "Stefan Weil" <sw@weilnetz.de> wrote:
>
> Am 13.08.18 um 19:52 schrieb Aleksandar Markovic:
>
>> From: Aleksandar Markovic <amarkovic@wavecomp.com>
>>
>> Mark switch fallthroughs with comments, in cases fallthroughs
>> are intentional.
>
>
>
> This is a general problem all over the QEMU code. I usually compile with
nearly all warnings enabled and get now lots of errors with the latest code
and after updating to gcc-8.3.0 (Debian buster). It should be reproducible
by enabling -Werror=implicit-fallthrough.
>

Hi, Stefan.

Thanks for bringing this to the attention of the community.

I am sure there is a number of nasty bugs of this nature in our code - yet
to be found.

> The current situation is like this:
>
> - Some code has fallthrough comments which are accepted by the compiler.
>
> - Other code has fallthrough comments which are not accepted (resulting
in a compiler error).
>
> - Some code is correct, but has no indication that the fallthrough is
intentional.
>
> - There is also fallthrough code which is obviously not correct (even in
target/mips/translate.c).

Can you please be more specific about those cases from
target/mips/translate.c?

Yours,
Aleksandar

>
>
> I suggest to enable -Werror=implicit-fallthrough by default and add a new
macro to mark all fallthrough locations which are correct, but not accepted
by the compiler.
>
> This can be done with a definition for GCC compatible compilers in
include/qemu/compiler.h:
>
> #define QEMU_FALLTHROUGH __attribute__ ((fallthrough))
>
> Then fallthrough code would look like this:
>
>     case 1:
>         do_something();
>         QEMU_FALLTHROUGH;
>
>     case 2:
>
>
> VIXL_FALLTHROUGH also needs a similar definition to work with gcc-8.3.0.
>
> Please comment. Would you prefer another macro name or a macro with
parentheses like this:
>
> #define QEMU_FALLTHROUGH() __attribute__ ((fallthrough))
>
>
> As soon as there is consensus on the macro name and form, I can send a
patch which adds it (but would not mind if someone else adds it).
>
> Then I suggest that the maintainers build with the fallthrough warning
enabled and fix all warnings, either by using the new macro or by adding
the missing break.
>
> Finally we can enforce the warning by default.
>
>
> Another macro which is currently missing is a scanf variant of
GCC_FMT_ATTR.
>
> I suggest to add and use a GCC_SCANF_ATTR macro:
>
> #define GCC_SCANF_ATTR(n, m) __attribute__((format(gnu_scanf, n, m)))
>
> A more regular solution would require renaming GCC_FMT_ATTR to
GCC_FMT_PRINTF and use GCC_FMT_SCANF for the new macro.
>
>
> Regards
> Stefan Weil
>
>
>
Peter Maydell July 8, 2019, 8:42 a.m. UTC | #5 
On Sun, 7 Jul 2019 at 21:26, Stefan Weil <sw@weilnetz.de> wrote:
> This is a general problem all over the QEMU code. I usually compile with
> nearly all warnings enabled and get now lots of errors with the latest
> code and after updating to gcc-8.3.0 (Debian buster). It should be
> reproducible by enabling -Werror=implicit-fallthrough.

Coverity warns about implicit fallthroughs, and we are
currently warning-free in that department, so I think
our remaining problems are largely down to perhaps
using idioms which the compiler doesn't spot.
Being able to enable gcc implicit-fallthrough errors would
definitely be better than spotting them only after the
fact with Coverity.

> I suggest to enable -Werror=implicit-fallthrough by default and add a
> new macro to mark all fallthrough locations which are correct, but not
> accepted by the compiler.

I'm not sure why we need a macro. Our standard way to
mark fallthrough is /* fall through */, which has hundreds
of uses in the codebase. -Wimplicit-fallthrough=2 will match this,
so it seems simpler to just use that rather than to rework
how we mark fallthroughs.

Since vixl is 3rd-party code it might be easier to just
add -Wno-implicit-fallthrough to the cflags that
disas/libvixl/Makefile.objs sets up for building those files.
(We should check also for newer libvixl and/or suggest
something upstream that works with gcc.)

thanks
-- PMM
Stefan Weil July 8, 2019, 12:04 p.m. UTC | #6 
Am 08.07.2019 um 10:14 schrieb Aleksandar Markovic:
>
> On Jul 7, 2019 10:26 PM, "Stefan Weil" <sw@weilnetz.de
> <mailto:sw@weilnetz.de>> wrote:
> > - There is also fallthrough code which is obviously not correct
> (even in target/mips/translate.c).
>
> Can you please be more specific about those cases from
> target/mips/translate.c?
>

Hi Aleksandar,

this is the list of warnings for target/mips/translate.c:

/home/debian/src/github/qemu/qemu/target/mips/translate.c:10047:13:
warning: this statement may fall through [-Wimplicit-fallthrough=]
/home/debian/src/github/qemu/qemu/target/mips/translate.c:10056:13:
warning: this statement may fall through [-Wimplicit-fallthrough=]
/home/debian/src/github/qemu/qemu/target/mips/translate.c:20138:13:
warning: this statement may fall through [-Wimplicit-fallthrough=]
/home/debian/src/github/qemu/qemu/target/mips/translate.c:20144:13:
warning: this statement may fall through [-Wimplicit-fallthrough=]
/home/debian/src/github/qemu/qemu/target/mips/translate.c:6739:9:
warning: this statement may fall through [-Wimplicit-fallthrough=]
/home/debian/src/github/qemu/qemu/target/mips/translate.c:9820:13:
warning: this statement may fall through [-Wimplicit-fallthrough=]
/home/debian/src/github/qemu/qemu/target/mips/translate.c:9829:13:
warning: this statement may fall through [-Wimplicit-fallthrough=]

I have built using

    ../configure --disable-werror '--extra-cflags=-Wextra
-Wimplicit-fallthrough=2'

The build process produced warnings for 79 fallthrough locations. See
build protocol:

    https://qemu.weilnetz.de/results/build-20190708.txt (116 MiB !)

Sometimes the fallthrough comment is misplaced in the following case
statement or followed by code, so the compiler (correctly) won't accept
it. Maybe there exist also fallthrough comments although there is no
fallthrough code.

The protocol also shows 18 warnings [-Wcast-function-type] which might
indicate bugs:

/home/debian/src/github/qemu/qemu/chardev/char-fe.c:372:32: warning:
cast between incompatible function types from ‘GIOFunc’ {aka ‘int
(*)(struct _GIOChannel *, enum <anonymous>,  void *)’} to ‘gboolean
(*)(void *)’ {aka ‘int (*)(void *)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/chardev/char-io.c:92:20: warning: cast
between incompatible function types from ‘QIOChannelFunc’ {aka ‘int
(*)(struct QIOChannel *, enum <anonymous>,  void *)’} to ‘gboolean
(*)(void *)’ {aka ‘int (*)(void *)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/chardev/char-socket.c:606:42: warning:
cast between incompatible function types from ‘gboolean (*)(QIOChannel
*, GIOCondition,  void *)’ {aka ‘int (*)(struct QIOChannel *, enum
<anonymous>,  void *)’} to ‘gboolean (*)(void *)’ {aka ‘int (*)(void
*)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/contrib/libvhost-user/libvhost-user-glib.c:59:6:
warning: cast between incompatible function types from ‘GSourceFunc’
{aka ‘int (*)(void *)’} to ‘void (*)(VuDev *, int,  void *)’ {aka ‘void
(*)(struct VuDev *, int,  void *)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/contrib/libvhost-user/libvhost-user-glib.c:85:33:
warning: cast between incompatible function types from ‘vu_watch_cb’
{aka ‘void (*)(struct VuDev *, int,  void *)’} to ‘gboolean (*)(void *)’
{aka ‘int (*)(void *)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/io/channel-buffer.c:182:27: warning:
cast between incompatible function types from ‘GSourceFunc’ {aka ‘int
(*)(void *)’} to ‘gboolean (*)(QIOChannel *, GIOCondition,  void *)’
{aka ‘int (*)(struct QIOChannel *, enum <anonymous>,  void *)’}
[-Wcast-function-type]
/home/debian/src/github/qemu/qemu/io/channel.c:315:35: warning: cast
between incompatible function types from ‘QIOChannelFunc’ {aka ‘int
(*)(struct QIOChannel *, enum <anonymous>,  void *)’} to ‘gboolean
(*)(void *)’ {aka ‘int (*)(void *)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/io/channel.c:507:27: warning: cast
between incompatible function types from ‘gboolean (*)(QIOChannel *,
GIOCondition,  void *)’ {aka ‘int (*)(struct QIOChannel *, enum
<anonymous>,  void *)’} to ‘gboolean (*)(void *)’ {aka ‘int (*)(void
*)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/io/channel-watch.c:216:27: warning:
cast between incompatible function types from ‘GSourceFunc’ {aka ‘int
(*)(void *)’} to ‘gboolean (*)(QIOChannel *, GIOCondition,  void *)’
{aka ‘int (*)(struct QIOChannel *, enum <anonymous>,  void *)’}
[-Wcast-function-type]
/home/debian/src/github/qemu/qemu/io/channel-watch.c:81:27: warning:
cast between incompatible function types from ‘GSourceFunc’ {aka ‘int
(*)(void *)’} to ‘gboolean (*)(QIOChannel *, GIOCondition,  void *)’
{aka ‘int (*)(struct QIOChannel *, enum <anonymous>,  void *)’}
[-Wcast-function-type]
/home/debian/src/github/qemu/qemu/io/channel-websock.c:1258:27: warning:
cast between incompatible function types from ‘GSourceFunc’ {aka ‘int
(*)(void *)’} to ‘gboolean (*)(QIOChannel *, GIOCondition,  void *)’
{aka ‘int (*)(struct QIOChannel *, enum <anonymous>,  void *)’}
[-Wcast-function-type]
/home/debian/src/github/qemu/qemu/io/net-listener.c:236:31: warning:
cast between incompatible function types from ‘gboolean (*)(QIOChannel
*, GIOCondition,  void *)’ {aka ‘int (*)(struct QIOChannel *, enum
<anonymous>,  void *)’} to ‘gboolean (*)(void *)’ {aka ‘int (*)(void
*)’} [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/target/i386/translate.c:2850:16:
warning: cast between incompatible function types from ‘void (*)(struct
TCGv_ptr_d *, struct TCGv_ptr_d *, struct TCGv_ptr_d *, struct
TCGv_i32_d *)’ to ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *,
struct TCGv_ptr_d *)’ [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/target/i386/translate.c:2850:16:
warning: cast between incompatible function types from ‘void (*)(struct
TCGv_ptr_d *, struct TCGv_ptr_d *, struct TCGv_ptr_d *, struct
TCGv_i64_d *)’ to ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *,
struct TCGv_ptr_d *)’ [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/target/i386/translate.c:2851:16:
warning: cast between incompatible function types from ‘void (*)(struct
TCGv_ptr_d *, struct TCGv_ptr_d *, struct TCGv_ptr_d *, struct
TCGv_i32_d *)’ to ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *,
struct TCGv_ptr_d *)’ [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/target/i386/translate.c:2851:16:
warning: cast between incompatible function types from ‘void (*)(struct
TCGv_ptr_d *, struct TCGv_ptr_d *, struct TCGv_ptr_d *, struct
TCGv_i64_d *)’ to ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *,
struct TCGv_ptr_d *)’ [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/target/i386/translate.c:4469:27:
warning: cast between incompatible function types from ‘SSEFunc_0_epp’
{aka ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *, struct
TCGv_ptr_d *)’} to ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *,
struct TCGv_ptr_d *, struct TCGv_i32_d *)’ [-Wcast-function-type]
/home/debian/src/github/qemu/qemu/target/i386/translate.c:4469:27:
warning: cast between incompatible function types from ‘SSEFunc_0_epp’
{aka ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *, struct
TCGv_ptr_d *)’} to ‘void (*)(struct TCGv_ptr_d *, struct TCGv_ptr_d *,
struct TCGv_ptr_d *, struct TCGv_i64_d *)’ [-Wcast-function-type]

Daniel, here is one possible fix (I am not sure how important that is):

diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
index 96f186f442..074cece468 100644
--- a/crypto/hash-nettle.c
+++ b/crypto/hash-nettle.c
@@ -28,10 +28,10 @@
 
 typedef void (*qcrypto_nettle_init)(void *ctx);
 typedef void (*qcrypto_nettle_write)(void *ctx,
-                                     unsigned int len,
+                                     size_t len,
                                      const uint8_t *buf);
 typedef void (*qcrypto_nettle_result)(void *ctx,
-                                      unsigned int len,
+                                      size_t len,
                                       uint8_t *buf);

Regards
Stefan
Daniel P. Berrangé July 8, 2019, 12:08 p.m. UTC | #7 
On Mon, Jul 08, 2019 at 02:04:27PM +0200, Stefan Weil wrote:
> Am 08.07.2019 um 10:14 schrieb Aleksandar Markovic:
> >
> > On Jul 7, 2019 10:26 PM, "Stefan Weil" <sw@weilnetz.de
> > <mailto:sw@weilnetz.de>> wrote:
> > > - There is also fallthrough code which is obviously not correct
> > (even in target/mips/translate.c).
> >
> > Can you please be more specific about those cases from
> > target/mips/translate.c?
> >
> 
> Daniel, here is one possible fix (I am not sure how important that is):
> 
> diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
> index 96f186f442..074cece468 100644
> --- a/crypto/hash-nettle.c
> +++ b/crypto/hash-nettle.c
> @@ -28,10 +28,10 @@
>  
>  typedef void (*qcrypto_nettle_init)(void *ctx);
>  typedef void (*qcrypto_nettle_write)(void *ctx,
> -                                     unsigned int len,
> +                                     size_t len,
>                                       const uint8_t *buf);
>  typedef void (*qcrypto_nettle_result)(void *ctx,
> -                                      unsigned int len,
> +                                      size_t len,
>                                        uint8_t *buf);

This is a case of nettle changing its API contract in version 3.

We dealt with it in cipher-nettle.c already by creating a cipher_length_t
type. need to copy this solution into the hash code, unless perhaps we
can drop older nettle per out platform support matrix.

Regards,
Daniel
Aleksandar Markovic July 8, 2019, 7:39 p.m. UTC | #8 
> ...this is the list of warnings for target/mips/translate.c:

> /home/debian/src/github/qemu/qemu/target/mips/translate.c:10047:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
> /home/debian/src/github/qemu/qemu/target/mips/translate.c:10056:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
> /home/debian/src/github/qemu/qemu/target/mips/translate.c:20138:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
> /home/debian/src/github/qemu/qemu/target/mips/translate.c:20144:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
> /home/debian/src/github/qemu/qemu/target/mips/translate.c:6739:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
> /home/debian/src/github/qemu/qemu/target/mips/translate.c:9820:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
> /home/debian/src/github/qemu/qemu/target/mips/translate.c:9829:13: warning: this statement may fall through [-Wimplicit-fallthrough=]

They are all real issues. Two of them are cases of missing '/* fall through */' (I plan to send fixes for them in 4.2 timeframe) and five of them are cases of missing 'break' (I plan to send corresponding fixes for 4.1 in few days).

Last time I checked gcc 'implicit-fallthrough' option was around five months ago, and meanwhile some new code with missing annotation sneaked in. However, there is some news - it appears to me that gcc 8 improved that feature significantly compared to gcc 7: some cases now detected by gcc 8 are simply went undetected by gcc 7. It appears that at least some of such cases are ignored by Coverity too.

Great info! Thanks again!!

Aleksandar




> Stefan
Markus Armbruster July 9, 2019, 5:40 a.m. UTC | #9 
Stefan Weil <sw@weilnetz.de> writes:

> Am 08.07.19 um 06:40 schrieb Markus Armbruster:
>
[...]
>> However, the gnu_ in gnu_scanf tells the compiler we're linking with the
>> GNU C Library, which seems unwise.  Hmm, we already use gnu_printf.
>> Commit 9c9e7d51bf0:
>>
>>      Newer gcc versions support format gnu_printf which is
>>      better suited for use in QEMU than format printf
>>      (QEMU always uses standard format strings (even with mingw32)).
>>
>> Should we limit the use of gnu_printf to #ifdef _WIN32?
>
>
> No, because we don't want lots of conditional code with different
> format strings for POSIX and Windows (I made that commit 9 years ago).

I'm afraid I failed to express myself clearly.

I'm not proposing to conditionally use MS conversion specifications
instead of ISO C ones.  That's mess we can do without indeed.

The documentation of gnu_printf vs. plain printf format attribute in
"The GNU Compiler Collection" made me expect gnu_printf accepts
extensions over ISO C provided by glibc, while plain printf rejects
them.  Since we don't require glibc, catching use of extensions would be
useful, even if we still had to use gnu_printf with MinGW.  However, it
appears not to be the case (I tried on Fedora 30).

[...]
Markus Armbruster July 9, 2019, 5:42 a.m. UTC | #10 
Peter Maydell <peter.maydell@linaro.org> writes:

> On Sun, 7 Jul 2019 at 21:26, Stefan Weil <sw@weilnetz.de> wrote:
>> This is a general problem all over the QEMU code. I usually compile with
>> nearly all warnings enabled and get now lots of errors with the latest
>> code and after updating to gcc-8.3.0 (Debian buster). It should be
>> reproducible by enabling -Werror=implicit-fallthrough.
>
> Coverity warns about implicit fallthroughs, and we are
> currently warning-free in that department, so I think
> our remaining problems are largely down to perhaps
> using idioms which the compiler doesn't spot.
> Being able to enable gcc implicit-fallthrough errors would
> definitely be better than spotting them only after the
> fact with Coverity.
>
>> I suggest to enable -Werror=implicit-fallthrough by default and add a
>> new macro to mark all fallthrough locations which are correct, but not
>> accepted by the compiler.
>
> I'm not sure why we need a macro. Our standard way to
> mark fallthrough is /* fall through */, which has hundreds
> of uses in the codebase. -Wimplicit-fallthrough=2 will match this,
> so it seems simpler to just use that rather than to rework
> how we mark fallthroughs.
>
> Since vixl is 3rd-party code it might be easier to just
> add -Wno-implicit-fallthrough to the cflags that
> disas/libvixl/Makefile.objs sets up for building those files.
> (We should check also for newer libvixl and/or suggest
> something upstream that works with gcc.)

Concur.
Peter Maydell July 9, 2019, 8:25 a.m. UTC | #11 
On Mon, 8 Jul 2019 at 20:39, Aleksandar Markovic <amarkovic@wavecomp.com> wrote:
> They are all real issues. Two of them are cases of missing
> '/* fall through */' (I plan to send fixes for them in 4.2 timeframe)
> and five of them are cases of missing 'break' (I plan to send
> corresponding fixes for 4.1 in few days).

Adding missing /* fall through */ comments would be fine as
a patch for 4.1 rc1 if you liked, though you can of course
wait til 4.2 if that's better for you.

thanks
-- PMM
Stefan Weil July 21, 2019, 4:39 p.m. UTC | #12 
Am 09.07.2019 um 10:25 schrieb Peter Maydell:
> On Mon, 8 Jul 2019 at 20:39, Aleksandar Markovic <amarkovic@wavecomp.com> wrote:
>> They are all real issues. Two of them are cases of missing
>> '/* fall through */' (I plan to send fixes for them in 4.2 timeframe)
>> and five of them are cases of missing 'break' (I plan to send
>> corresponding fixes for 4.1 in few days).
> Adding missing /* fall through */ comments would be fine as
> a patch for 4.1 rc1 if you liked, though you can of course
> wait til 4.2 if that's better for you.
>
> thanks
> -- PMM


Peter, is this fall through for ARM correct?

https://github.com/qemu/qemu/blob/master/target/arm/helper.c#L7958

It looks rather suspicious and is one of the remaining related compiler
warnings.

Regards
Stefan
Peter Maydell July 22, 2019, 9:09 a.m. UTC | #13 
On Sun, 21 Jul 2019 at 17:39, Stefan Weil <sw@weilnetz.de> wrote:
> Peter, is this fall through for ARM correct?
>
> https://github.com/qemu/qemu/blob/master/target/arm/helper.c#L7958
>
> It looks rather suspicious and is one of the remaining related compiler
> warnings.

It's wrong, and Philippe posted a patch for it a day or two ago:

https://patchew.org/QEMU/20190719111451.12406-1-philmd@redhat.com/

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/mips/translate.c b/target/mips/translate.c
index b944ea2..3dd66b6 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -14290,8 +14290,8 @@  static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
         case SDP:
             check_insn(ctx, ISA_MIPS3);
             check_mips_64(ctx);
-            /* Fallthrough */
 #endif
+            /* fall through */
         case LWP:
         case SWP:
             gen_ldst_pair(ctx, minor, rt, rs, SIMM(ctx->opcode, 0, 12));
@@ -14301,8 +14301,8 @@  static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
         case SDM:
             check_insn(ctx, ISA_MIPS3);
             check_mips_64(ctx);
-            /* Fallthrough */
 #endif
+            /* fall through */
         case LWM32:
         case SWM32:
             gen_ldst_multiple(ctx, minor, rt, rs, SIMM(ctx->opcode, 0, 12));
@@ -20087,6 +20087,7 @@  static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         case OPC_MTHC1:
             check_cp1_enabled(ctx);
             check_insn(ctx, ISA_MIPS32R2);
+            /* fall through */
         case OPC_MFC1:
         case OPC_CFC1:
         case OPC_MTC1: