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

[Laszlo Ersek <lersek@redhat.com>]

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.

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

* Anyway, the patch seems good.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo