[PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Leif Lindholm]

When building without LTO, gcc incorrectly resolves the hazards for
'PciRegBase’ when inlining, leading to "may be used uninitialized"
warnings (and hence build failure with -Werror).
Eliminate this warning by explicitly initializing the variable to 0.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---

For those who have heard me mentioning this before but arguing against
upstreaming this patch: I only just tweaked that this warning doesn't
go away with more recent toolchains, but simply when switching to GCC5
build profile, and hence LTO. Build failure still reproducible with
gcc 6.3.1 and GCC49.

 ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index da93eb5829..18491c7378 100644
--- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
+++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
@@ -343,6 +343,7 @@ ArmJunoSetNicMacAddress ()
     return Status;
   }
 
+  PciRegBase = 0;
   Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
   if (EFI_ERROR (Status)) {
     return Status;
-- 
2.11.0

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Ard Biesheuvel]

(+ Laszlo)

On 20 June 2017 at 13:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> When building without LTO, gcc incorrectly resolves the hazards for
> 'PciRegBase’ when inlining, leading to "may be used uninitialized"
> warnings (and hence build failure with -Werror).
> Eliminate this warning by explicitly initializing the variable to 0.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>
> For those who have heard me mentioning this before but arguing against
> upstreaming this patch: I only just tweaked that this warning doesn't
> go away with more recent toolchains, but simply when switching to GCC5
> build profile, and hence LTO. Build failure still reproducible with
> gcc 6.3.1 and GCC49.
>

/me annoyed

We keep hitting this with GCC, and I profoundly dislike having to add
redundant initialization sequences. Is there any other solution
possible, e.g., disable this warning for certain builds?

>  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index da93eb5829..18491c7378 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> @@ -343,6 +343,7 @@ ArmJunoSetNicMacAddress ()
>      return Status;
>    }
>
> +  PciRegBase = 0;
>    Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
>    if (EFI_ERROR (Status)) {
>      return Status;
> --
> 2.11.0
>

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Ryan Harkin]

On 20 June 2017 at 12:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> (+ Laszlo)
>
> On 20 June 2017 at 13:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> When building without LTO, gcc incorrectly resolves the hazards for
>> 'PciRegBase’ when inlining, leading to "may be used uninitialized"
>> warnings (and hence build failure with -Werror).
>> Eliminate this warning by explicitly initializing the variable to 0.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Ryan Harkin <ryan.harkin@linaro.org>

>
>> ---
>>
>> For those who have heard me mentioning this before but arguing against
>> upstreaming this patch: I only just tweaked that this warning doesn't
>> go away with more recent toolchains, but simply when switching to GCC5
>> build profile, and hence LTO. Build failure still reproducible with
>> gcc 6.3.1 and GCC49.
>>
>
> /me annoyed
>
> We keep hitting this with GCC, and I profoundly dislike having to add
> redundant initialization sequences. Is there any other solution
> possible, e.g., disable this warning for certain builds?
>
>>  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
>> index da93eb5829..18491c7378 100644
>> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
>> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
>> @@ -343,6 +343,7 @@ ArmJunoSetNicMacAddress ()
>>      return Status;
>>    }
>>
>> +  PciRegBase = 0;
>>    Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
>>    if (EFI_ERROR (Status)) {
>>      return Status;
>> --
>> 2.11.0
>>

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Leif Lindholm]

On Tue, Jun 20, 2017 at 01:28:38PM +0200, Ard Biesheuvel wrote:
> (+ Laszlo)
> 
> On 20 June 2017 at 13:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > When building without LTO, gcc incorrectly resolves the hazards for
> > 'PciRegBase’ when inlining, leading to "may be used uninitialized"
> > warnings (and hence build failure with -Werror).
> > Eliminate this warning by explicitly initializing the variable to 0.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks!

> > ---
> >
> > For those who have heard me mentioning this before but arguing against
> > upstreaming this patch: I only just tweaked that this warning doesn't
> > go away with more recent toolchains, but simply when switching to GCC5
> > build profile, and hence LTO. Build failure still reproducible with
> > gcc 6.3.1 and GCC49.
> 
> /me annoyed

