Re: [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent truncating constant values.

["Zeng, Star" <star.zeng@intel.com>]

Could simply use 0xFFFC for this case?

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin H?user
Sent: Wednesday, February 28, 2018 2:56 AM
To: edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550: Prevent truncating constant values.

Hey Laszlo,

Thanks for your... detailed explanation. :) I actually submitted another patch to prevent what you explained - "[edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.", which marks all BIT defines (and more) as unsigned.
Most definitely I should have mentioned it in the commit message or held it back till that patch will be accepted (or denied?), seems like I forgot about that.
Would you still prefer your suggestion even when the Base.h patch is merged? After all, int might happen to be even larger than INT32, if I'm not mistaken.

I'm quite sure VS2015x86 issued the warning despite that commit being applied locally. It seems to always warn when a constant is truncated, explicitely or implicitely, to give you the change to increase its size:
https://msdn.microsoft.com/en-us/library/sz5z1byt.aspx

Thanks again for your comprehensive review!

Best regards,
Marvin.

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, February 27, 2018 7:35 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- 
> devel@lists.01.org
> Cc: ruiyu.ni@intel.com; eric.dong@intel.com; star.zeng@intel.com
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/BaseSerialPortLib16550:
> Prevent truncating constant values.
> 
> 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 <Marvin.Haeuser@outlook.com>
> > ---
> >  
> > 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/BaseSerialPortLib165
> > +++ 50.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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel