diff mbox series

tcg/optimize: Add fallthrough annotations

Message ID 20201029122833.195420-1-thuth@redhat.com
State New
Headers show
Series tcg/optimize: Add fallthrough annotations | expand

Commit Message

Thomas Huth Oct. 29, 2020, 12:28 p.m. UTC
To be able to compile this file with -Werror=implicit-fallthrough,
we need to add some fallthrough annotations to the case statements
that might fall through. Unfortunately, the typical "/* fallthrough */"
comments do not work here as expected since some case labels are
wrapped in macros and the compiler fails to match the comments in
this case. But using __attribute__((fallthrough)) seems to work fine,
so let's use that instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tcg/optimize.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Philippe Mathieu-Daudé Oct. 29, 2020, 12:37 p.m. UTC | #1
On 10/29/20 1:28 PM, Thomas Huth wrote:
> To be able to compile this file with -Werror=implicit-fallthrough,

> we need to add some fallthrough annotations to the case statements

> that might fall through. Unfortunately, the typical "/* fallthrough */"

> comments do not work here as expected since some case labels are

> wrapped in macros and the compiler fails to match the comments in

> this case. But using __attribute__((fallthrough)) seems to work fine,

> so let's use that instead.

> 

> Signed-off-by: Thomas Huth <thuth@redhat.com>

> ---

>  tcg/optimize.c | 10 ++++++++++

>  1 file changed, 10 insertions(+)

> 

> diff --git a/tcg/optimize.c b/tcg/optimize.c

> index 220f4601d5..c2768c9770 100644

> --- a/tcg/optimize.c

> +++ b/tcg/optimize.c

> @@ -26,6 +26,12 @@

>  #include "qemu/osdep.h"

>  #include "tcg/tcg-op.h"

>  

> +#if __has_attribute(fallthrough)

> +# define fallthrough() __attribute__((fallthrough))

> +#else

> +# define fallthrough() do {} while (0)

> +#endif


This looks something we can reuse, what about adding it
as QEMU_FALLTHROUGH in "qemu/compiler.h"?

> +

>  #define CASE_OP_32_64(x)                        \

>          glue(glue(case INDEX_op_, x), _i32):    \

>          glue(glue(case INDEX_op_, x), _i64)

> @@ -855,6 +861,7 @@ void tcg_optimize(TCGContext *s)

>              if ((arg_info(op->args[1])->mask & 0x80) != 0) {

>                  break;

>              }

> +            fallthrough();

>          CASE_OP_32_64(ext8u):

>              mask = 0xff;

>              goto and_const;

> @@ -862,6 +869,7 @@ void tcg_optimize(TCGContext *s)

>              if ((arg_info(op->args[1])->mask & 0x8000) != 0) {

>                  break;

>              }

> +            fallthrough();

>          CASE_OP_32_64(ext16u):

>              mask = 0xffff;

>              goto and_const;

> @@ -869,6 +877,7 @@ void tcg_optimize(TCGContext *s)

>              if ((arg_info(op->args[1])->mask & 0x80000000) != 0) {

>                  break;

>              }

> +            fallthrough();

>          case INDEX_op_ext32u_i64:

>              mask = 0xffffffffU;

>              goto and_const;

> @@ -886,6 +895,7 @@ void tcg_optimize(TCGContext *s)

>              if ((arg_info(op->args[1])->mask & 0x80000000) != 0) {

>                  break;

>              }

> +            fallthrough();

>          case INDEX_op_extu_i32_i64:

>              /* We do not compute affected as it is a size changing op.  */

>              mask = (uint32_t)arg_info(op->args[1])->mask;

>
Richard Henderson Oct. 29, 2020, 8:07 p.m. UTC | #2
On 10/29/20 5:28 AM, Thomas Huth wrote:
> To be able to compile this file with -Werror=implicit-fallthrough,

> we need to add some fallthrough annotations to the case statements

> that might fall through. Unfortunately, the typical "/* fallthrough */"

> comments do not work here as expected since some case labels are

> wrapped in macros and the compiler fails to match the comments in

> this case. But using __attribute__((fallthrough)) seems to work fine,

> so let's use that instead.


Why would the macro matter?  It expands to two case statements with nothing in
between them.

This sounds like a compiler bug that should be reported.


r~
Chenqun (kuhn) Oct. 30, 2020, 2:43 a.m. UTC | #3
> -----Original Message-----

