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

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

> -----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