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

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

[Dongao Guo]

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
-- 
2.6.1.windows.1

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

[Gao, Liming]

Dongao:
  Please remove Change-id in commit message. And, update commit title: MdeModulePkg RegularExpressionDxe: Disable warning to pass gcc4.8 build

Thanks
Liming
> -----Original Message-----
> From: Guo, Dongao
> Sent: Saturday, September 29, 2018 9:55 AM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH] MdeModulePkg:disable wraning to pass gcc4.8 build
> 
> 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
> --
> 2.6.1.windows.1

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

[Zeng, Star]

No Signed-off-by?

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dongao Guo
Sent: Saturday, September 29, 2018 9:55 AM
To: edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: [edk2] [PATCH] MdeModulePkg:disable wraning to pass gcc4.8 build

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.i
+++ nf
@@ -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
--
2.6.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Tomas Pilar (tpilar)]

Wouldn't it be better to fix the warnings in code rather than silencing them?

Tom

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

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

[Laszlo Ersek]

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
>

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

[Laszlo Ersek]

Wait a sec:

On 10/01/18 19:48, Laszlo Ersek wrote:

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

[...]

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

In fact, another improvement is possible: if the issue only affects
gcc-4.8, then:

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

this toolchain glob pattern is too general. It should be:

  GCC:*_GCC48_*_CC_FLAGS

(or maybe spell it out for all GCC versions up to and including 4.8,
from the earliest we support, 4.4.)

Thanks
Laszlo

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

[Gao, Liming]

Laszlo:
   We meet with this failure in GCC4.8 and GCC4.9, but not in GCC5.  We don't verify earlier version than GCC4.8. 
   GCC48 is the specific tool chain for GCC4.8. But, GCC49 is the general tool chain that can be used with GCC4.9 or the above GCC version for LTO disable. So, even if we specify this warning for GCC48 and GCC49, it may be applied for GCC5 version. To be simplified, I suggest to disable this warning in the general style. I suggest to add more comments here to describe this warning for GCC4.8, GCC4.9 version. Not for GCC5. 
  
>>>> +  # Oniguruma: tag_end in parse_callout_of_name
>>>> +  GCC:*_*_*_CC_FLAGS = -Wno-error=maybe-uninitialized

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Tuesday, October 02, 2018 2:02 AM
>To: Tomas Pilar (tpilar) <tpilar@solarflare.com>; edk2-devel@lists.01.org
>Subject: Re: [edk2] [PATCH] MdeModulePkg:disable wraning to pass gcc4.8
>build
>
>Wait a sec:
>
>On 10/01/18 19:48, Laszlo Ersek wrote:
>
>> 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.
>
>[...]
>
>> So, I think I agree with this patch, but the commit message should be a
>> *lot* more detailed. (Basically, include the analysis from above.)
>
>In fact, another improvement is possible: if the issue only affects
>gcc-4.8, then:
>
>>> 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.in
>f
>b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.in
>f
>>>> index 16e91bd..07bc02e 100644
>>>> ---
>a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.in
>f
>>>> +++
>b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.in
>f
>>>> @@ -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
>
>this toolchain glob pattern is too general. It should be:
>
>  GCC:*_GCC48_*_CC_FLAGS
>
>(or maybe spell it out for all GCC versions up to and including 4.8,
>from the earliest we support, 4.4.)
>
>Thanks
>Laszlo
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 10/08/18 05:04, Gao, Liming wrote:
> Laszlo:
>    We meet with this failure in GCC4.8 and GCC4.9, but not in GCC5.  We don't verify earlier version than GCC4.8. 
>    GCC48 is the specific tool chain for GCC4.8. But, GCC49 is the general tool chain that can be used with GCC4.9 or the above GCC version for LTO disable. So, even if we specify this warning for GCC48 and GCC49, it may be applied for GCC5 version. To be simplified, I suggest to disable this warning in the general style. I suggest to add more comments here to describe this warning for GCC4.8, GCC4.9 version. Not for GCC5. 

OK, thank you.
Laszlo