[PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[Leif Lindholm]

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

Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[PierreGondois]

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.

Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[Bob Feng]

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

Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[Leif Lindholm]

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.

Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[Laszlo Ersek]

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

Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[Leif Lindholm]

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

Re: [PATCH 1/1] EmbeddedPkg: fix gcc build errors in AndroidBootImgLib

[Leif Lindholm]

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
>