+1

> We keep hitting this with GCC, and I profoundly dislike having to add
> redundant initialization sequences. Is there any other solution
> possible, e.g., disable this warning for certain builds?

I would really like to not disable it.
When it gets it right, that's usually a pretty horrific bug caught.
And if we switch to LTO as the norm, I guess these will become less
noticeable over time.
But it does bug me too.

/
    Leif

> >  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> > index da93eb5829..18491c7378 100644
> > --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> > +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> > @@ -343,6 +343,7 @@ ArmJunoSetNicMacAddress ()
> >      return Status;
> >    }
> >
> > +  PciRegBase = 0;
> >    Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> > --
> > 2.11.0
> >

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Leif Lindholm]

On Tue, Jun 20, 2017 at 12:44:09PM +0100, Ryan Harkin wrote:
> On 20 June 2017 at 12:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > (+ Laszlo)
> >
> > On 20 June 2017 at 13:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> When building without LTO, gcc incorrectly resolves the hazards for
> >> 'PciRegBase’ when inlining, leading to "may be used uninitialized"
> >> warnings (and hence build failure with -Werror).
> >> Eliminate this warning by explicitly initializing the variable to 0.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Reviewed-by: Ryan Harkin <ryan.harkin@linaro.org>

Thanks!

Pushed as 2f93c5077f.

> >
> >> ---
> >>
> >> For those who have heard me mentioning this before but arguing against
> >> upstreaming this patch: I only just tweaked that this warning doesn't
> >> go away with more recent toolchains, but simply when switching to GCC5
> >> build profile, and hence LTO. Build failure still reproducible with
> >> gcc 6.3.1 and GCC49.
> >>
> >
> > /me annoyed
> >
> > We keep hitting this with GCC, and I profoundly dislike having to add
> > redundant initialization sequences. Is there any other solution
> > possible, e.g., disable this warning for certain builds?
> >
> >>  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> >> index da93eb5829..18491c7378 100644
> >> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> >> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> >> @@ -343,6 +343,7 @@ ArmJunoSetNicMacAddress ()
> >>      return Status;
> >>    }
> >>
> >> +  PciRegBase = 0;
> >>    Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
> >>    if (EFI_ERROR (Status)) {
> >>      return Status;
> >> --
> >> 2.11.0
> >>

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Laszlo Ersek]

On 06/20/17 14:00, Leif Lindholm wrote:
> On Tue, Jun 20, 2017 at 01:28:38PM +0200, Ard Biesheuvel wrote:
>> (+ Laszlo)
>>
>> On 20 June 2017 at 13:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> When building without LTO, gcc incorrectly resolves the hazards for
>>> 'PciRegBase’ when inlining, leading to "may be used uninitialized"
>>> warnings (and hence build failure with -Werror).
>>> Eliminate this warning by explicitly initializing the variable to 0.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Thanks!
> 
>>> ---
>>>
>>> For those who have heard me mentioning this before but arguing against
>>> upstreaming this patch: I only just tweaked that this warning doesn't
>>> go away with more recent toolchains, but simply when switching to GCC5
>>> build profile, and hence LTO. Build failure still reproducible with
>>> gcc 6.3.1 and GCC49.
>>
>> /me annoyed
> 
> +1
> 
>> We keep hitting this with GCC, and I profoundly dislike having to add
>> redundant initialization sequences. Is there any other solution
>> possible, e.g., disable this warning for certain builds?
> 
> I would really like to not disable it.
> When it gets it right, that's usually a pretty horrific bug caught.
> And if we switch to LTO as the norm, I guess these will become less
> noticeable over time.
> But it does bug me too.

Perhaps we should introduce two macros to "MdePkg/Include/Base.h",

#define UNUSED_POINTER NULL
#define UNUSED_INTEGER 0

These could be used to suppress such warnings, without the risk of
misleading programmers (as to the real necessity of the variable
assignment at hand).

Thanks
Laszlo

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Ard Biesheuvel]

