From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 11 Jul 2019 05:14:52 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B549B308339F; Thu, 11 Jul 2019 12:14:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-231.ams2.redhat.com [10.36.117.231]) by smtp.corp.redhat.com (Postfix) with ESMTP id C2DF360603; Thu, 11 Jul 2019 12:14:50 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpu: Fix GCC7/GCC8 build failure From: "Laszlo Ersek" To: Ray Ni , devel@edk2.groups.io Cc: Michael D Kinney , Eric Dong References: <20190711011638.95268-1-ray.ni@intel.com> Message-ID: <0ed76e95-ab20-1673-8732-a5a4363a89eb@redhat.com> Date: Thu, 11 Jul 2019 14:14:49 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Thu, 11 Jul 2019 12:14:51 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/11/19 12:02, Laszlo Ersek wrote: > Hello Ray, > > On 07/11/19 03:16, Ray Ni wrote: >> Signed-off-by: Ray Ni >> Cc: Michael D Kinney >> Cc: Eric Dong >> Cc: Laszlo Ersek >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> index c31160735a..a3b62f7787 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> @@ -571,7 +571,7 @@ ReclaimPages ( >> // >> // First, find the leaf entry has the smallest access record value >> // >> - for (Pml5Index = 0; Pml5Index < Enable5LevelPaging ? (EFI_PAGE_SIZE / sizeof (*Pml4)) : 1; Pml5Index++) { >> + for (Pml5Index = 0; Pml5Index < (Enable5LevelPaging ? (EFI_PAGE_SIZE / sizeof (*Pml4)) : 1); Pml5Index++) { >> if ((Pml5[Pml5Index] & IA32_PG_P) == 0 || (Pml5[Pml5Index] & IA32_PG_PMNT) != 0) { >> // >> // If the PML5 entry is not present or is masked, skip it >> > > This is not a GCC7/GCC8 build failure, but a genuine bug in the code > that GCC7/GCC8 helpfully reported. > > The conditional operator ?: has weaker precedence than the relational > operator <, and so the patch incurs a behavioral change -- thus, it is a > bugfix. > > If we were only adding the parentheses to reinforce the operator > bingings that are already in place, i.e., preserving the behavior, > *then* we could call this a "build failure". > > > (1) Please update the subject accordingly, for example: > > UefiCpuPkg/PiSmmCpuDxeSmm: ReclaimPages: fix incorrect operator binding > > (71 characters) > > > (2) I'd suggest also adding, to the commit message body: > > Fixes: 7365eb2c8cf1d7112330d09918c0c67e8d0b827a > > With those: > > Reviewed-by: Laszlo Ersek IMPORTANT: please do not push this patch before you revert and reapply 7365eb2c8cf1; otherwise we'll get into a huge mess. Please see here: https://edk2.groups.io/g/devel/message/43599 (Namely, if you apply this patch on top of 7365eb2c8cf1, then you won't be able to revert 7365eb2c8cf1, which is a pre-requisite for reverting 7c5010c7f88b.) Thanks Laszlo