From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E8B0C21157FDF for ; Mon, 1 Oct 2018 10:48:41 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 27B5F3001763; Mon, 1 Oct 2018 17:48:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-68.rdu2.redhat.com [10.10.123.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4BB05308BDB8; Mon, 1 Oct 2018 17:48:40 +0000 (UTC) To: "Tomas Pilar (tpilar)" , edk2-devel@lists.01.org References: <1538186116-3792-1-git-send-email-dongao.guo@intel.com> <7b490137-3c8a-b5e3-9b48-c4ba1a3d588b@solarflare.com> From: Laszlo Ersek Message-ID: Date: Mon, 1 Oct 2018 19:48:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7b490137-3c8a-b5e3-9b48-c4ba1a3d588b@solarflare.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Mon, 01 Oct 2018 17:48:41 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg:disable wraning to pass gcc4.8 build X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Oct 2018 17:48:42 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >