From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2D5BE21A143CF for ; Thu, 8 Jun 2017 13:39:16 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CF36E6A6C5; Thu, 8 Jun 2017 20:40:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CF36E6A6C5 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com CF36E6A6C5 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-122.phx2.redhat.com [10.3.116.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id 10CAA1821B; Thu, 8 Jun 2017 20:40:23 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org Cc: Jiewen Yao References: <20170608082230.13984-1-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 8 Jun 2017 22:40:23 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170608082230.13984-1-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 08 Jun 2017 20:40:25 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/PciHostBridgeDxe: Make bitwise operands of the same size X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jun 2017 20:39:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/08/17 10:22, Hao Wu wrote: > Cc: Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Hao Wu > --- > 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 Thanks Laszlo