> From: Richard Henderson [mailto:richard.henderson@linaro.org]

> Sent: Friday, October 30, 2020 4:07 AM

> To: Thomas Huth <thuth@redhat.com>; Richard Henderson <rth@twiddle.net>;

> qemu-devel@nongnu.org

> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>

> Subject: Re: [PATCH] tcg/optimize: Add fallthrough annotations

> 

> On 10/29/20 5:28 AM, Thomas Huth wrote:

> > To be able to compile this file with -Werror=implicit-fallthrough, we

> > need to add some fallthrough annotations to the case statements that

> > might fall through. Unfortunately, the typical "/* fallthrough */"

> > comments do not work here as expected since some case labels are

> > wrapped in macros and the compiler fails to match the comments in this

> > case. But using __attribute__((fallthrough)) seems to work fine, so

> > let's use that instead.

> 

> Why would the macro matter?  It expands to two case statements with

> nothing in between them.

> 

> This sounds like a compiler bug that should be reported.

> 

Hi all,
  I have queried the GCC options description about the Wimplicit-fallthrough and verified it.
The value of Wimplicit-fallthrough ranges from 0 to 5. 
The value 0 is to ignore all warnings, which is certainly not what we need.
If the value is set to 1 or 2, most fall through on the QEMU can take effect.
   Eg:/* FALLTHRU */、/* fallthru */、/* fall-through */、/* FALLTHOUGH */、/* fall through */、/* fallthrough */..

When the value ranges from 3 to 5, more fallthrough comments become invalid as the value increases.

So, I agree with Philippe's suggestion to add a QEMU_FALLTHROUGH to unify this compiler property.

Thanks,
Chen Qun

Additional gcc information is as follows:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
-Wimplicit-fallthrough is the same as -Wimplicit-fallthrough=3 and -Wno-implicit-fallthrough is the same as -Wimplicit-fallthrough=0.

The option argument n specifies what kind of comments are accepted:
-Wimplicit-fallthrough=0 disables the warning altogether.
-Wimplicit-fallthrough=1 matches .* regular expression, any comment is used as fallthrough comment.
-Wimplicit-fallthrough=2 case insensitively matches .*falls?[ \t-]*thr(ough|u).* regular expression.
-Wimplicit-fallthrough=3 case sensitively matches one of the following regular expressions:
   -fallthrough
   @fallthrough@
   lint -fallthrough[ \t]*
   [ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?
   FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)?
   [ \t.!]*(Else,? |Intentional(ly)? )?
   Fall((s | |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)?
   [ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )?
   fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?
-Wimplicit-fallthrough=4 case sensitively matches one of the following regular expressions:
    -fallthrough
    @fallthrough@
    lint -fallthrough[ \t]*
    [ \t]*FALLTHR(OUGH|U)[ \t]*
-Wimplicit-fallthrough=5 doesn’t recognize any comments as fallthrough comments, only attributes disable the warning.
diff mbox series

Patch

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 220f4601d5..c2768c9770 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -26,6 +26,12 @@ 
 #include "qemu/osdep.h"
 #include "tcg/tcg-op.h"
 
+#if __has_attribute(fallthrough)
+# define fallthrough() __attribute__((fallthrough))
+#else
+# define fallthrough() do {} while (0)
+#endif
+
 #define CASE_OP_32_64(x)                        \
         glue(glue(case INDEX_op_, x), _i32):    \
         glue(glue(case INDEX_op_, x), _i64)
@@ -855,6 +861,7 @@  void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x80) != 0) {
                 break;
             }
+            fallthrough();
         CASE_OP_32_64(ext8u):
             mask = 0xff;
             goto and_const;
@@ -862,6 +869,7 @@  void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x8000) != 0) {
                 break;
             }
+            fallthrough();
         CASE_OP_32_64(ext16u):
             mask = 0xffff;
             goto and_const;
@@ -869,6 +877,7 @@  void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x80000000) != 0) {
                 break;
             }
+            fallthrough();
         case INDEX_op_ext32u_i64:
             mask = 0xffffffffU;
             goto and_const;
@@ -886,6 +895,7 @@  void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x80000000) != 0) {
                 break;
             }
+            fallthrough();
         case INDEX_op_extu_i32_i64:
             /* We do not compute affected as it is a size changing op.  */
             mask = (uint32_t)arg_info(op->args[1])->mask;