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

[Laszlo Ersek <lersek@redhat.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