[PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.

[PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.

[Marvin Häuser]

The toolcahin VS2015x86 issues warnings when truncating constant
values. Explicitely cast such to avoid it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index 2f713fcee95e..a752853f3e9e 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -1936,7 +1936,7 @@ ProgramP2C (
                    &BridgeControl
                    );
 
-      BridgeControl &= (UINT16) ~PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
+      BridgeControl &= (UINT16) ~(UINT16)PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
       PciIo->Pci.Write (
                    PciIo,
                    EfiPciIoWidthUint16,
@@ -2005,7 +2005,7 @@ ProgramP2C (
                    &BridgeControl
                    );
 
-      BridgeControl &= (UINT16) ~(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
+      BridgeControl &= (UINT16) ~(UINT16)(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
       PciIo->Pci.Write (
                    PciIo,
                    EfiPciIoWidthUint16,
-- 
2.16.0.windows.2

Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.

[Laszlo Ersek]

On 02/27/18 17:49, Marvin Häuser wrote:
> The toolcahin VS2015x86 issues warnings when truncating constant
> values. Explicitely cast such to avoid it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index 2f713fcee95e..a752853f3e9e 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -1936,7 +1936,7 @@ ProgramP2C (
>                     &BridgeControl
>                     );
>  
> -      BridgeControl &= (UINT16) ~PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
> +      BridgeControl &= (UINT16) ~(UINT16)PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
>        PciIo->Pci.Write (
>                     PciIo,
>                     EfiPciIoWidthUint16,
> @@ -2005,7 +2005,7 @@ ProgramP2C (
>                     &BridgeControl
>                     );
>  
> -      BridgeControl &= (UINT16) ~(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
> +      BridgeControl &= (UINT16) ~(UINT16)(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
>        PciIo->Pci.Write (
>                     PciIo,
>                     EfiPciIoWidthUint16,
> 

My recommendation is the same as for:

  [edk2] [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent
                     truncating constant values.

#define PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE BIT8
#define PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE BIT9

#define  BIT8     0x00000100
#define  BIT9     0x00000200

So I suggest (UINT32) for the casts.


I'd also suggest a better subject line / commit message. Can we say:

  Package/Module: avoid bit-negating signed integer constants

and then the commit message could quote the VS2015x86 warnings.

Again, just my two cents; I defer to the MdeModulePkg maintainers.

Thanks!
Laszlo

Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.

[Andrew Fish]

> On Feb 27, 2018, at 10:42 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 02/27/18 17:49, Marvin Häuser wrote:
>> The toolcahin VS2015x86 issues warnings when truncating constant
>> values. Explicitely cast such to avoid it.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
>> ---
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> index 2f713fcee95e..a752853f3e9e 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> @@ -1936,7 +1936,7 @@ ProgramP2C (
>>                    &BridgeControl
>>                    );
>> 
>> -      BridgeControl &= (UINT16) ~PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
>> +      BridgeControl &= (UINT16) ~(UINT16)PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
>>       PciIo->Pci.Write (
>>                    PciIo,
>>                    EfiPciIoWidthUint16,
>> @@ -2005,7 +2005,7 @@ ProgramP2C (
>>                    &BridgeControl
>>                    );
>> 
>> -      BridgeControl &= (UINT16) ~(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
>> +      BridgeControl &= (UINT16) ~(UINT16)(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
>>       PciIo->Pci.Write (
>>                    PciIo,
>>                    EfiPciIoWidthUint16,
>> 
> 
> My recommendation is the same as for:
> 
>  [edk2] [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent
>                     truncating constant values.
> 
> #define PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE BIT8
> #define PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE BIT9
> 
> #define  BIT8     0x00000100
> #define  BIT9     0x00000200
> 

Laszlo,

Stupid question? Would making BIT8 0x00000100U help? I notice we use ULL for the larger ones, and I don't remember why we don't use U for the ones that fit into a int?

Thanks,

Andrew Fish

> So I suggest (UINT32) for the casts.
> 
> 
> I'd also suggest a better subject line / commit message. Can we say:
> 
>  Package/Module: avoid bit-negating signed integer constants
> 
> and then the commit message could quote the VS2015x86 warnings.
> 
> Again, just my two cents; I defer to the MdeModulePkg maintainers.
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.

[Marvin Häuser]

Hello Andrew,

Thanks for your input. I already submitted a patch to do that: "[edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations."
Would be great if you took a look.

Thanks,
Marvin.

From: afish@apple.com <afish@apple.com>
Sent: Tuesday, February 27, 2018 7:50 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org; ruiyu.ni@intel.com; eric.dong@intel.com; star.zeng@intel.com
Subject: Re: [edk2] [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.




On Feb 27, 2018, at 10:42 AM, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:

On 02/27/18 17:49, Marvin Häuser wrote:

The toolcahin VS2015x86 issues warnings when truncating constant
values. Explicitely cast such to avoid it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com<mailto:Marvin.Haeuser@outlook.com>>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index 2f713fcee95e..a752853f3e9e 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -1936,7 +1936,7 @@ ProgramP2C (
                   &BridgeControl
                   );

-      BridgeControl &= (UINT16) ~PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
+      BridgeControl &= (UINT16) ~(UINT16)PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
      PciIo->Pci.Write (
                   PciIo,
                   EfiPciIoWidthUint16,
@@ -2005,7 +2005,7 @@ ProgramP2C (
                   &BridgeControl
                   );

-      BridgeControl &= (UINT16) ~(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
+      BridgeControl &= (UINT16) ~(UINT16)(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
      PciIo->Pci.Write (
                   PciIo,
                   EfiPciIoWidthUint16,

My recommendation is the same as for:

 [edk2] [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent
                    truncating constant values.

#define PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE BIT8
#define PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE BIT9

#define  BIT8     0x00000100
#define  BIT9     0x00000200


Laszlo,

Stupid question? Would making BIT8 0x00000100U help? I notice we use ULL for the larger ones, and I don't remember why we don't use U for the ones that fit into a int?

Thanks,

Andrew Fish


So I suggest (UINT32) for the casts.


I'd also suggest a better subject line / commit message. Can we say:

 Package/Module: avoid bit-negating signed integer constants

and then the commit message could quote the VS2015x86 warnings.

Again, just my two cents; I defer to the MdeModulePkg maintainers.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.

[Laszlo Ersek]

On 02/27/18 19:50, Andrew Fish wrote:
> 
> 
>> On Feb 27, 2018, at 10:42 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 02/27/18 17:49, Marvin Häuser wrote:
>>> The toolcahin VS2015x86 issues warnings when truncating constant
>>> values. Explicitely cast such to avoid it.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
>>> ---
>>> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>>> index 2f713fcee95e..a752853f3e9e 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>>> @@ -1936,7 +1936,7 @@ ProgramP2C (
>>>                    &BridgeControl
>>>                    );
>>>
>>> -      BridgeControl &= (UINT16) ~PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
>>> +      BridgeControl &= (UINT16) ~(UINT16)PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
>>>       PciIo->Pci.Write (
>>>                    PciIo,
>>>                    EfiPciIoWidthUint16,
>>> @@ -2005,7 +2005,7 @@ ProgramP2C (
>>>                    &BridgeControl
>>>                    );
>>>
>>> -      BridgeControl &= (UINT16) ~(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
>>> +      BridgeControl &= (UINT16) ~(UINT16)(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
>>>       PciIo->Pci.Write (
>>>                    PciIo,
>>>                    EfiPciIoWidthUint16,
>>>
>>
>> My recommendation is the same as for:
>>
>>  [edk2] [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent
>>                     truncating constant values.
>>
>> #define PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE BIT8
>> #define PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE BIT9
>>
>> #define  BIT8     0x00000100
>> #define  BIT9     0x00000200
>>
> 
> Laszlo,
> 
> Stupid question? Would making BIT8 0x00000100U help? I notice we use ULL for the larger ones, and I don't remember why we don't use U for the ones that fit into a int?

I don't know why BIT[0..31] don't use the U suffix from the beginning. I
agree they should have. Adding the suffixes now is the right thing in
theory, although some "clever" applications of those macros could
regress as a result.

Regarding the question whether this patch would be obviated by making
BIT[0..31] unsigned, Marvin explained elsewhere that just making
BIT[0..31] unsigned didn't help with suppressing the warning. As I
commented under Marvin's explanation, I understand neither why the
compiler warns about the current code (with or without making BIT[0..31]
unsigned), nor -- assuming the original warning is somehow justifiable
-- why the warning is apparently silenced by inserting a UINT16 cast
ahead of the bit-neg. As far as I could see, if the warning applied to
the original code, it should have applied to the patched code too.

Thanks
Laszlo

Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values.

[Marvin Häuser]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 28, 2018 12:42 PM
> To: Andrew Fish <afish@apple.com>
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org; ruiyu.ni@intel.com; eric.dong@intel.com;
> star.zeng@intel.com
> Subject: Re: [edk2] [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent
> truncating constant values.
> 
> On 02/27/18 19:50, Andrew Fish wrote:
> >
> >
> >> On Feb 27, 2018, at 10:42 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 02/27/18 17:49, Marvin Häuser wrote:
> >>> The toolcahin VS2015x86 issues warnings when truncating constant
> >>> values. Explicitely cast such to avoid it.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> >>> ---
> >>> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >>> index 2f713fcee95e..a752853f3e9e 100644
> >>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> >>> @@ -1936,7 +1936,7 @@ ProgramP2C (
> >>>                    &BridgeControl
> >>>                    );
> >>>
> >>> -      BridgeControl &= (UINT16)
> ~PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
> >>> +      BridgeControl &= (UINT16)
> >>> + ~(UINT16)PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE;
> >>>       PciIo->Pci.Write (
> >>>                    PciIo,
> >>>                    EfiPciIoWidthUint16, @@ -2005,7 +2005,7 @@
> >>> ProgramP2C (
> >>>                    &BridgeControl
> >>>                    );
> >>>
> >>> -      BridgeControl &= (UINT16)
> ~(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
> >>> +      BridgeControl &= (UINT16)
> >>> + ~(UINT16)(PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE);
> >>>       PciIo->Pci.Write (
> >>>                    PciIo,
> >>>                    EfiPciIoWidthUint16,
> >>>
> >>
> >> My recommendation is the same as for:
> >>
> >>  [edk2] [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent
> >>                     truncating constant values.
> >>
> >> #define PCI_CARD_PREFETCHABLE_MEMORY_0_ENABLE BIT8 #define
> >> PCI_CARD_PREFETCHABLE_MEMORY_1_ENABLE BIT9
> >>
> >> #define  BIT8     0x00000100
> >> #define  BIT9     0x00000200
> >>
> >
> > Laszlo,
> >
> > Stupid question? Would making BIT8 0x00000100U help? I notice we use
> ULL for the larger ones, and I don't remember why we don't use U for the
> ones that fit into a int?
> 
> I don't know why BIT[0..31] don't use the U suffix from the beginning. I agree
> they should have. Adding the suffixes now is the right thing in theory,
> although some "clever" applications of those macros could regress as a
> result.
> 
> Regarding the question whether this patch would be obviated by making
> BIT[0..31] unsigned, Marvin explained elsewhere that just making BIT[0..31]
> unsigned didn't help with suppressing the warning. As I commented under
> Marvin's explanation, I understand neither why the compiler warns about
> the current code (with or without making BIT[0..31] unsigned), nor --
> assuming the original warning is somehow justifiable
> -- why the warning is apparently silenced by inserting a UINT16 cast ahead of
> the bit-neg. As far as I could see, if the warning applied to the original code, it
> should have applied to the patched code too.

Please check my new reply to the Base.h patch. Once we find a decent concept for a v2, I will undo these patches locally and try to find out what MSVC's problem is more carefully.
Either way, I agree that the warning, in this context, doesn't make any sense. If I did not happen to make any mistake or the warning plainly persists after v2, maybe MS should be contacted.

Best regards,
Marvin.

> 
> Thanks
> Laszlo