* Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
2020-07-21 12:50 [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib Leif Lindholm
@ 2020-07-21 13:18 ` PierreGondois
2020-07-22 15:24 ` Leif Lindholm
2020-07-21 14:26 ` Bob Feng
2020-07-22 20:34 ` Laszlo Ersek
2 siblings, 1 reply; 7+ messages in thread
From: PierreGondois @ 2020-07-21 13:18 UTC (permalink / raw)
To: Leif Lindholm, devel@edk2.groups.io; +Cc: Laszlo Ersek, Bob Feng
Hello Leif,
I actually submitted this modification on the build flags because of the error that you fixed. I submitted a similar modification here https://edk2.groups.io/g/devel/message/61832 . I should have waited before modifying the flags, my apologizes.
I tried building edk2 top-level packages and this should be the only impact.
Regards,
Pierre
-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com>
Sent: 21 July 2020 13:51
To: devel@edk2.groups.io
Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Laszlo Ersek <lersek@redhat.com>; Bob Feng <bob.c.feng@intel.com>
Subject: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
Commit dbd546a32d5a
("BaseTools: Add gcc flag to warn on void* pointer arithmetic") does its work and triggers build errors in this library.
Update the affected code to build correctly again.
Cc: Pierre Gondois <pierre.gondois@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Bob Feng<bob.c.feng@intel.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
Pierre - can you please ensure to CC Arm maintainers when proposing changes to Arm build flags? (And build test all the top-level edk2 packages *cough*.)
Bob - can you please ensure Arm maintainers have commented on changes to global build flags?
(Would it be possible to break up tools_def.template into separate arch-specific include files so we could have GetMaintainer.py be more helpful for this?)
Laszlo - you're not formally an EmbeddedPkg reviewer, but Ard is out for another couple of weeks. But since the Linaro CI is currently broken and the fix is trivial, could you have a look please?
EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index e1036954ee58..15b5bf451330 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
*KernelSize = Header->KernelSize;
- *Kernel = BootImg + Header->PageSize;
+ *Kernel = (VOID *)((UINTN)BootImg + Header->PageSize);
return EFI_SUCCESS;
}
@@ -341,7 +341,7 @@ AndroidBootImgUpdateFdt (
Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
"linux,initrd-end",
- (UINTN)(RamdiskData + RamdiskSize));
+ ((UINTN)RamdiskData +
+ RamdiskSize));
if (EFI_ERROR (Status)) {
goto Fdt_Exit;
}
--
2.20.1
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
2020-07-21 13:18 ` PierreGondois
@ 2020-07-22 15:24 ` Leif Lindholm
0 siblings, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2020-07-22 15:24 UTC (permalink / raw)
To: Pierre Gondois; +Cc: devel@edk2.groups.io, Laszlo Ersek, Bob Feng
On Tue, Jul 21, 2020 at 13:18:02 +0000, Pierre Gondois wrote:
> Hello Leif,
> I actually submitted this modification on the build flags because of
> the error that you fixed. I submitted a similar modification here
> https://edk2.groups.io/g/devel/message/61832 .
Right, sorry, I think I had a micommunication with Ard about who was
looking at that. I'll respond to that separately.
Fwiw I prefer my version of the fix, so if Laszlo could review this
one, I would be most grateful.
> I should have waited before modifying the flags, my apologizes.
Ideally, the change would have been part of the same set - then this
sort of mix-up could not have happened.
The real problem was the being merged without Arm maintainer review.
Regards,
Leif
> I tried building edk2 top-level packages and this should be the only impact.
>
> Regards,
> Pierre
>
> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: 21 July 2020 13:51
> To: devel@edk2.groups.io
> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Laszlo Ersek <lersek@redhat.com>; Bob Feng <bob.c.feng@intel.com>
> Subject: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
>
> Commit dbd546a32d5a
> ("BaseTools: Add gcc flag to warn on void* pointer arithmetic") does its work and triggers build errors in this library.
> Update the affected code to build correctly again.
>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Bob Feng<bob.c.feng@intel.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>
> Pierre - can you please ensure to CC Arm maintainers when proposing changes to Arm build flags? (And build test all the top-level edk2 packages *cough*.)
>
> Bob - can you please ensure Arm maintainers have commented on changes to global build flags?
> (Would it be possible to break up tools_def.template into separate arch-specific include files so we could have GetMaintainer.py be more helpful for this?)
>
> Laszlo - you're not formally an EmbeddedPkg reviewer, but Ard is out for another couple of weeks. But since the Linaro CI is currently broken and the fix is trivial, could you have a look please?
>
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index e1036954ee58..15b5bf451330 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
> ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>
> *KernelSize = Header->KernelSize;
> - *Kernel = BootImg + Header->PageSize;
> + *Kernel = (VOID *)((UINTN)BootImg + Header->PageSize);
> return EFI_SUCCESS;
> }
>
> @@ -341,7 +341,7 @@ AndroidBootImgUpdateFdt (
>
> Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
> "linux,initrd-end",
> - (UINTN)(RamdiskData + RamdiskSize));
> + ((UINTN)RamdiskData +
> + RamdiskSize));
> if (EFI_ERROR (Status)) {
> goto Fdt_Exit;
> }
> --
> 2.20.1
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
2020-07-21 12:50 [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib Leif Lindholm
2020-07-21 13:18 ` PierreGondois
@ 2020-07-21 14:26 ` Bob Feng
2020-07-22 20:34 ` Laszlo Ersek
2 siblings, 0 replies; 7+ messages in thread
From: Bob Feng @ 2020-07-21 14:26 UTC (permalink / raw)
To: Leif Lindholm, devel@edk2.groups.io; +Cc: Pierre Gondois, Laszlo Ersek
Leif,
Sorry about this patch cause some build break.
I'd agree to split the tools_def.template into multiple smaller files that would be helpful for maintenance.
Thanks,
Bob
-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com>
Sent: Tuesday, July 21, 2020 8:51 PM
To: devel@edk2.groups.io
Cc: Pierre Gondois <pierre.gondois@arm.com>; Laszlo Ersek <lersek@redhat.com>; Feng, Bob C <bob.c.feng@intel.com>
Subject: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
Commit dbd546a32d5a
("BaseTools: Add gcc flag to warn on void* pointer arithmetic") does its work and triggers build errors in this library.
Update the affected code to build correctly again.
Cc: Pierre Gondois <pierre.gondois@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Bob Feng<bob.c.feng@intel.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
Pierre - can you please ensure to CC Arm maintainers when proposing changes to Arm build flags? (And build test all the top-level edk2 packages *cough*.)
Bob - can you please ensure Arm maintainers have commented on changes to global build flags?
(Would it be possible to break up tools_def.template into separate arch-specific include files so we could have GetMaintainer.py be more helpful for this?)
Laszlo - you're not formally an EmbeddedPkg reviewer, but Ard is out for another couple of weeks. But since the Linaro CI is currently broken and the fix is trivial, could you have a look please?
EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index e1036954ee58..15b5bf451330 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
*KernelSize = Header->KernelSize;
- *Kernel = BootImg + Header->PageSize;
+ *Kernel = (VOID *)((UINTN)BootImg + Header->PageSize);
return EFI_SUCCESS;
}
@@ -341,7 +341,7 @@ AndroidBootImgUpdateFdt (
Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
"linux,initrd-end",
- (UINTN)(RamdiskData + RamdiskSize));
+ ((UINTN)RamdiskData +
+ RamdiskSize));
if (EFI_ERROR (Status)) {
goto Fdt_Exit;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
2020-07-21 12:50 [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib Leif Lindholm
2020-07-21 13:18 ` PierreGondois
2020-07-21 14:26 ` Bob Feng
@ 2020-07-22 20:34 ` Laszlo Ersek
2020-07-23 10:57 ` Leif Lindholm
2020-07-23 12:08 ` Leif Lindholm
2 siblings, 2 replies; 7+ messages in thread
From: Laszlo Ersek @ 2020-07-22 20:34 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Pierre Gondois, Bob Feng
On 07/21/20 14:50, Leif Lindholm wrote:
> Commit dbd546a32d5a
> ("BaseTools: Add gcc flag to warn on void* pointer arithmetic")
> does its work and triggers build errors in this library.
> Update the affected code to build correctly again.
>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Bob Feng<bob.c.feng@intel.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>
> Pierre - can you please ensure to CC Arm maintainers when proposing
> changes to Arm build flags? (And build test all the top-level edk2
> packages *cough*.)
or we could perhaps introduce
EmbeddedPkg/EmbeddedPkg.ci.yaml
>
> Bob - can you please ensure Arm maintainers have commented on changes to
> global build flags?
> (Would it be possible to break up tools_def.template into separate
> arch-specific include files so we could have GetMaintainer.py be
> more helpful for this?)
>
> Laszlo - you're not formally an EmbeddedPkg reviewer, but Ard is out for
> another couple of weeks. But since the Linaro CI is currently broken and
> the fix is trivial, could you have a look please?
>
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index e1036954ee58..15b5bf451330 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
> ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>
> *KernelSize = Header->KernelSize;
> - *Kernel = BootImg + Header->PageSize;
> + *Kernel = (VOID *)((UINTN)BootImg + Header->PageSize);
> return EFI_SUCCESS;
> }
>
Header->PageSize has type UINT32, so this is OK (the addition is
performed in UINTN, so the conversion back to VOID* is from UINTN).
> @@ -341,7 +341,7 @@ AndroidBootImgUpdateFdt (
>
> Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
> "linux,initrd-end",
> - (UINTN)(RamdiskData + RamdiskSize));
> + ((UINTN)RamdiskData + RamdiskSize));
> if (EFI_ERROR (Status)) {
> goto Fdt_Exit;
> }
>
RamdiskSize is a UINTN, so this is OK too.
(You could even strip the outer parentheses:
(UINTN)RamdiskData + RamdiskSize
)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
2020-07-22 20:34 ` Laszlo Ersek
@ 2020-07-23 10:57 ` Leif Lindholm
2020-07-23 12:08 ` Leif Lindholm
1 sibling, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2020-07-23 10:57 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: devel, Pierre Gondois, Bob Feng
On Wed, Jul 22, 2020 at 22:34:52 +0200, Laszlo Ersek wrote:
> On 07/21/20 14:50, Leif Lindholm wrote:
> > Commit dbd546a32d5a
> > ("BaseTools: Add gcc flag to warn on void* pointer arithmetic")
> > does its work and triggers build errors in this library.
> > Update the affected code to build correctly again.
> >
> > Cc: Pierre Gondois <pierre.gondois@arm.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Bob Feng<bob.c.feng@intel.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >
> > Pierre - can you please ensure to CC Arm maintainers when proposing
> > changes to Arm build flags? (And build test all the top-level edk2
> > packages *cough*.)
>
> or we could perhaps introduce
>
> EmbeddedPkg/EmbeddedPkg.ci.yaml
Ah, do those get picked up automagically?
If so we should definitely do that, as well as for ArmPkg.
> >
> > Bob - can you please ensure Arm maintainers have commented on changes to
> > global build flags?
> > (Would it be possible to break up tools_def.template into separate
> > arch-specific include files so we could have GetMaintainer.py be
> > more helpful for this?)
> >
> > Laszlo - you're not formally an EmbeddedPkg reviewer, but Ard is out for
> > another couple of weeks. But since the Linaro CI is currently broken and
> > the fix is trivial, could you have a look please?
> >
> > EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> > index e1036954ee58..15b5bf451330 100644
> > --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> > +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> > @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
> > ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> >
> > *KernelSize = Header->KernelSize;
> > - *Kernel = BootImg + Header->PageSize;
> > + *Kernel = (VOID *)((UINTN)BootImg + Header->PageSize);
> > return EFI_SUCCESS;
> > }
> >
>
> Header->PageSize has type UINT32, so this is OK (the addition is
> performed in UINTN, so the conversion back to VOID* is from UINTN).
>
> > @@ -341,7 +341,7 @@ AndroidBootImgUpdateFdt (
> >
> > Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
> > "linux,initrd-end",
> > - (UINTN)(RamdiskData + RamdiskSize));
> > + ((UINTN)RamdiskData + RamdiskSize));
> > if (EFI_ERROR (Status)) {
> > goto Fdt_Exit;
> > }
> >
>
> RamdiskSize is a UINTN, so this is OK too.
>
> (You could even strip the outer parentheses:
>
> (UINTN)RamdiskData + RamdiskSize
> )
Yes, I'll do that.
I just found the moving a single character aesthetically pleasing :)
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
/
Leif
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib
2020-07-22 20:34 ` Laszlo Ersek
2020-07-23 10:57 ` Leif Lindholm
@ 2020-07-23 12:08 ` Leif Lindholm
1 sibling, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2020-07-23 12:08 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: devel, Pierre Gondois, Bob Feng
Series pushed as d0da48f112de..e43d0884ed93, with parentheses dropped
as per Laszlo's comment, and Pierre added as reported-by for this
flavour of this patch.
Regards,
Leif
On Wed, Jul 22, 2020 at 22:34:52 +0200, Laszlo Ersek wrote:
> On 07/21/20 14:50, Leif Lindholm wrote:
> > Commit dbd546a32d5a
> > ("BaseTools: Add gcc flag to warn on void* pointer arithmetic")
> > does its work and triggers build errors in this library.
> > Update the affected code to build correctly again.
> >
> > Cc: Pierre Gondois <pierre.gondois@arm.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Bob Feng<bob.c.feng@intel.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >
> > Pierre - can you please ensure to CC Arm maintainers when proposing
> > changes to Arm build flags? (And build test all the top-level edk2
> > packages *cough*.)
>
> or we could perhaps introduce
>
> EmbeddedPkg/EmbeddedPkg.ci.yaml
>
> >
> > Bob - can you please ensure Arm maintainers have commented on changes to
> > global build flags?
> > (Would it be possible to break up tools_def.template into separate
> > arch-specific include files so we could have GetMaintainer.py be
> > more helpful for this?)
> >
> > Laszlo - you're not formally an EmbeddedPkg reviewer, but Ard is out for
> > another couple of weeks. But since the Linaro CI is currently broken and
> > the fix is trivial, could you have a look please?
> >
> > EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> > index e1036954ee58..15b5bf451330 100644
> > --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> > +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> > @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
> > ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> >
> > *KernelSize = Header->KernelSize;
> > - *Kernel = BootImg + Header->PageSize;
> > + *Kernel = (VOID *)((UINTN)BootImg + Header->PageSize);
> > return EFI_SUCCESS;
> > }
> >
>
> Header->PageSize has type UINT32, so this is OK (the addition is
> performed in UINTN, so the conversion back to VOID* is from UINTN).
>
> > @@ -341,7 +341,7 @@ AndroidBootImgUpdateFdt (
> >
> > Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
> > "linux,initrd-end",
> > - (UINTN)(RamdiskData + RamdiskSize));
> > + ((UINTN)RamdiskData + RamdiskSize));
> > if (EFI_ERROR (Status)) {
> > goto Fdt_Exit;
> > }
> >
>
> RamdiskSize is a UINTN, so this is OK too.
>
> (You could even strip the outer parentheses:
>
> (UINTN)RamdiskData + RamdiskSize
> )
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 7+ messages in thread