Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Fix GCC7/GCC8 build failure
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43601): https://edk2.groups.io/g/devel/message/43601 Mute This Topic: https://groups.io/mt/32422914/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Fix GCC7/GCC8 build failure
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 Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43595): https://edk2.groups.io/g/devel/message/43595 Mute This Topic: https://groups.io/mt/32422914/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Fix GCC7/GCC8 build failure
Reviewed-by: Eric Dong > -Original Message- > From: Ni, Ray > Sent: Thursday, July 11, 2019 9:17 AM > To: devel@edk2.groups.io > Cc: Kinney, Michael D ; Dong, Eric > ; Laszlo Ersek > Subject: [PATCH] UefiCpuPkg/PiSmmCpu: Fix GCC7/GCC8 build failure > > 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 > -- > 2.21.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43547): https://edk2.groups.io/g/devel/message/43547 Mute This Topic: https://groups.io/mt/32422914/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Fix GCC7/GCC8 build failure
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 -- 2.21.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43533): https://edk2.groups.io/g/devel/message/43533 Mute This Topic: https://groups.io/mt/32422914/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-