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

[Laszlo Ersek <lersek@redhat.com>]

Hi Dongao,

On 10/10/18 04:42, Dongao Guo wrote:
> There are three warnings reported by GCC 4.8 and the later GCC release
> are workaround with them.
> And all the three warnings are invalid,so I just disable warnings rather
> than fix them at now.
> 
> Following is the analysis from Laszlo Ersek.
> (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:

(1) Here, my original email at
<http://mid.mail-archive.com/e4131806-14d5-b11b-5b68-2c552fcadf0c@redhat.com>,
contained another line:

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

This line is not part of your commit message, because "git commit"
considers all lines that start with "#" a comment, and they are removed
from the final commit message.

When quoting content that may contain such lines, it is usually best to
either indent the quote by a few space characters, or else prefix each
line with the string "> ", or something similar.

> 
> 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.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dongao Guo <dongao.guo@intel.com>
> ---
>  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

(2) Based on the analysis now included in the commit message, I suggest:
- either dropping this comment (because it is incomplete),
- or including all three cases briefly.

> +  GCC:*_*_*_CC_FLAGS = -Wno-error=maybe-uninitialized
> 

In my opinion, (1) should be fixed, because otherwise the commit message
is confusing.

Regarding (2), I don't feel strongly, so if you disagree, I don't mind.

Thanks!
Laszlo