Re: [PATCH] MdeModulePkg:disable wraning to pass gcc4.8 build

[Laszlo Ersek <lersek@redhat.com>]

On 10/01/18 11:23, Tomas Pilar (tpilar) wrote:
> Wouldn't it be better to fix the warnings in code rather than
> silencing them?

The same idea occurred to me, but it really depends on upstream
Oniguruma. The upstream project's issue tracker on github seems quite
active:

  https://github.com/kkos/oniguruma/issues

so we could reasonably file a bug report there (and/or submit a pull
request).

However, this (edk2) commit message seems to suggest that the warnings
are only present with gcc-4.8 (and no later gcc releases). This in turn
suggests that the warnings are spurious (presumably, later gcc releases
recognize that none of the affected variables are actually read without
a prior initialization or assignment.)

Hence, upstream Oniguruma might argue that we should fix our compiler --
they might consider gcc-4.8 too old to care -- rather than litter their
code with superfluous assignments, just to pacify gcc-4.8.

(Obviously, if any one of those warnings is valid, then we *must* fix it
in upstream Oniguruma first, and backport the fix.)


... For reference, I've now built RegularExpressionDxe as follows (using
gcc-4.8.5-28.el7_5.1.x86_64):

  build -t GCC48 -b DEBUG -a X64 \
    -p MdeModulePkg/MdeModulePkg.dsc \
    -m MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf

and I get the following 3 "maybe-uninitialized" warnings (currently:
errors):

(1)

> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c: In
> function 'compile_length_tree':
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c:1516:7:
> warning: 'len' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>    int len;
>        ^

I think this is an invalid warning; the type of the controlling
expression (node->type) is enum GimmickType, and the case labels cover
all values of the enum. An assert(0) could be added, I guess, but again,
upstream Oniguruma would be justified to reject the idea.

(2)

> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c: In
> function 'parse_callout_args.isra.10.constprop.30':
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c:6753:25:
> warning: 'rl' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>                vals[n].l = rl;
>                          ^

This warning is invalid, given:

  6749            if (cn > 0) {
  6750              long rl;
  6751              r = parse_long(enc, buf, bufend, 1, LONG_MAX, &rl);
  6752              if (r == ONIG_NORMAL) {
  6753                vals[n].l = rl;

Because parse_long() only returns ONIG_NORMAL after it sets (*rl).

(3)

> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c: In
> function 'parse_callout_of_name.constprop.29':
> MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c:6861:38:
> warning: 'tag_end' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>      if (! is_allowed_callout_tag_name(enc, tag_start, tag_end))
                                      ^

This warning is also invalid, given:

  6852    if (c == '[') {
  6853      if (PEND) return ONIGERR_END_PATTERN_IN_GROUP;
  6854      tag_start = p;
  6855      while (! PEND) {
  6856        if (PEND) return ONIGERR_END_PATTERN_IN_GROUP;
  6857        tag_end = p;
  6858        PFETCH_S(c);
  6859        if (c == ']') break;
  6860      }
  6861      if (! is_allowed_callout_tag_name(enc, tag_start, tag_end))
  6862        return ONIGERR_INVALID_CALLOUT_TAG_NAME;
  6863

To see that, first we should note:

#define PEND         (p < end ?  0 : 1)

therefore PEND doesn't change if neither "p" nor "end" change.

Second, when we reach line 6855 (the "while") for the very first time,
(!PEND) is certainly true (i.e., PEND is false), because otherwise we
would have bailed at line 6853. Therefore we reach line 6857, and assign
"tag_end". Regardless of whether we iterate zero or more *additional*
times around the loop, "tag_end" will have been set, whenever we reach
line 6861.

So, I think I agree with this patch, but the commit message should be a
*lot* more detailed. (Basically, include the analysis from above.)

Thanks
Laszlo


> On 29/09/18 02:55, Dongao Guo wrote:
>> Change-Id: I782962e4994a8edf14beb7ede8b1aabe233dc3a8
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> ---
>>  MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
>> index 16e91bd..07bc02e 100644
>> --- a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
>> +++ b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.inf
>> @@ -109,3 +109,6 @@
>>
>>    # Oniguruma: error: variable 'fp' set but not used
>>    GCC:*_*_*_CC_FLAGS = -Wno-error=unused-but-set-variable
>> +
>> +  # Oniguruma: tag_end in parse_callout_of_name
>> +  GCC:*_*_*_CC_FLAGS = -Wno-error=maybe-uninitialized
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>