[PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[Sergey Temerkhanov]

These registers are reserved for PPIs and unimplemented on
some architectures

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 8af97a9..dc6b896 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -257,7 +257,7 @@ GicV3DxeInitialize (
     MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
   }
 
-  for (Index = 0; Index < mGicNumInterrupts; Index++) {
+  for (Index = 32; Index < mGicNumInterrupts; Index++) {
     GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
 
     // Set Priority
-- 
2.7.4

[PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Sergey Temerkhanov]

Fix undefined behavior by avoiding parameter type promotion

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 MdePkg/Include/Library/UefiLib.h | 2 +-
 MdePkg/Library/UefiLib/UefiLib.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 0b14792..4e4697c 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -818,7 +818,7 @@ CHAR8 *
 EFIAPI
 GetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages, 
-  IN BOOLEAN      Iso639Language,
+  IN UINTN      Iso639Language,
   ...
   );
 
diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
index a7eee01..74528ec 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -1514,7 +1514,7 @@ CHAR8 *
 EFIAPI
 GetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages, 
-  IN BOOLEAN      Iso639Language,
+  IN UINTN      Iso639Language,
   ...
   )
 {
-- 
2.7.4

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Gao, Liming]

Sergey:
  Could you give more detail on the undefined behavior on variadic parameters?

  I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find other tool chain reports it?

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
> Sent: Tuesday, May 16, 2017 10:57 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
> 
> Fix undefined behavior by avoiding parameter type promotion
> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> ---
>  MdePkg/Include/Library/UefiLib.h | 2 +-
>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
> index 0b14792..4e4697c 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -818,7 +818,7 @@ CHAR8 *
>  EFIAPI
>  GetBestLanguage (
>    IN CONST CHAR8  *SupportedLanguages,
> -  IN BOOLEAN      Iso639Language,
> +  IN UINTN      Iso639Language,
>    ...
>    );
> 
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
> index a7eee01..74528ec 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1514,7 +1514,7 @@ CHAR8 *
>  EFIAPI
>  GetBestLanguage (
>    IN CONST CHAR8  *SupportedLanguages,
> -  IN BOOLEAN      Iso639Language,
> +  IN UINTN      Iso639Language,
>    ...
>    )
>  {
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Sergei Temerkhanov]

On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
> Sergey:
>   Could you give more detail on the undefined behavior on variadic parameters?
>
>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find other tool chain reports it?

Yes, this is exactly the bug this patch fixes.

As per the C99 standard:
"The parameter parmN is the identifier of the rightmost parameter in
the variable parameter list in the function definition (the one just
before the , ...). If the parameter parmN is declared with the
register storage class, with a function or array type, or with a type
that is not compatible with the type that results after application of
the default argument promotions, the behavior is undefined."

That's exactly the case here since BOOLEAN is a typedef for unsigned
char. It undergoes a promotion to an unsigned int which is not a
compatible type for unsigned char. Correct me if I'm wrong.

Regards,
Sergey

>
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
>> Sent: Tuesday, May 16, 2017 10:57 AM
>> To: edk2-devel@lists.01.org
>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>
>> Fix undefined behavior by avoiding parameter type promotion
>>
>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> ---
>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>> index 0b14792..4e4697c 100644
>> --- a/MdePkg/Include/Library/UefiLib.h
>> +++ b/MdePkg/Include/Library/UefiLib.h
>> @@ -818,7 +818,7 @@ CHAR8 *
>>  EFIAPI
>>  GetBestLanguage (
>>    IN CONST CHAR8  *SupportedLanguages,
>> -  IN BOOLEAN      Iso639Language,
>> +  IN UINTN      Iso639Language,
>>    ...
>>    );
>>
>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>> index a7eee01..74528ec 100644
>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>  EFIAPI
>>  GetBestLanguage (
>>    IN CONST CHAR8  *SupportedLanguages,
>> -  IN BOOLEAN      Iso639Language,
>> +  IN UINTN      Iso639Language,
>>    ...
>>    )
>>  {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Gao, Liming]

Sergey:
  Now, VS and GCC don't report such issue. How do you find them? And, this change modifies the function API, does it impact the consumer code?

Thanks
Liming
> -----Original Message-----
> From: Sergei Temerkhanov [mailto:s.temerkhanov@gmail.com]
> Sent: Tuesday, May 16, 2017 8:11 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
> 
> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
> > Sergey:
> >   Could you give more detail on the undefined behavior on variadic parameters?
> >
> >   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find
> other tool chain reports it?
> 
> Yes, this is exactly the bug this patch fixes.
> 
> As per the C99 standard:
> "The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the , ...). If the parameter parmN is declared with the
> register storage class, with a function or array type, or with a type
> that is not compatible with the type that results after application of
> the default argument promotions, the behavior is undefined."
> 
> That's exactly the case here since BOOLEAN is a typedef for unsigned
> char. It undergoes a promotion to an unsigned int which is not a
> compatible type for unsigned char. Correct me if I'm wrong.
> 
> Regards,
> Sergey
> 
> >
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
> >> Sent: Tuesday, May 16, 2017 10:57 AM
> >> To: edk2-devel@lists.01.org
> >> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
> >>
> >> Fix undefined behavior by avoiding parameter type promotion
> >>
> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> >> ---
> >>  MdePkg/Include/Library/UefiLib.h | 2 +-
> >>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
> >> index 0b14792..4e4697c 100644
> >> --- a/MdePkg/Include/Library/UefiLib.h
> >> +++ b/MdePkg/Include/Library/UefiLib.h
> >> @@ -818,7 +818,7 @@ CHAR8 *
> >>  EFIAPI
> >>  GetBestLanguage (
> >>    IN CONST CHAR8  *SupportedLanguages,
> >> -  IN BOOLEAN      Iso639Language,
> >> +  IN UINTN      Iso639Language,
> >>    ...
> >>    );
> >>
> >> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
> >> index a7eee01..74528ec 100644
> >> --- a/MdePkg/Library/UefiLib/UefiLib.c
> >> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> >> @@ -1514,7 +1514,7 @@ CHAR8 *
> >>  EFIAPI
> >>  GetBestLanguage (
> >>    IN CONST CHAR8  *SupportedLanguages,
> >> -  IN BOOLEAN      Iso639Language,
> >> +  IN UINTN      Iso639Language,
> >>    ...
> >>    )
> >>  {
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Sergei Temerkhanov]

On Wed, May 17, 2017 at 6:30 PM, Gao, Liming <liming.gao@intel.com> wrote:
> Sergey:
>   Now, VS and GCC don't report such issue. How do you find them?

Clang build for ARM64 revealed it.

> And, this change modifies the function API, does it impact the consumer code?

No changes to the consumer code are needed - integer promotions are
handled by the compilers. In fact, even the ABI remains the same b/c
full words are passed as function parameters.

Regards,
Sergey

>
> Thanks
> Liming
>> -----Original Message-----
>> From: Sergei Temerkhanov [mailto:s.temerkhanov@gmail.com]
>> Sent: Tuesday, May 16, 2017 8:11 PM
>> To: Gao, Liming <liming.gao@intel.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>
>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
>> > Sergey:
>> >   Could you give more detail on the undefined behavior on variadic parameters?
>> >
>> >   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find
>> other tool chain reports it?
>>
>> Yes, this is exactly the bug this patch fixes.
>>
>> As per the C99 standard:
>> "The parameter parmN is the identifier of the rightmost parameter in
>> the variable parameter list in the function definition (the one just
>> before the , ...). If the parameter parmN is declared with the
>> register storage class, with a function or array type, or with a type
>> that is not compatible with the type that results after application of
>> the default argument promotions, the behavior is undefined."
>>
>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>> char. It undergoes a promotion to an unsigned int which is not a
>> compatible type for unsigned char. Correct me if I'm wrong.
>>
>> Regards,
>> Sergey
>>
>> >
>> > Thanks
>> > Liming
>> >> -----Original Message-----
>> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
>> >> Sent: Tuesday, May 16, 2017 10:57 AM
>> >> To: edk2-devel@lists.01.org
>> >> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>> >>
>> >> Fix undefined behavior by avoiding parameter type promotion
>> >>
>> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> >> ---
>> >>  MdePkg/Include/Library/UefiLib.h | 2 +-
>> >>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>> >>  2 files changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>> >> index 0b14792..4e4697c 100644
>> >> --- a/MdePkg/Include/Library/UefiLib.h
>> >> +++ b/MdePkg/Include/Library/UefiLib.h
>> >> @@ -818,7 +818,7 @@ CHAR8 *
>> >>  EFIAPI
>> >>  GetBestLanguage (
>> >>    IN CONST CHAR8  *SupportedLanguages,
>> >> -  IN BOOLEAN      Iso639Language,
>> >> +  IN UINTN      Iso639Language,
>> >>    ...
>> >>    );
>> >>
>> >> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>> >> index a7eee01..74528ec 100644
>> >> --- a/MdePkg/Library/UefiLib/UefiLib.c
>> >> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>> >> @@ -1514,7 +1514,7 @@ CHAR8 *
>> >>  EFIAPI
>> >>  GetBestLanguage (
>> >>    IN CONST CHAR8  *SupportedLanguages,
>> >> -  IN BOOLEAN      Iso639Language,
>> >> +  IN UINTN      Iso639Language,
>> >>    ...
>> >>    )
>> >>  {
>> >> --
>> >> 2.7.4
>> >>
>> >> _______________________________________________
>> >> edk2-devel mailing list
>> >> edk2-devel@lists.01.org
>> >> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Andrew Fish]

Sergei,

We are hitting this too. 

Our temporary work around was to suppress the warning for the libraries via our platforms DSC file so we did not need to override things in edk2. 

You can override the library build options if you add them to the [Components] section. 

MdePkg/Library/UefiLib/UefiLib.inf {
  <BuildOptions>
    XCODE:*_*_*_CC_FLAGS  = -Wno-varargs
}

Thanks,

Andrew Fish

> On May 17, 2017, at 5:26 PM, Sergei Temerkhanov <s.temerkhanov@gmail.com> wrote:
> 
> On Wed, May 17, 2017 at 6:30 PM, Gao, Liming <liming.gao@intel.com> wrote:
>> Sergey:
>>  Now, VS and GCC don't report such issue. How do you find them?
> 
> Clang build for ARM64 revealed it.
> 
>> And, this change modifies the function API, does it impact the consumer code?
> 
> No changes to the consumer code are needed - integer promotions are
> handled by the compilers. In fact, even the ABI remains the same b/c
> full words are passed as function parameters.
> 
> Regards,
> Sergey
> 
>> 
>> Thanks
>> Liming
>>> -----Original Message-----
>>> From: Sergei Temerkhanov [mailto:s.temerkhanov@gmail.com]
>>> Sent: Tuesday, May 16, 2017 8:11 PM
>>> To: Gao, Liming <liming.gao@intel.com>
>>> Cc: edk2-devel@lists.01.org
>>> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>> 
>>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
>>>> Sergey:
>>>>  Could you give more detail on the undefined behavior on variadic parameters?
>>>> 
>>>>  I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find
>>> other tool chain reports it?
>>> 
>>> Yes, this is exactly the bug this patch fixes.
>>> 
>>> As per the C99 standard:
>>> "The parameter parmN is the identifier of the rightmost parameter in
>>> the variable parameter list in the function definition (the one just
>>> before the , ...). If the parameter parmN is declared with the
>>> register storage class, with a function or array type, or with a type
>>> that is not compatible with the type that results after application of
>>> the default argument promotions, the behavior is undefined."
>>> 
>>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>>> char. It undergoes a promotion to an unsigned int which is not a
>>> compatible type for unsigned char. Correct me if I'm wrong.
>>> 
>>> Regards,
>>> Sergey
>>> 
>>>> 
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
>>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>>>> To: edk2-devel@lists.01.org
>>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>>>> 
>>>>> Fix undefined behavior by avoiding parameter type promotion
>>>>> 
>>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>>> ---
>>>>> MdePkg/Include/Library/UefiLib.h | 2 +-
>>>>> MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>>>>> index 0b14792..4e4697c 100644
>>>>> --- a/MdePkg/Include/Library/UefiLib.h
>>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>>> EFIAPI
>>>>> GetBestLanguage (
>>>>>   IN CONST CHAR8  *SupportedLanguages,
>>>>> -  IN BOOLEAN      Iso639Language,
>>>>> +  IN UINTN      Iso639Language,
>>>>>   ...
>>>>>   );
>>>>> 
>>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>>>>> index a7eee01..74528ec 100644
>>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>>> EFIAPI
>>>>> GetBestLanguage (
>>>>>   IN CONST CHAR8  *SupportedLanguages,
>>>>> -  IN BOOLEAN      Iso639Language,
>>>>> +  IN UINTN      Iso639Language,
>>>>>   ...
>>>>>   )
>>>>> {
>>>>> --
>>>>> 2.7.4
>>>>> 
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Sergei Temerkhanov]

On Thu, May 18, 2017 at 6:06 AM, Andrew Fish <afish@apple.com> wrote:
> Sergei,
>
> We are hitting this too.
>
> Our temporary work around was to suppress the warning for the libraries via our platforms DSC file so we did not need to override things in edk2.
>
> You can override the library build options if you add them to the [Components] section.
>
> MdePkg/Library/UefiLib/UefiLib.inf {
>   <BuildOptions>
>     XCODE:*_*_*_CC_FLAGS  = -Wno-varargs

That would be GCC: *_CLANG38_*_CC_FLAGS at least

> }
>
> Thanks,
>
> Andrew Fish
>
>> On May 17, 2017, at 5:26 PM, Sergei Temerkhanov <s.temerkhanov@gmail.com> wrote:
>>
>> On Wed, May 17, 2017 at 6:30 PM, Gao, Liming <liming.gao@intel.com> wrote:
>>> Sergey:
>>>  Now, VS and GCC don't report such issue. How do you find them?
>>
>> Clang build for ARM64 revealed it.
>>
>>> And, this change modifies the function API, does it impact the consumer code?
>>
>> No changes to the consumer code are needed - integer promotions are
>> handled by the compilers. In fact, even the ABI remains the same b/c
>> full words are passed as function parameters.
>>
>> Regards,
>> Sergey
>>
>>>
>>> Thanks
>>> Liming
>>>> -----Original Message-----
>>>> From: Sergei Temerkhanov [mailto:s.temerkhanov@gmail.com]
>>>> Sent: Tuesday, May 16, 2017 8:11 PM
>>>> To: Gao, Liming <liming.gao@intel.com>
>>>> Cc: edk2-devel@lists.01.org
>>>> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>>>
>>>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
>>>>> Sergey:
>>>>>  Could you give more detail on the undefined behavior on variadic parameters?
>>>>>
>>>>>  I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find
>>>> other tool chain reports it?
>>>>
>>>> Yes, this is exactly the bug this patch fixes.
>>>>
>>>> As per the C99 standard:
>>>> "The parameter parmN is the identifier of the rightmost parameter in
>>>> the variable parameter list in the function definition (the one just
>>>> before the , ...). If the parameter parmN is declared with the
>>>> register storage class, with a function or array type, or with a type
>>>> that is not compatible with the type that results after application of
>>>> the default argument promotions, the behavior is undefined."
>>>>
>>>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>>>> char. It undergoes a promotion to an unsigned int which is not a
>>>> compatible type for unsigned char. Correct me if I'm wrong.
>>>>
>>>> Regards,
>>>> Sergey
>>>>
>>>>>
>>>>> Thanks
>>>>> Liming
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
>>>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>>>>> To: edk2-devel@lists.01.org
>>>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>>>>>
>>>>>> Fix undefined behavior by avoiding parameter type promotion
>>>>>>
>>>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>>>> ---
>>>>>> MdePkg/Include/Library/UefiLib.h | 2 +-
>>>>>> MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>>>>>> index 0b14792..4e4697c 100644
>>>>>> --- a/MdePkg/Include/Library/UefiLib.h
>>>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>>>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>>>> EFIAPI
>>>>>> GetBestLanguage (
>>>>>>   IN CONST CHAR8  *SupportedLanguages,
>>>>>> -  IN BOOLEAN      Iso639Language,
>>>>>> +  IN UINTN      Iso639Language,
>>>>>>   ...
>>>>>>   );
>>>>>>
>>>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>>>>>> index a7eee01..74528ec 100644
>>>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>>>> EFIAPI
>>>>>> GetBestLanguage (
>>>>>>   IN CONST CHAR8  *SupportedLanguages,
>>>>>> -  IN BOOLEAN      Iso639Language,
>>>>>> +  IN UINTN      Iso639Language,
>>>>>>   ...
>>>>>>   )
>>>>>> {
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> edk2-devel mailing list
>>>>>> edk2-devel@lists.01.org
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Laszlo Ersek]

On 05/16/17 14:10, Sergei Temerkhanov wrote:
> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
>> Sergey:
>>   Could you give more detail on the undefined behavior on variadic parameters?
>>
>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find other tool chain reports it?
> 
> Yes, this is exactly the bug this patch fixes.
> 
> As per the C99 standard:
> "The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the , ...). If the parameter parmN is declared with the
> register storage class, with a function or array type, or with a type
> that is not compatible with the type that results after application of
> the default argument promotions, the behavior is undefined."
> 
> That's exactly the case here since BOOLEAN is a typedef for unsigned
> char. It undergoes a promotion to an unsigned int

Side topic:

It is promoted, but not to "unsigned int".

The standard says, in "6.3.1.1 Boolean, characters, and integers",
paragraph 2,

    The following may be used in an expression wherever an /int/ or
    /unsigned int/ may be used:

    — An object or expression with an integer type whose integer
      conversion rank is less than or equal to the rank of /int/ and
      /unsigned int/.
    — A bit-field of type /_Bool/, /int/, /signed int/, or
      /unsigned int/.

    If an /int/ can represent all values of the original type, the value
    is converted to an /int/; otherwise, it is converted to an
    /unsigned int/. These are called the /integer promotions/. [...]

On all supported edk2 platforms, "unsigned char"'s range is 0..255
inclusive, which can be represented by "int" (again on all supported
edk2 platforms). So the promotion occurs to "int", not "unsigned int"


Furthermore, in place of the suggested UINTN type (which is fine), the
following further types would be correct: INT32, UINT32, INT64, UINT64,
INTN. The reason is that all of these map to standard C types, on all
edk2 platforms, whose integer conversion ranks are not less than that of
"int" and "unsigned int". Hence they are all unaffected by the integer
promotions.

(This digression does not affect your main point, which remains correct;
I just wanted to be precise here, since we're quoting the standard.)

Thanks
Laszlo

> which is not a
> compatible type for unsigned char. Correct me if I'm wrong.
> 
> Regards,
> Sergey
> 
>>
>> Thanks
>> Liming
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>> To: edk2-devel@lists.01.org
>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>>
>>> Fix undefined behavior by avoiding parameter type promotion
>>>
>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>> ---
>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>>> index 0b14792..4e4697c 100644
>>> --- a/MdePkg/Include/Library/UefiLib.h
>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>  EFIAPI
>>>  GetBestLanguage (
>>>    IN CONST CHAR8  *SupportedLanguages,
>>> -  IN BOOLEAN      Iso639Language,
>>> +  IN UINTN      Iso639Language,
>>>    ...
>>>    );
>>>
>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>>> index a7eee01..74528ec 100644
>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>  EFIAPI
>>>  GetBestLanguage (
>>>    IN CONST CHAR8  *SupportedLanguages,
>>> -  IN BOOLEAN      Iso639Language,
>>> +  IN UINTN      Iso639Language,
>>>    ...
>>>    )
>>>  {
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Gao, Liming]

Laszlo:
  Based on your analysis, the parameter type should be changed from BOOLEAN to INT32 to match its promotion type. Right?

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Thursday, May 18, 2017 6:20 PM
>To: Sergei Temerkhanov <s.temerkhanov@gmail.com>; Gao, Liming
><liming.gao@intel.com>
>Cc: edk2-devel@lists.01.org
>Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>parameters
>
>On 05/16/17 14:10, Sergei Temerkhanov wrote:
>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com>
>wrote:
>>> Sergey:
>>>   Could you give more detail on the undefined behavior on variadic
>parameters?
>>>
>>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this
>issues found in the latest CLANG tool chain. Do you find other tool chain
>reports it?
>>
>> Yes, this is exactly the bug this patch fixes.
>>
>> As per the C99 standard:
>> "The parameter parmN is the identifier of the rightmost parameter in
>> the variable parameter list in the function definition (the one just
>> before the , ...). If the parameter parmN is declared with the
>> register storage class, with a function or array type, or with a type
>> that is not compatible with the type that results after application of
>> the default argument promotions, the behavior is undefined."
>>
>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>> char. It undergoes a promotion to an unsigned int
>
>Side topic:
>
>It is promoted, but not to "unsigned int".
>
>The standard says, in "6.3.1.1 Boolean, characters, and integers",
>paragraph 2,
>
>    The following may be used in an expression wherever an /int/ or
>    /unsigned int/ may be used:
>
>    — An object or expression with an integer type whose integer
>      conversion rank is less than or equal to the rank of /int/ and
>      /unsigned int/.
>    — A bit-field of type /_Bool/, /int/, /signed int/, or
>      /unsigned int/.
>
>    If an /int/ can represent all values of the original type, the value
>    is converted to an /int/; otherwise, it is converted to an
>    /unsigned int/. These are called the /integer promotions/. [...]
>
>On all supported edk2 platforms, "unsigned char"'s range is 0..255
>inclusive, which can be represented by "int" (again on all supported
>edk2 platforms). So the promotion occurs to "int", not "unsigned int"
>
>
>Furthermore, in place of the suggested UINTN type (which is fine), the
>following further types would be correct: INT32, UINT32, INT64, UINT64,
>INTN. The reason is that all of these map to standard C types, on all
>edk2 platforms, whose integer conversion ranks are not less than that of
>"int" and "unsigned int". Hence they are all unaffected by the integer
>promotions.
>
>(This digression does not affect your main point, which remains correct;
>I just wanted to be precise here, since we're quoting the standard.)
>
>Thanks
>Laszlo
>
>> which is not a
>> compatible type for unsigned char. Correct me if I'm wrong.
>>
>> Regards,
>> Sergey
>>
>>>
>>> Thanks
>>> Liming
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Sergey Temerkhanov
>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>>> To: edk2-devel@lists.01.org
>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>parameters
>>>>
>>>> Fix undefined behavior by avoiding parameter type promotion
>>>>
>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>> ---
>>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/MdePkg/Include/Library/UefiLib.h
>b/MdePkg/Include/Library/UefiLib.h
>>>> index 0b14792..4e4697c 100644
>>>> --- a/MdePkg/Include/Library/UefiLib.h
>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>>  EFIAPI
>>>>  GetBestLanguage (
>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>> -  IN BOOLEAN      Iso639Language,
>>>> +  IN UINTN      Iso639Language,
>>>>    ...
>>>>    );
>>>>
>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
>b/MdePkg/Library/UefiLib/UefiLib.c
>>>> index a7eee01..74528ec 100644
>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>>  EFIAPI
>>>>  GetBestLanguage (
>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>> -  IN BOOLEAN      Iso639Language,
>>>> +  IN UINTN      Iso639Language,
>>>>    ...
>>>>    )
>>>>  {
>>>> --
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Laszlo Ersek]

On 05/19/17 03:35, Gao, Liming wrote:
> Laszlo:
>   Based on your analysis, the parameter type should be changed from BOOLEAN to INT32 to match its promotion type. Right?
Not necessarily.

BOOLEAN (unsigned char) is indeed promoted to INT32 (int), so BOOLEAN is
not appropriate.

But any integer type that is compatible with the promoted type is fine.
Such types are basically all the integer types whose integer conversion
rank is equal to, or larger than, the rank of "int" and "unsigned int".
That means INT32, UINT32, INT64, UINT64, INTN, UINTN. Any of these are
fine, because they are not changed by integer promotion. The patch
picked UINTN, so it's OK.

Laszlo

> 
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, May 18, 2017 6:20 PM
>> To: Sergei Temerkhanov <s.temerkhanov@gmail.com>; Gao, Liming
>> <liming.gao@intel.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>> parameters
>>
>> On 05/16/17 14:10, Sergei Temerkhanov wrote:
>>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com>
>> wrote:
>>>> Sergey:
>>>>   Could you give more detail on the undefined behavior on variadic
>> parameters?
>>>>
>>>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this
>> issues found in the latest CLANG tool chain. Do you find other tool chain
>> reports it?
>>>
>>> Yes, this is exactly the bug this patch fixes.
>>>
>>> As per the C99 standard:
>>> "The parameter parmN is the identifier of the rightmost parameter in
>>> the variable parameter list in the function definition (the one just
>>> before the , ...). If the parameter parmN is declared with the
>>> register storage class, with a function or array type, or with a type
>>> that is not compatible with the type that results after application of
>>> the default argument promotions, the behavior is undefined."
>>>
>>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>>> char. It undergoes a promotion to an unsigned int
>>
>> Side topic:
>>
>> It is promoted, but not to "unsigned int".
>>
>> The standard says, in "6.3.1.1 Boolean, characters, and integers",
>> paragraph 2,
>>
>>    The following may be used in an expression wherever an /int/ or
>>    /unsigned int/ may be used:
>>
>>    — An object or expression with an integer type whose integer
>>      conversion rank is less than or equal to the rank of /int/ and
>>      /unsigned int/.
>>    — A bit-field of type /_Bool/, /int/, /signed int/, or
>>      /unsigned int/.
>>
>>    If an /int/ can represent all values of the original type, the value
>>    is converted to an /int/; otherwise, it is converted to an
>>    /unsigned int/. These are called the /integer promotions/. [...]
>>
>> On all supported edk2 platforms, "unsigned char"'s range is 0..255
>> inclusive, which can be represented by "int" (again on all supported
>> edk2 platforms). So the promotion occurs to "int", not "unsigned int"
>>
>>
>> Furthermore, in place of the suggested UINTN type (which is fine), the
>> following further types would be correct: INT32, UINT32, INT64, UINT64,
>> INTN. The reason is that all of these map to standard C types, on all
>> edk2 platforms, whose integer conversion ranks are not less than that of
>> "int" and "unsigned int". Hence they are all unaffected by the integer
>> promotions.
>>
>> (This digression does not affect your main point, which remains correct;
>> I just wanted to be precise here, since we're quoting the standard.)
>>
>> Thanks
>> Laszlo
>>
>>> which is not a
>>> compatible type for unsigned char. Correct me if I'm wrong.
>>>
>>> Regards,
>>> Sergey
>>>
>>>>
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Sergey Temerkhanov
>>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>>>> To: edk2-devel@lists.01.org
>>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>> parameters
>>>>>
>>>>> Fix undefined behavior by avoiding parameter type promotion
>>>>>
>>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>>> ---
>>>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>>>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/MdePkg/Include/Library/UefiLib.h
>> b/MdePkg/Include/Library/UefiLib.h
>>>>> index 0b14792..4e4697c 100644
>>>>> --- a/MdePkg/Include/Library/UefiLib.h
>>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>>>  EFIAPI
>>>>>  GetBestLanguage (
>>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>>> -  IN BOOLEAN      Iso639Language,
>>>>> +  IN UINTN      Iso639Language,
>>>>>    ...
>>>>>    );
>>>>>
>>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
>> b/MdePkg/Library/UefiLib/UefiLib.c
>>>>> index a7eee01..74528ec 100644
>>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>>>  EFIAPI
>>>>>  GetBestLanguage (
>>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>>> -  IN BOOLEAN      Iso639Language,
>>>>> +  IN UINTN      Iso639Language,
>>>>>    ...
>>>>>    )
>>>>>  {
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Sergei Temerkhanov]

On Thu, May 18, 2017 at 1:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/16/17 14:10, Sergei Temerkhanov wrote:
>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
>>> Sergey:
>>>   Could you give more detail on the undefined behavior on variadic parameters?
>>>
>>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find other tool chain reports it?
>>
>> Yes, this is exactly the bug this patch fixes.
>>
>> As per the C99 standard:
>> "The parameter parmN is the identifier of the rightmost parameter in
>> the variable parameter list in the function definition (the one just
>> before the , ...). If the parameter parmN is declared with the
>> register storage class, with a function or array type, or with a type
>> that is not compatible with the type that results after application of
>> the default argument promotions, the behavior is undefined."
>>
>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>> char. It undergoes a promotion to an unsigned int
>
> Side topic:
>
> It is promoted, but not to "unsigned int".
>
> The standard says, in "6.3.1.1 Boolean, characters, and integers",
> paragraph 2,
>
>     The following may be used in an expression wherever an /int/ or
>     /unsigned int/ may be used:
>
>     — An object or expression with an integer type whose integer
>       conversion rank is less than or equal to the rank of /int/ and
>       /unsigned int/.
>     — A bit-field of type /_Bool/, /int/, /signed int/, or
>       /unsigned int/.
>
>     If an /int/ can represent all values of the original type, the value
>     is converted to an /int/; otherwise, it is converted to an
>     /unsigned int/. These are called the /integer promotions/. [...]
>
> On all supported edk2 platforms, "unsigned char"'s range is 0..255
> inclusive, which can be represented by "int" (again on all supported
> edk2 platforms). So the promotion occurs to "int", not "unsigned int"
>
>
> Furthermore, in place of the suggested UINTN type (which is fine), the
> following further types would be correct: INT32, UINT32, INT64, UINT64,
> INTN.

On 32-bit architectures, using 64-bit types here may change the ABI. Which might
affect some corner cases like linking precompiled object files to the
library in question.

> The reason is that all of these map to standard C types, on all
> edk2 platforms, whose integer conversion ranks are not less than that of
> "int" and "unsigned int". Hence they are all unaffected by the integer
> promotions.
>
> (This digression does not affect your main point, which remains correct;
> I just wanted to be precise here, since we're quoting the standard.)
>
> Thanks
> Laszlo
>
>> which is not a
>> compatible type for unsigned char. Correct me if I'm wrong.
>>
>> Regards,
>> Sergey
>>
>>>
>>> Thanks
>>> Liming
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>>> To: edk2-devel@lists.01.org
>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>>>
>>>> Fix undefined behavior by avoiding parameter type promotion
>>>>
>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>> ---
>>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>>>> index 0b14792..4e4697c 100644
>>>> --- a/MdePkg/Include/Library/UefiLib.h
>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>>  EFIAPI
>>>>  GetBestLanguage (
>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>> -  IN BOOLEAN      Iso639Language,
>>>> +  IN UINTN      Iso639Language,
>>>>    ...
>>>>    );
>>>>
>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>>>> index a7eee01..74528ec 100644
>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>>  EFIAPI
>>>>  GetBestLanguage (
>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>> -  IN BOOLEAN      Iso639Language,
>>>> +  IN UINTN      Iso639Language,
>>>>    ...
>>>>    )
>>>>  {
>>>> --
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Laszlo Ersek]

On 05/19/17 04:45, Sergei Temerkhanov wrote:
> On Thu, May 18, 2017 at 1:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 05/16/17 14:10, Sergei Temerkhanov wrote:
>>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com> wrote:
>>>> Sergey:
>>>>   Could you give more detail on the undefined behavior on variadic parameters?
>>>>
>>>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this issues found in the latest CLANG tool chain. Do you find other tool chain reports it?
>>>
>>> Yes, this is exactly the bug this patch fixes.
>>>
>>> As per the C99 standard:
>>> "The parameter parmN is the identifier of the rightmost parameter in
>>> the variable parameter list in the function definition (the one just
>>> before the , ...). If the parameter parmN is declared with the
>>> register storage class, with a function or array type, or with a type
>>> that is not compatible with the type that results after application of
>>> the default argument promotions, the behavior is undefined."
>>>
>>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>>> char. It undergoes a promotion to an unsigned int
>>
>> Side topic:
>>
>> It is promoted, but not to "unsigned int".
>>
>> The standard says, in "6.3.1.1 Boolean, characters, and integers",
>> paragraph 2,
>>
>>     The following may be used in an expression wherever an /int/ or
>>     /unsigned int/ may be used:
>>
>>     — An object or expression with an integer type whose integer
>>       conversion rank is less than or equal to the rank of /int/ and
>>       /unsigned int/.
>>     — A bit-field of type /_Bool/, /int/, /signed int/, or
>>       /unsigned int/.
>>
>>     If an /int/ can represent all values of the original type, the value
>>     is converted to an /int/; otherwise, it is converted to an
>>     /unsigned int/. These are called the /integer promotions/. [...]
>>
>> On all supported edk2 platforms, "unsigned char"'s range is 0..255
>> inclusive, which can be represented by "int" (again on all supported
>> edk2 platforms). So the promotion occurs to "int", not "unsigned int"
>>
>>
>> Furthermore, in place of the suggested UINTN type (which is fine), the
>> following further types would be correct: INT32, UINT32, INT64, UINT64,
>> INTN.
> 
> On 32-bit architectures, using 64-bit types here may change the ABI. Which might
> affect some corner cases like linking precompiled object files to the
> library in question.

True.

I missed the fact that in edk2 you can have binary-only library
instances. I should have remembered, after all I had filed
<https://bugzilla.tianocore.org/show_bug.cgi?id=463> :)

So yes, UINTN is the best choice; it keeps binary compat beyond
everything else.

Thanks!
Laszlo

> 
>> The reason is that all of these map to standard C types, on all
>> edk2 platforms, whose integer conversion ranks are not less than that of
>> "int" and "unsigned int". Hence they are all unaffected by the integer
>> promotions.
>>
>> (This digression does not affect your main point, which remains correct;
>> I just wanted to be precise here, since we're quoting the standard.)
>>
>> Thanks
>> Laszlo
>>
>>> which is not a
>>> compatible type for unsigned char. Correct me if I'm wrong.
>>>
>>> Regards,
>>> Sergey
>>>
>>>>
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sergey Temerkhanov
>>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>>>> To: edk2-devel@lists.01.org
>>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>>>>
>>>>> Fix undefined behavior by avoiding parameter type promotion
>>>>>
>>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>>> ---
>>>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>>>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>>>>> index 0b14792..4e4697c 100644
>>>>> --- a/MdePkg/Include/Library/UefiLib.h
>>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>>>  EFIAPI
>>>>>  GetBestLanguage (
>>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>>> -  IN BOOLEAN      Iso639Language,
>>>>> +  IN UINTN      Iso639Language,
>>>>>    ...
>>>>>    );
>>>>>
>>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>>>>> index a7eee01..74528ec 100644
>>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>>>  EFIAPI
>>>>>  GetBestLanguage (
>>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>>> -  IN BOOLEAN      Iso639Language,
>>>>> +  IN UINTN      Iso639Language,
>>>>>    ...
>>>>>    )
>>>>>  {
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Gao, Liming]

Sergey:
  This patch updates API interface. I still need to verify its functionality on other tool chain. I will give you feedback after I am done. 

Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Friday, May 19, 2017 4:16 PM
>To: Sergei Temerkhanov <s.temerkhanov@gmail.com>
>Cc: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>parameters
>
>On 05/19/17 04:45, Sergei Temerkhanov wrote:
>> On Thu, May 18, 2017 at 1:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 05/16/17 14:10, Sergei Temerkhanov wrote:
>>>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com>
>wrote:
>>>>> Sergey:
>>>>>   Could you give more detail on the undefined behavior on variadic
>parameters?
>>>>>
>>>>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this
>issues found in the latest CLANG tool chain. Do you find other tool chain
>reports it?
>>>>
>>>> Yes, this is exactly the bug this patch fixes.
>>>>
>>>> As per the C99 standard:
>>>> "The parameter parmN is the identifier of the rightmost parameter in
>>>> the variable parameter list in the function definition (the one just
>>>> before the , ...). If the parameter parmN is declared with the
>>>> register storage class, with a function or array type, or with a type
>>>> that is not compatible with the type that results after application of
>>>> the default argument promotions, the behavior is undefined."
>>>>
>>>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>>>> char. It undergoes a promotion to an unsigned int
>>>
>>> Side topic:
>>>
>>> It is promoted, but not to "unsigned int".
>>>
>>> The standard says, in "6.3.1.1 Boolean, characters, and integers",
>>> paragraph 2,
>>>
>>>     The following may be used in an expression wherever an /int/ or
>>>     /unsigned int/ may be used:
>>>
>>>     — An object or expression with an integer type whose integer
>>>       conversion rank is less than or equal to the rank of /int/ and
>>>       /unsigned int/.
>>>     — A bit-field of type /_Bool/, /int/, /signed int/, or
>>>       /unsigned int/.
>>>
>>>     If an /int/ can represent all values of the original type, the value
>>>     is converted to an /int/; otherwise, it is converted to an
>>>     /unsigned int/. These are called the /integer promotions/. [...]
>>>
>>> On all supported edk2 platforms, "unsigned char"'s range is 0..255
>>> inclusive, which can be represented by "int" (again on all supported
>>> edk2 platforms). So the promotion occurs to "int", not "unsigned int"
>>>
>>>
>>> Furthermore, in place of the suggested UINTN type (which is fine), the
>>> following further types would be correct: INT32, UINT32, INT64, UINT64,
>>> INTN.
>>
>> On 32-bit architectures, using 64-bit types here may change the ABI. Which
>might
>> affect some corner cases like linking precompiled object files to the
>> library in question.
>
>True.
>
>I missed the fact that in edk2 you can have binary-only library
>instances. I should have remembered, after all I had filed
><https://bugzilla.tianocore.org/show_bug.cgi?id=463> :)
>
>So yes, UINTN is the best choice; it keeps binary compat beyond
>everything else.
>
>Thanks!
>Laszlo
>
>>
>>> The reason is that all of these map to standard C types, on all
>>> edk2 platforms, whose integer conversion ranks are not less than that of
>>> "int" and "unsigned int". Hence they are all unaffected by the integer
>>> promotions.
>>>
>>> (This digression does not affect your main point, which remains correct;
>>> I just wanted to be precise here, since we're quoting the standard.)
>>>
>>> Thanks
>>> Laszlo
>>>
>>>> which is not a
>>>> compatible type for unsigned char. Correct me if I'm wrong.
>>>>
>>>> Regards,
>>>> Sergey
>>>>
>>>>>
>>>>> Thanks
>>>>> Liming
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>Of Sergey Temerkhanov
>>>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>>>>>> To: edk2-devel@lists.01.org
>>>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>parameters
>>>>>>
>>>>>> Fix undefined behavior by avoiding parameter type promotion
>>>>>>
>>>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>>>> ---
>>>>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>>>>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/MdePkg/Include/Library/UefiLib.h
>b/MdePkg/Include/Library/UefiLib.h
>>>>>> index 0b14792..4e4697c 100644
>>>>>> --- a/MdePkg/Include/Library/UefiLib.h
>>>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>>>>>> @@ -818,7 +818,7 @@ CHAR8 *
>>>>>>  EFIAPI
>>>>>>  GetBestLanguage (
>>>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>>>> -  IN BOOLEAN      Iso639Language,
>>>>>> +  IN UINTN      Iso639Language,
>>>>>>    ...
>>>>>>    );
>>>>>>
>>>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
>b/MdePkg/Library/UefiLib/UefiLib.c
>>>>>> index a7eee01..74528ec 100644
>>>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>>>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>>>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>>>>>>  EFIAPI
>>>>>>  GetBestLanguage (
>>>>>>    IN CONST CHAR8  *SupportedLanguages,
>>>>>> -  IN BOOLEAN      Iso639Language,
>>>>>> +  IN UINTN      Iso639Language,
>>>>>>    ...
>>>>>>    )
>>>>>>  {
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> edk2-devel mailing list
>>>>>> edk2-devel@lists.01.org
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>
>>>

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Kinney, Michael D]

Sergey,

We also need to do a full search for use of variadic parameters to make 
sure we have a solution for all of them and update the EDK II C coding
standard to make sure the rules for use of variadic parameters for 
maximum compiler compatibility are captured correctly.

Did you try Andrew Fish's suggestion to add -Wno-varargs to the components
that have this issue to see if that is a temporary workaround for your
specific build failures?

Thanks,

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Gao, Liming
> Sent: Tuesday, May 23, 2017 7:14 PM
> To: 'Laszlo Ersek' <lersek@redhat.com>; Sergei Temerkhanov <s.temerkhanov@gmail.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
> 
> Sergey:
>   This patch updates API interface. I still need to verify its functionality on other
> tool chain. I will give you feedback after I am done.
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: Laszlo Ersek [mailto:lersek@redhat.com]
> >Sent: Friday, May 19, 2017 4:16 PM
> >To: Sergei Temerkhanov <s.temerkhanov@gmail.com>
> >Cc: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> >Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
> >parameters
> >
> >On 05/19/17 04:45, Sergei Temerkhanov wrote:
> >> On Thu, May 18, 2017 at 1:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> >>> On 05/16/17 14:10, Sergei Temerkhanov wrote:
> >>>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com>
> >wrote:
> >>>>> Sergey:
> >>>>>   Could you give more detail on the undefined behavior on variadic
> >parameters?
> >>>>>
> >>>>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this
> >issues found in the latest CLANG tool chain. Do you find other tool chain
> >reports it?
> >>>>
> >>>> Yes, this is exactly the bug this patch fixes.
> >>>>
> >>>> As per the C99 standard:
> >>>> "The parameter parmN is the identifier of the rightmost parameter in
> >>>> the variable parameter list in the function definition (the one just
> >>>> before the , ...). If the parameter parmN is declared with the
> >>>> register storage class, with a function or array type, or with a type
> >>>> that is not compatible with the type that results after application of
> >>>> the default argument promotions, the behavior is undefined."
> >>>>
> >>>> That's exactly the case here since BOOLEAN is a typedef for unsigned
> >>>> char. It undergoes a promotion to an unsigned int
> >>>
> >>> Side topic:
> >>>
> >>> It is promoted, but not to "unsigned int".
> >>>
> >>> The standard says, in "6.3.1.1 Boolean, characters, and integers",
> >>> paragraph 2,
> >>>
> >>>     The following may be used in an expression wherever an /int/ or
> >>>     /unsigned int/ may be used:
> >>>
> >>>     — An object or expression with an integer type whose integer
> >>>       conversion rank is less than or equal to the rank of /int/ and
> >>>       /unsigned int/.
> >>>     — A bit-field of type /_Bool/, /int/, /signed int/, or
> >>>       /unsigned int/.
> >>>
> >>>     If an /int/ can represent all values of the original type, the value
> >>>     is converted to an /int/; otherwise, it is converted to an
> >>>     /unsigned int/. These are called the /integer promotions/. [...]
> >>>
> >>> On all supported edk2 platforms, "unsigned char"'s range is 0..255
> >>> inclusive, which can be represented by "int" (again on all supported
> >>> edk2 platforms). So the promotion occurs to "int", not "unsigned int"
> >>>
> >>>
> >>> Furthermore, in place of the suggested UINTN type (which is fine), the
> >>> following further types would be correct: INT32, UINT32, INT64, UINT64,
> >>> INTN.
> >>
> >> On 32-bit architectures, using 64-bit types here may change the ABI. Which
> >might
> >> affect some corner cases like linking precompiled object files to the
> >> library in question.
> >
> >True.
> >
> >I missed the fact that in edk2 you can have binary-only library
> >instances. I should have remembered, after all I had filed
> ><https://bugzilla.tianocore.org/show_bug.cgi?id=463> :)
> >
> >So yes, UINTN is the best choice; it keeps binary compat beyond
> >everything else.
> >
> >Thanks!
> >Laszlo
> >
> >>
> >>> The reason is that all of these map to standard C types, on all
> >>> edk2 platforms, whose integer conversion ranks are not less than that of
> >>> "int" and "unsigned int". Hence they are all unaffected by the integer
> >>> promotions.
> >>>
> >>> (This digression does not affect your main point, which remains correct;
> >>> I just wanted to be precise here, since we're quoting the standard.)
> >>>
> >>> Thanks
> >>> Laszlo
> >>>
> >>>> which is not a
> >>>> compatible type for unsigned char. Correct me if I'm wrong.
> >>>>
> >>>> Regards,
> >>>> Sergey
> >>>>
> >>>>>
> >>>>> Thanks
> >>>>> Liming
> >>>>>> -----Original Message-----
> >>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >Of Sergey Temerkhanov
> >>>>>> Sent: Tuesday, May 16, 2017 10:57 AM
> >>>>>> To: edk2-devel@lists.01.org
> >>>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
> >parameters
> >>>>>>
> >>>>>> Fix undefined behavior by avoiding parameter type promotion
> >>>>>>
> >>>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> >>>>>> ---
> >>>>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
> >>>>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
> >>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/MdePkg/Include/Library/UefiLib.h
> >b/MdePkg/Include/Library/UefiLib.h
> >>>>>> index 0b14792..4e4697c 100644
> >>>>>> --- a/MdePkg/Include/Library/UefiLib.h
> >>>>>> +++ b/MdePkg/Include/Library/UefiLib.h
> >>>>>> @@ -818,7 +818,7 @@ CHAR8 *
> >>>>>>  EFIAPI
> >>>>>>  GetBestLanguage (
> >>>>>>    IN CONST CHAR8  *SupportedLanguages,
> >>>>>> -  IN BOOLEAN      Iso639Language,
> >>>>>> +  IN UINTN      Iso639Language,
> >>>>>>    ...
> >>>>>>    );
> >>>>>>
> >>>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
> >b/MdePkg/Library/UefiLib/UefiLib.c
> >>>>>> index a7eee01..74528ec 100644
> >>>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
> >>>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> >>>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
> >>>>>>  EFIAPI
> >>>>>>  GetBestLanguage (
> >>>>>>    IN CONST CHAR8  *SupportedLanguages,
> >>>>>> -  IN BOOLEAN      Iso639Language,
> >>>>>> +  IN UINTN      Iso639Language,
> >>>>>>    ...
> >>>>>>    )
> >>>>>>  {
> >>>>>> --
> >>>>>> 2.7.4
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> edk2-devel mailing list
> >>>>>> edk2-devel@lists.01.org
> >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>>> _______________________________________________
> >>>> edk2-devel mailing list
> >>>> edk2-devel@lists.01.org
> >>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>>>
> >>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters

[Sergei Temerkhanov]

On Wed, May 24, 2017 at 5:46 AM, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> Sergey,
>
> We also need to do a full search for use of variadic parameters to make
> sure we have a solution for all of them and update the EDK II C coding
> standard to make sure the rules for use of variadic parameters for
> maximum compiler compatibility are captured correctly.

I've sent another patch for MdePkg with a similar change but there
might be more.

>
> Did you try Andrew Fish's suggestion to add -Wno-varargs to the components
> that have this issue to see if that is a temporary workaround for your
> specific build failures?

For my projects, using these 2 changes have been sufficient .

>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Gao, Liming
>> Sent: Tuesday, May 23, 2017 7:14 PM
>> To: 'Laszlo Ersek' <lersek@redhat.com>; Sergei Temerkhanov <s.temerkhanov@gmail.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters
>>
>> Sergey:
>>   This patch updates API interface. I still need to verify its functionality on other
>> tool chain. I will give you feedback after I am done.
>>
>> Thanks
>> Liming
>> >-----Original Message-----
>> >From: Laszlo Ersek [mailto:lersek@redhat.com]
>> >Sent: Friday, May 19, 2017 4:16 PM
>> >To: Sergei Temerkhanov <s.temerkhanov@gmail.com>
>> >Cc: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>> >Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>> >parameters
>> >
>> >On 05/19/17 04:45, Sergei Temerkhanov wrote:
>> >> On Thu, May 18, 2017 at 1:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> >>> On 05/16/17 14:10, Sergei Temerkhanov wrote:
>> >>>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming <liming.gao@intel.com>
>> >wrote:
>> >>>>> Sergey:
>> >>>>>   Could you give more detail on the undefined behavior on variadic
>> >parameters?
>> >>>>>
>> >>>>>   I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this
>> >issues found in the latest CLANG tool chain. Do you find other tool chain
>> >reports it?
>> >>>>
>> >>>> Yes, this is exactly the bug this patch fixes.
>> >>>>
>> >>>> As per the C99 standard:
>> >>>> "The parameter parmN is the identifier of the rightmost parameter in
>> >>>> the variable parameter list in the function definition (the one just
>> >>>> before the , ...). If the parameter parmN is declared with the
>> >>>> register storage class, with a function or array type, or with a type
>> >>>> that is not compatible with the type that results after application of
>> >>>> the default argument promotions, the behavior is undefined."
>> >>>>
>> >>>> That's exactly the case here since BOOLEAN is a typedef for unsigned
>> >>>> char. It undergoes a promotion to an unsigned int
>> >>>
>> >>> Side topic:
>> >>>
>> >>> It is promoted, but not to "unsigned int".
>> >>>
>> >>> The standard says, in "6.3.1.1 Boolean, characters, and integers",
>> >>> paragraph 2,
>> >>>
>> >>>     The following may be used in an expression wherever an /int/ or
>> >>>     /unsigned int/ may be used:
>> >>>
>> >>>     — An object or expression with an integer type whose integer
>> >>>       conversion rank is less than or equal to the rank of /int/ and
>> >>>       /unsigned int/.
>> >>>     — A bit-field of type /_Bool/, /int/, /signed int/, or
>> >>>       /unsigned int/.
>> >>>
>> >>>     If an /int/ can represent all values of the original type, the value
>> >>>     is converted to an /int/; otherwise, it is converted to an
>> >>>     /unsigned int/. These are called the /integer promotions/. [...]
>> >>>
>> >>> On all supported edk2 platforms, "unsigned char"'s range is 0..255
>> >>> inclusive, which can be represented by "int" (again on all supported
>> >>> edk2 platforms). So the promotion occurs to "int", not "unsigned int"
>> >>>
>> >>>
>> >>> Furthermore, in place of the suggested UINTN type (which is fine), the
>> >>> following further types would be correct: INT32, UINT32, INT64, UINT64,
>> >>> INTN.
>> >>
>> >> On 32-bit architectures, using 64-bit types here may change the ABI. Which
>> >might
>> >> affect some corner cases like linking precompiled object files to the
>> >> library in question.
>> >
>> >True.
>> >
>> >I missed the fact that in edk2 you can have binary-only library
>> >instances. I should have remembered, after all I had filed
>> ><https://bugzilla.tianocore.org/show_bug.cgi?id=463> :)
>> >
>> >So yes, UINTN is the best choice; it keeps binary compat beyond
>> >everything else.
>> >
>> >Thanks!
>> >Laszlo
>> >
>> >>
>> >>> The reason is that all of these map to standard C types, on all
>> >>> edk2 platforms, whose integer conversion ranks are not less than that of
>> >>> "int" and "unsigned int". Hence they are all unaffected by the integer
>> >>> promotions.
>> >>>
>> >>> (This digression does not affect your main point, which remains correct;
>> >>> I just wanted to be precise here, since we're quoting the standard.)
>> >>>
>> >>> Thanks
>> >>> Laszlo
>> >>>
>> >>>> which is not a
>> >>>> compatible type for unsigned char. Correct me if I'm wrong.
>> >>>>
>> >>>> Regards,
>> >>>> Sergey
>> >>>>
>> >>>>>
>> >>>>> Thanks
>> >>>>> Liming
>> >>>>>> -----Original Message-----
>> >>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>> >Of Sergey Temerkhanov
>> >>>>>> Sent: Tuesday, May 16, 2017 10:57 AM
>> >>>>>> To: edk2-devel@lists.01.org
>> >>>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic
>> >parameters
>> >>>>>>
>> >>>>>> Fix undefined behavior by avoiding parameter type promotion
>> >>>>>>
>> >>>>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> >>>>>> ---
>> >>>>>>  MdePkg/Include/Library/UefiLib.h | 2 +-
>> >>>>>>  MdePkg/Library/UefiLib/UefiLib.c | 2 +-
>> >>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/MdePkg/Include/Library/UefiLib.h
>> >b/MdePkg/Include/Library/UefiLib.h
>> >>>>>> index 0b14792..4e4697c 100644
>> >>>>>> --- a/MdePkg/Include/Library/UefiLib.h
>> >>>>>> +++ b/MdePkg/Include/Library/UefiLib.h
>> >>>>>> @@ -818,7 +818,7 @@ CHAR8 *
>> >>>>>>  EFIAPI
>> >>>>>>  GetBestLanguage (
>> >>>>>>    IN CONST CHAR8  *SupportedLanguages,
>> >>>>>> -  IN BOOLEAN      Iso639Language,
>> >>>>>> +  IN UINTN      Iso639Language,
>> >>>>>>    ...
>> >>>>>>    );
>> >>>>>>
>> >>>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
>> >b/MdePkg/Library/UefiLib/UefiLib.c
>> >>>>>> index a7eee01..74528ec 100644
>> >>>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>> >>>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>> >>>>>> @@ -1514,7 +1514,7 @@ CHAR8 *
>> >>>>>>  EFIAPI
>> >>>>>>  GetBestLanguage (
>> >>>>>>    IN CONST CHAR8  *SupportedLanguages,
>> >>>>>> -  IN BOOLEAN      Iso639Language,
>> >>>>>> +  IN UINTN      Iso639Language,
>> >>>>>>    ...
>> >>>>>>    )
>> >>>>>>  {
>> >>>>>> --
>> >>>>>> 2.7.4
>> >>>>>>
>> >>>>>> _______________________________________________
>> >>>>>> edk2-devel mailing list
>> >>>>>> edk2-devel@lists.01.org
>> >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> >>>> _______________________________________________
>> >>>> edk2-devel mailing list
>> >>>> edk2-devel@lists.01.org
>> >>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> >>>>
>> >>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH] MdeModulePkg: Fix undefined behavior on variadic parameters

[Sergey Temerkhanov]

Fix undefined behavior by avoiding automatic type promotion

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 0a325de..03605b9 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -1611,7 +1611,7 @@ CHAR8 *
 EFIAPI
 VariableGetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages,
-  IN BOOLEAN      Iso639Language,
+  IN UINTN      Iso639Language,
   ...
   )
 {
-- 
2.7.4

Re: [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[Ard Biesheuvel]

On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
> These registers are reserved for PPIs and unimplemented on
> some architectures
>


What do you mean by 'architectures'? Could you elaborate on which SoC
needs this?

> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> ---
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 8af97a9..dc6b896 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>    }
>
> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>
>      // Set Priority
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[Sergei Temerkhanov]

On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
>> These registers are reserved for PPIs and unimplemented on
>> some architectures
>>
>
>
> What do you mean by 'architectures'?

GIC core implementations.

> Could you elaborate on which SoC
> needs this?
At least Cavium ThunderX/OcteonTX need it.

The GICv3 spec says this on the subject:

8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254

"These registers are always used when affinity routing is not enabled.
When affinity routing is enabled for the Security state of an
interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
where n = 0 to 7."

>
>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> ---
>>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> index 8af97a9..dc6b896 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
>>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>>    }
>>
>> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
>> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
>>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>>
>>      // Set Priority
>> --
>> 2.7.4
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[Leif Lindholm]

On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
> >> These registers are reserved for PPIs and unimplemented on
> >> some architectures
> >>
> >
> >
> > What do you mean by 'architectures'?
> 
> GIC core implementations.
> 
> > Could you elaborate on which SoC
> > needs this?
> At least Cavium ThunderX/OcteonTX need it.
> 
> The GICv3 spec says this on the subject:
> 
> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
> 
> "These registers are always used when affinity routing is not enabled.
> When affinity routing is enabled for the Security state of an
> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
> where n = 0 to 7."

Since they are RAZ/WI, why is this change needed?

/
    Leif

> >
> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> >> ---
> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> index 8af97a9..dc6b896 100644
> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
> >>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
> >>    }
> >>
> >> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> >> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
> >>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
> >>
> >>      // Set Priority
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[Sergei Temerkhanov]

On Fri, May 19, 2017 at 1:26 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
>> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
>> >> These registers are reserved for PPIs and unimplemented on
>> >> some architectures
>> >>
>> >
>> >
>> > What do you mean by 'architectures'?
>>
>> GIC core implementations.
>>
>> > Could you elaborate on which SoC
>> > needs this?
>> At least Cavium ThunderX/OcteonTX need it.
>>
>> The GICv3 spec says this on the subject:
>>
>> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
>>
>> "These registers are always used when affinity routing is not enabled.
>> When affinity routing is enabled for the Security state of an
>> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
>> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
>> where n = 0 to 7."
>
> Since they are RAZ/WI, why is this change needed?

B/c for some GICv3 cores accessing these registers result in exceptions

Regards,
Sergey

>
> /
>     Leif
>
>> >
>> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> >> ---
>> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> >> index 8af97a9..dc6b896 100644
>> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> >> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
>> >>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>> >>    }
>> >>
>> >> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
>> >> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
>> >>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>> >>
>> >>      // Set Priority
>> >> --
>> >> 2.7.4
>> >>
>> >> _______________________________________________
>> >> edk2-devel mailing list
>> >> edk2-devel@lists.01.org
>> >> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[Leif Lindholm]

On Fri, May 19, 2017 at 05:31:36PM +0300, Sergei Temerkhanov wrote:
> On Fri, May 19, 2017 at 1:26 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
> >> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org> wrote:
> >> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
> >> >> These registers are reserved for PPIs and unimplemented on
> >> >> some architectures
> >> >>
> >> >
> >> >
> >> > What do you mean by 'architectures'?
> >>
> >> GIC core implementations.
> >>
> >> > Could you elaborate on which SoC
> >> > needs this?
> >> At least Cavium ThunderX/OcteonTX need it.
> >>
> >> The GICv3 spec says this on the subject:
> >>
> >> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
> >>
> >> "These registers are always used when affinity routing is not enabled.
> >> When affinity routing is enabled for the Security state of an
> >> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
> >> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
> >> where n = 0 to 7."
> >
> > Since they are RAZ/WI, why is this change needed?
> 
> B/c for some GICv3 cores accessing these registers result in exceptions

Right, so that's architecturally non-compliant.

At which point, as a workaround for a hardware erratum, I think this
should either be a configurable option (ArmPkg Pcd), or the code
should be modified to use GICR_IPRIORITYR as the text describes (if
applicable).

Regards,

Leif

> Regards,
> Sergey
> 
> >
> > /
> >     Leif
> >
> >> >
> >> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> >> >> ---
> >> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> >> index 8af97a9..dc6b896 100644
> >> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> >> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
> >> >>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
> >> >>    }
> >> >>
> >> >> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> >> >> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
> >> >>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
> >> >>
> >> >>      // Set Priority
> >> >> --
> >> >> 2.7.4
> >> >>
> >> >> _______________________________________________
> >> >> edk2-devel mailing list
> >> >> edk2-devel@lists.01.org
> >> >> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[Ard Biesheuvel]

On 22 May 2017 at 10:45, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, May 19, 2017 at 05:31:36PM +0300, Sergei Temerkhanov wrote:
>> On Fri, May 19, 2017 at 1:26 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
>> >> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
>> >> <ard.biesheuvel@linaro.org> wrote:
>> >> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
>> >> >> These registers are reserved for PPIs and unimplemented on
>> >> >> some architectures
>> >> >>
>> >> >
>> >> >
>> >> > What do you mean by 'architectures'?
>> >>
>> >> GIC core implementations.
>> >>
>> >> > Could you elaborate on which SoC
>> >> > needs this?
>> >> At least Cavium ThunderX/OcteonTX need it.
>> >>
>> >> The GICv3 spec says this on the subject:
>> >>
>> >> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
>> >>
>> >> "These registers are always used when affinity routing is not enabled.
>> >> When affinity routing is enabled for the Security state of an
>> >> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
>> >> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
>> >> where n = 0 to 7."
>> >
>> > Since they are RAZ/WI, why is this change needed?
>>
>> B/c for some GICv3 cores accessing these registers result in exceptions
>
> Right, so that's architecturally non-compliant.
>
> At which point, as a workaround for a hardware erratum, I think this
> should either be a configurable option (ArmPkg Pcd), or the code
> should be modified to use GICR_IPRIORITYR as the text describes (if
> applicable).
>

That is a good point. The intention of the code is to disable all
interrupt sources, so it is incorrect to begin with, given that PPIs
and SGIs need to be disabled at the respective redistributor of each
CPU. Fix that, and it should no longer raise exceptions on the broken
hardware.

-- 
Ard.