Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Make bitwise operands of the same size

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, June 09, 2017 4:40 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Yao, Jiewen
> Subject: Re: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Make bitwise
> operands of the same size
> 
> On 06/08/17 10:22, Hao Wu wrote:
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > index a0e7e5b6f2..8e4f032772 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> > @@ -1349,7 +1349,7 @@ RootBridgeIoAllocateBuffer (
> >        //
> >        // Clear DUAL_ADDRESS_CYCLE
> >        //
> > -      Attributes &= ~EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
> > +      Attributes &= ~((UINT64) EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE);
> >      }
> >      Status = mIoMmuProtocol->AllocateBuffer (
> >                                 mIoMmuProtocol,
> >
> 
> * Side remark: my general preference would be to #define the macros
> immediately with the right type, which can often be deduced from the
> intended use. So in this case, we could have one of:
> 
> #define EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE          ((UINT64)0x8000)
> 
> or
> 
> #define EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE          0x8000ULL
> 
> * In particular, all macros that are bitmasks should be suffixed with
> "U" minimally, or cast to UINT32. For example,
> 
> #define  BIT26    0x04000000
> 
> should actually be
> 
> #define  BIT26    0x04000000U
> 
> or else
> 
> #define  BIT26    ((UINT32)0x04000000)
> 
> This is because bitmasks are often bitwise-negated (ex.: ~BIT26), and
> given that the original is of type "int", the negated one is also an
> "int", with negative value. If, on top, you shift that left, for example
> 
>   (~BIT26 << 1)
> 
> you immediately have undefined behavior at your hands.
> 
> * I think the most curious example might be
> 
> #define  BIT31    0x80000000
> 
> Namely, *unlike* the other BIT{N} macros, with N<=30, this has type
> "unsigned int" (--> UINT32) automatically. The reason is that the 0x
> prefix enables the C language to include unsigned types when trying to
> determine the type of the integer constant. Given that this one doesn't
> fit in an "int" (INT32), the language picks the next type on the
> "ladder", and due to the 0x prefix, "unsigned int" comes next on the ladder.
> 
> * Here's a good way to think about integer constant prefixes and suffixes:
> 
> - normally the "ladder" only contains signed types -- int, long, long long
> 
> - if you add a 0x -- hex -- or 0 -- octal -- prefix, then you *insert*
> unsigned types to the ladder. int, unsigned, long, unsigned long, long
> long, unsigned long long.
> 
> - if you add the "u" suffix, then you *exclude* the signed types from
> the ladder, and force the unsigned types (regardless of 0 or 0x prefix).
> So you get unsigned, unsigned long, unsigned long long.

Yes, I think adding the 'u' suffix to those bit/bitmask definitions will
help to reduce the chance of incurring possible undefined behaviors during
left shift operations.

> 
> - the "l" and "ll" suffixes are orthogonal to signedness. They determine
> the "height" of the first step on the ladder, where the search starts.

I'm not very sure if adding the 'l'/'ll' suffixes will impact the size of
the binaries.

> 
> * Anyway, the patch seems good.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks for the effort.
Pushed as commit 8df95dd04f467c5626850b34dec564dec918c47d.

Best Regards,
Hao Wu

> 
> Thanks
> Laszlo