On 20 June 2017 at 17:44, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/20/17 14:00, Leif Lindholm wrote:
>> On Tue, Jun 20, 2017 at 01:28:38PM +0200, Ard Biesheuvel wrote:
>>> (+ Laszlo)
>>>
>>> On 20 June 2017 at 13:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>> When building without LTO, gcc incorrectly resolves the hazards for
>>>> 'PciRegBase’ when inlining, leading to "may be used uninitialized"
>>>> warnings (and hence build failure with -Werror).
>>>> Eliminate this warning by explicitly initializing the variable to 0.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>
>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Thanks!
>>
>>>> ---
>>>>
>>>> For those who have heard me mentioning this before but arguing against
>>>> upstreaming this patch: I only just tweaked that this warning doesn't
>>>> go away with more recent toolchains, but simply when switching to GCC5
>>>> build profile, and hence LTO. Build failure still reproducible with
>>>> gcc 6.3.1 and GCC49.
>>>
>>> /me annoyed
>>
>> +1
>>
>>> We keep hitting this with GCC, and I profoundly dislike having to add
>>> redundant initialization sequences. Is there any other solution
>>> possible, e.g., disable this warning for certain builds?
>>
>> I would really like to not disable it.
>> When it gets it right, that's usually a pretty horrific bug caught.
>> And if we switch to LTO as the norm, I guess these will become less
>> noticeable over time.
>> But it does bug me too.
>
> Perhaps we should introduce two macros to "MdePkg/Include/Base.h",
>
> #define UNUSED_POINTER NULL
> #define UNUSED_INTEGER 0
>
> These could be used to suppress such warnings, without the risk of
> misleading programmers (as to the real necessity of the variable
> assignment at hand).
>

Or perhaps

#ifdef __GNUC__
#define USED(var)  var = (typeof(var))0
#else
#define USED(var)
#endif

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Laszlo Ersek]

On 06/20/17 21:52, Ard Biesheuvel wrote:
> On 20 June 2017 at 17:44, Laszlo Ersek <lersek@redhat.com> wrote:

>> Perhaps we should introduce two macros to "MdePkg/Include/Base.h",
>>
>> #define UNUSED_POINTER NULL
>> #define UNUSED_INTEGER 0
>>
>> These could be used to suppress such warnings, without the risk of
>> misleading programmers (as to the real necessity of the variable
>> assignment at hand).
>>
> 
> Or perhaps
> 
> #ifdef __GNUC__
> #define USED(var)  var = (typeof(var))0
> #else
> #define USED(var)
> #endif
> 

Sure, why not :)

Re: [PATCH] ArmPlatformPkg: eliminate Juno gcc build warning

[Leif Lindholm]

On Tue, Jun 20, 2017 at 09:52:20PM +0200, Ard Biesheuvel wrote:
> >>>> For those who have heard me mentioning this before but arguing against
> >>>> upstreaming this patch: I only just tweaked that this warning doesn't
> >>>> go away with more recent toolchains, but simply when switching to GCC5
> >>>> build profile, and hence LTO. Build failure still reproducible with
> >>>> gcc 6.3.1 and GCC49.
> >>>
> >>> /me annoyed
> >>
> >> +1
> >>
> >>> We keep hitting this with GCC, and I profoundly dislike having to add
> >>> redundant initialization sequences. Is there any other solution
> >>> possible, e.g., disable this warning for certain builds?
> >>
> >> I would really like to not disable it.
> >> When it gets it right, that's usually a pretty horrific bug caught.
> >> And if we switch to LTO as the norm, I guess these will become less
> >> noticeable over time.
> >> But it does bug me too.
> >
> > Perhaps we should introduce two macros to "MdePkg/Include/Base.h",
> >
> > #define UNUSED_POINTER NULL
> > #define UNUSED_INTEGER 0
> >
> > These could be used to suppress such warnings, without the risk of
> > misleading programmers (as to the real necessity of the variable
> > assignment at hand).
> >
> 
> Or perhaps
> 
> #ifdef __GNUC__
> #define USED(var)  var = (typeof(var))0
> #else
> #define USED(var)
> #endif

So ... I prefer this version.

However, semantically, for the situation here (may be used before
initialized), "USED" does not make sense.
Could we have an identical "INITIALIZED" instead?

/
    Leif