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

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

[Hao Wu]

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,
-- 
2.12.0.windows.1

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

[Yao, Jiewen]

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao
> Wu
> Sent: Thursday, June 8, 2017 4:23 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [edk2] [PATCH] MdeModulePkg/PciHostBridgeDxe: Make bitwise
> operands of the same size
> 
> 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,
> --
> 2.12.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

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

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

[Wu, Hao A]

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