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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
