Re: [PATCH] regexec: fix unintentinal fallthrough warning with GCC7

[Andrei Borzenkov]

23.03.2017 19:21, Eric Blake пишет:
> On 03/23/2017 11:16 AM, Andrei Borzenkov wrote:
>> [  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:
> 
> 
> 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)?
> 

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).

signature.asc
Description: OpenPGP digital signature