Discussion:
[PATCH] regexec: fix unintentinal fallthrough warning with GCC7
(too old to reply)
Andrei Borzenkov
2017-03-23 16:16:27 UTC
Permalink
Raw Message
[ 104s] In file included from ../../grub-core/gnulib/regex.c:72:0:
[ 104s] ../../grub-core/gnulib/regexec.c: In function 'check_node_accept':
[ 104s] ../../grub-core/gnulib/regexec.c:4100:10: error: this statement may fall through [-Werror=implicit-fallthrough=]
[ 104s] if (ch >= ASCII_CHARS)
[ 104s] ^
[ 104s] ../../grub-core/gnulib/regexec.c:4104:5: note: here
[ 104s] case OP_PERIOD:
[ 104s] ^~~~

The code in question does have FALLTHROUGH annotation; unfortunately GCC7
ignores it, because it is separated from case statement by #endif.

The patch fixes it by using new attribute that is honored even inside #ifdef'ed
code.

---
lib/regexec.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/lib/regexec.c b/lib/regexec.c
index ef52b24..4588e13 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -4078,6 +4078,9 @@ check_node_accept (const re_match_context_t *mctx, const re_token_t *node,
case OP_UTF8_PERIOD:
if (ch >= ASCII_CHARS)
return false;
+#if defined __GNUC__ && __GNUC__ >= 7
+ __attribute__ ((fallthrough));
+#endif
/* FALLTHROUGH */
#endif
case OP_PERIOD:
--
tg: (bd78ca3..) u/regexec-gcc7-fallthrough (depends on: master)
Eric Blake
2017-03-23 16:21:35 UTC
Permalink
Raw Message
Post by Andrei Borzenkov
[ 104s] ../../grub-core/gnulib/regexec.c:4100:10: error: this statement may fall through [-Werror=implicit-fallthrough=]
[ 104s] if (ch >= ASCII_CHARS)
[ 104s] ^
[ 104s] ../../grub-core/gnulib/regexec.c:4104:5: note: here
[ 104s] ^~~~
The code in question does have FALLTHROUGH annotation; unfortunately GCC7
ignores it, because it is separated from case statement by #endif.
The patch fixes it by using new attribute that is honored even inside #ifdef'ed
code.
---
lib/regexec.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/regexec.c b/lib/regexec.c
index ef52b24..4588e13 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -4078,6 +4078,9 @@ check_node_accept (const re_match_context_t *mctx, const re_token_t *node,
if (ch >= ASCII_CHARS)
return false;
+#if defined __GNUC__ && __GNUC__ >= 7
+ __attribute__ ((fallthrough));
+#endif
/* FALLTHROUGH */
#endif
Would it also work if you did:

case OP_UTF8_PERIOD:
if (ch >= ASCII_CHARS)
return false;
#endif
/* FALLTHROUGH */
case OP_PERIOD:

(that is, swap the #endif and FALLTHROUGH lines)?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Andrei Borzenkov
2017-03-23 16:27:24 UTC
Permalink
Raw Message
Post by Andrei Borzenkov
Post by Andrei Borzenkov
[ 104s] ../../grub-core/gnulib/regexec.c:4100:10: error: this statement may fall through [-Werror=implicit-fallthrough=]
[ 104s] if (ch >= ASCII_CHARS)
[ 104s] ^
[ 104s] ../../grub-core/gnulib/regexec.c:4104:5: note: here
[ 104s] ^~~~
The code in question does have FALLTHROUGH annotation; unfortunately GCC7
ignores it, because it is separated from case statement by #endif.
The patch fixes it by using new attribute that is honored even inside #ifdef'ed
code.
---
lib/regexec.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/regexec.c b/lib/regexec.c
index ef52b24..4588e13 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -4078,6 +4078,9 @@ check_node_accept (const re_match_context_t *mctx, const re_token_t *node,
if (ch >= ASCII_CHARS)
return false;
+#if defined __GNUC__ && __GNUC__ >= 7
+ __attribute__ ((fallthrough));
+#endif
/* FALLTHROUGH */
#endif
if (ch >= ASCII_CHARS)
return false;
#endif
/* FALLTHROUGH */
(that is, swap the #endif and FALLTHROUGH lines)?
The result is wrong when code is ifdef'ed out, because preceding case
label must *not* fall through into this one.

case SIMPLE_BRACKET:
if (!bitset_contain (node->opr.sbcset, ch))
return false;
break;

#ifdef RE_ENABLE_I18N
case OP_UTF8_PERIOD:
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
#endif
case OP_PERIOD:

If RE_ENABLE_I18N is not defined, FALLTHROUGH will be attributed to
wrong branch. If someone happens to add case label before #ifdef, it
will be rather hard to spot.

I thought about

#ifdef RE_ENABLE_I18N
case OP_UTF8_PERIOD:
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
case OP_PERIOD:
#else
case OP_PERIOD:
#endif


But it just looks ugly (and I did not test it).
Bernhard Voelker
2017-03-23 16:57:18 UTC
Permalink
Raw Message
Post by Andrei Borzenkov
If RE_ENABLE_I18N is not defined, FALLTHROUGH will be attributed to
wrong branch. If someone happens to add case label before #ifdef, it
will be rather hard to spot.
What about this?

diff --git a/lib/regexec.c b/lib/regexec.c
index ef52b243a..107a5c035 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -4079,7 +4079,10 @@ check_node_accept (const re_match_context_t *mctx, const re_token_t *node,
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
+ goto FALLTHRU_TO_OP_PERIOD;
#endif
+
+FALLTHRU_TO_OP_PERIOD:
case OP_PERIOD:
if ((ch == '\n' && !(mctx->dfa->syntax & RE_DOT_NEWLINE))
|| (ch == '\0' && (mctx->dfa->syntax & RE_DOT_NOT_NULL)))


Okay, some consider goto as ugly ... but it is explicit and
probably efficient as well in this case. ;-)

Have a nice day,
Berny
Eric Blake
2017-03-23 17:10:44 UTC
Permalink
Raw Message
Post by Bernhard Voelker
Post by Andrei Borzenkov
If RE_ENABLE_I18N is not defined, FALLTHROUGH will be attributed to
wrong branch. If someone happens to add case label before #ifdef, it
will be rather hard to spot.
What about this?
diff --git a/lib/regexec.c b/lib/regexec.c
index ef52b243a..107a5c035 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -4079,7 +4079,10 @@ check_node_accept (const re_match_context_t *mctx, const re_token_t *node,
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
+ goto FALLTHRU_TO_OP_PERIOD;
#endif
+
if ((ch == '\n' && !(mctx->dfa->syntax & RE_DOT_NEWLINE))
|| (ch == '\0' && (mctx->dfa->syntax & RE_DOT_NOT_NULL)))
Okay, some consider goto as ugly ... but it is explicit and
probably efficient as well in this case. ;-)
Then you get a warning about an unused label. But how about:

#ifdef RE_ENABLE_I18N
case OP_UTF8_PERIOD:
if (ch >= ASCII_CHARS)
return false;
goto FALLTHROUGH_TO_OP_PERIOD;
FALLTHROUGH_TO_OP_PERIOD:
#endif

case OP_PERIOD:
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Andrei Borzenkov
2017-03-23 17:21:56 UTC
Permalink
Raw Message
Post by Andrei Borzenkov
#ifdef RE_ENABLE_I18N
if (ch >= ASCII_CHARS)
return false;
goto FALLTHROUGH_TO_OP_PERIOD;
#endif
Yes, works too. Although it probably needs comment explaining what happens.

Although this looks like some more intelligence on GCC side may cause it
to fail again (as we finally do not leave case label at all, so we fall
through ...)

Andrei Borzenkov
2017-03-23 17:11:05 UTC
Permalink
Raw Message
Post by Bernhard Voelker
Post by Andrei Borzenkov
If RE_ENABLE_I18N is not defined, FALLTHROUGH will be attributed to
wrong branch. If someone happens to add case label before #ifdef, it
will be rather hard to spot.
What about this?
diff --git a/lib/regexec.c b/lib/regexec.c
index ef52b243a..107a5c035 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -4079,7 +4079,10 @@ check_node_accept (const re_match_context_t *mctx, const re_token_t *node,
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
+ goto FALLTHRU_TO_OP_PERIOD;
#endif
+
if ((ch == '\n' && !(mctx->dfa->syntax & RE_DOT_NEWLINE))
|| (ch == '\0' && (mctx->dfa->syntax & RE_DOT_NOT_NULL)))
Okay, some consider goto as ugly ... but it is explicit and
probably efficient as well in this case. ;-)
To my surprise, it works :)

I do not care, really. Whatever is decided and accepted by gnulib
maintainers, as long as it is fixed in gnulib.
Eric Blake
2017-03-23 17:08:42 UTC
Permalink
Raw Message
Post by Andrei Borzenkov
Post by Eric Blake
(that is, swap the #endif and FALLTHROUGH lines)?
The result is wrong when code is ifdef'ed out, because preceding case
label must *not* fall through into this one.
Rather than "must not", it "does not" fall through. But does gcc warn
if the magic /* FALLTHROUGH */ comment is present even when it is not
utilized?
Post by Andrei Borzenkov
if (!bitset_contain (node->opr.sbcset, ch))
return false;
break;
#ifdef RE_ENABLE_I18N
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
#endif
If RE_ENABLE_I18N is not defined, FALLTHROUGH will be attributed to
wrong branch.
Yes, but that should be harmless.
Post by Andrei Borzenkov
If someone happens to add case label before #ifdef, it
will be rather hard to spot.
Isn't that what code review is for?
Post by Andrei Borzenkov
I thought about
#ifdef RE_ENABLE_I18N
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
#else
#endif
But it just looks ugly (and I did not test it).
Indeed too ugly to consider.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Andrei Borzenkov
2017-03-23 17:13:57 UTC
Permalink
Raw Message
Post by Eric Blake
Post by Andrei Borzenkov
Post by Eric Blake
(that is, swap the #endif and FALLTHROUGH lines)?
The result is wrong when code is ifdef'ed out, because preceding case
label must *not* fall through into this one.
Rather than "must not", it "does not" fall through. But does gcc warn
if the magic /* FALLTHROUGH */ comment is present even when it is not
utilized?
No, this silence the warning.
Post by Eric Blake
Post by Andrei Borzenkov
if (!bitset_contain (node->opr.sbcset, ch))
return false;
break;
#ifdef RE_ENABLE_I18N
if (ch >= ASCII_CHARS)
return false;
/* FALLTHROUGH */
#endif
If RE_ENABLE_I18N is not defined, FALLTHROUGH will be attributed to
wrong branch.
Yes, but that should be harmless.
Post by Andrei Borzenkov
If someone happens to add case label before #ifdef, it
will be rather hard to spot.
Isn't that what code review is for?
Yes in ideal world. I would not do it personally.
Loading...