* [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values. @ 2018-02-27 16:49 Marvin Häuser 2018-02-27 18:42 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Marvin Häuser @ 2018-02-27 16:49 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: star.zeng@intel.com, eric.dong@intel.com, ruiyu.ni@intel.com 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values. 2018-02-27 16:49 [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values Marvin Häuser @ 2018-02-27 18:42 ` Laszlo Ersek 2018-02-27 18:50 ` Andrew Fish 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2018-02-27 18:42 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org Cc: ruiyu.ni@intel.com, eric.dong@intel.com, star.zeng@intel.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values. 2018-02-27 18:42 ` Laszlo Ersek @ 2018-02-27 18:50 ` Andrew Fish 2018-02-27 18:57 ` Marvin Häuser 2018-02-28 11:42 ` Laszlo Ersek 0 siblings, 2 replies; 6+ messages in thread From: Andrew Fish @ 2018-02-27 18:50 UTC (permalink / raw) To: Laszlo Ersek Cc: Marvin Häuser, edk2-devel@lists.01.org, ruiyu.ni@intel.com, eric.dong@intel.com, star.zeng@intel.com > 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values. 2018-02-27 18:50 ` Andrew Fish @ 2018-02-27 18:57 ` Marvin Häuser 2018-02-28 11:42 ` Laszlo Ersek 1 sibling, 0 replies; 6+ messages in thread From: Marvin Häuser @ 2018-02-27 18:57 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: ruiyu.ni@intel.com, eric.dong@intel.com, star.zeng@intel.com, afish@apple.com, lersek@redhat.com 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values. 2018-02-27 18:50 ` Andrew Fish 2018-02-27 18:57 ` Marvin Häuser @ 2018-02-28 11:42 ` Laszlo Ersek 2018-02-28 11:48 ` Marvin Häuser 1 sibling, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2018-02-28 11:42 UTC (permalink / raw) To: Andrew Fish Cc: Marvin Häuser, edk2-devel@lists.01.org, ruiyu.ni@intel.com, eric.dong@intel.com, star.zeng@intel.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values. 2018-02-28 11:42 ` Laszlo Ersek @ 2018-02-28 11:48 ` Marvin Häuser 0 siblings, 0 replies; 6+ messages in thread From: Marvin Häuser @ 2018-02-28 11:48 UTC (permalink / raw) To: edk2-devel@lists.01.org, Laszlo Ersek Cc: ruiyu.ni@intel.com, eric.dong@intel.com, star.zeng@intel.com, afish@apple.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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-28 11:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-27 16:49 [PATCH 1/2] MdeModulePkg/PciBusDxe: Prevent truncating constant values Marvin Häuser 2018-02-27 18:42 ` Laszlo Ersek 2018-02-27 18:50 ` Andrew Fish 2018-02-27 18:57 ` Marvin Häuser 2018-02-28 11:42 ` Laszlo Ersek 2018-02-28 11:48 ` Marvin Häuser
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox