From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 DB21122352286 for ; Tue, 27 Feb 2018 10:29:13 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5933CEAEAB; Tue, 27 Feb 2018 18:35:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-21.rdu2.redhat.com [10.10.120.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4D59DAFD70; Tue, 27 Feb 2018 18:35:18 +0000 (UTC) To: =?UTF-8?Q?Marvin_H=c3=a4user?= , "edk2-devel@lists.01.org" Cc: "ruiyu.ni@intel.com" , "eric.dong@intel.com" , "star.zeng@intel.com" References: <20180227164940.6956-1-Marvin.Haeuser@outlook.com> From: Laszlo Ersek Message-ID: Date: Tue, 27 Feb 2018 19:35:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 27 Feb 2018 18:35:19 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 27 Feb 2018 18:35:19 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent truncating constant values. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Feb 2018 18:29:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Hi Marvin, 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 > --- > MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c > index 0ccac96f419c..10eca6c0a7aa 100644 > --- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c > +++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c > @@ -366,7 +366,7 @@ GetSerialRegisterBase ( > // > if (DeviceInfo->PowerManagementStatusAndControlRegister != 0x00) { > if ((PciRead16 (PciLibAddress + DeviceInfo->PowerManagementStatusAndControlRegister) & (BIT0 | BIT1)) != 0x00) { > - PciAnd16 (PciLibAddress + DeviceInfo->PowerManagementStatusAndControlRegister, (UINT16)~(BIT0 | BIT1)); > + PciAnd16 (PciLibAddress + DeviceInfo->PowerManagementStatusAndControlRegister, (UINT16)~(UINT16)(BIT0 | BIT1)); > // > // If PCI UART was not in D0, then make sure FIFOs are enabled, but do not reset FIFOs > // > @@ -402,7 +402,7 @@ GetSerialRegisterBase ( > // > if (DeviceInfo->PowerManagementStatusAndControlRegister != 0x00) { > if ((PciRead16 (PciLibAddress + DeviceInfo->PowerManagementStatusAndControlRegister) & (BIT0 | BIT1)) != 0x00) { > - PciAnd16 (PciLibAddress + DeviceInfo->PowerManagementStatusAndControlRegister, (UINT16)~(BIT0 | BIT1)); > + PciAnd16 (PciLibAddress + DeviceInfo->PowerManagementStatusAndControlRegister, (UINT16)~(UINT16)(BIT0 | BIT1)); > } > } > > I find these warnings -- which I can only make up in my mind, since the commit message does not contain them -- bizarre. More precisely, I find the fact bizarre that the patch suppresses those warnings. Here's my argument: The expression (BIT0 | BIT1) has type "int". (Or, if you will, "INT32".) This is because of #define BIT0 0x00000001 #define BIT1 0x00000002 where the "0x" prefix, as I like to put it casually, only "enables" the compiler to consider unsigned integer types when picking the type for the integer constant; as opposed to "forcing" an unsigned type (that would come from the "u" suffix). Thus, because these constants are in range for "int", they are made "int"s, and the expression (BIT0 | BIT1) also has type "int". When you apply the bit-neg operator, the sign bit is negated. I consider that *exceedingly ugly*, but it is not undefined behavior, according to ISO C (as I understand it). Given our representation of "int" (which is implementation-defined), namely two's complement with no padding bits, the bit-neg does not produce a trap representation, it simply produces a negative value. (Namely, (-4).) Then, the outermost UINT16 cast, in the expression (UINT16)~(BIT0 | BIT1), converts the negative value to UINT16. This conversion is well-defined in the C standard; and, *solely* because of the two's complement representation that we use (see above), the complete expression happens to produce the exact value that we need; namely 65535 + 1 + (-4) == 65532 == binary 1111_1111_1111_1100. ( If we used one's complement, or sign-and-magnitude, then the bit-neg would produce a negative value that would *not* yield a correct end result, when converted to UINT16: - With one's complement, we'd get (-3) from the bit-neg, and the end result would be 65535 + 1 + (-3) == 65533 == binary 1111_1111_1111_1101. Not the mask we want. - With sign-and-magnitude, we'd get value (-2147483644) from the bit-neg (sign bit 1, magnitude 0x7FFF_FFFC). Converting this to UINT16 (i.e. adding 65536 repeatedly, 32768 times, until the value is in range for UINT16) yields the value 4; binary 0000_0000_0000_0100. Again not the mask we need. ) So, again: perhaps this "silent" dependency on two's complement in the bit-neg is why VS2015x86 complains -- I don't know. Now, the bizarre thing is, the patch does not change *anything* at all in this thought process! When you do (UINT16)(BIT0 | BIT1) the resultant UINT16 value is immediately promoted back to "int" (due to the default integer promotions -- all UINT16 values can be represented in "int"), before the bit-neg operator is applied. In other words, the bit-neg operator is applied to the exact same value (including type "int") as before the patch. Thus I don't understand how the patch can have any effect on the compiler. Now, if you wrote (UINT16)~(UINT32)(BIT0 | BIT1) ^^^^^^^^ I would understand that (and indeed this is the form that I find optimal). UINT32 maps to "unsigned int", and "unsigned int" is unaffected by the integer promotions. Therefore the bit-neg would be performed on an "unsigned int" -- fantastic: no dependency on the representation of negative values --, and the resultant non-negative value -- directly defined by the C standard: 4294967292, 0xFFFF_FFFC -- would be truncated to UINT16 "more cleanly". (Speaking in "assumed VS2015x86 terms" anyway). So... I'm not saying you should care about my review; however, if you do, would you please consider resubmitting the patch with UINT32? :) There's an argument for using UINT32 here (regardless of VS2015x86); namely that we should avoid bit-neg on signed integer types anyway, for our own sanity's sake. If that fixes some compiler warnings too, all the better. Thanks! Laszlo