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

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

The Base.h changes making BIT definitions unsigned have been denied.
Is a V2 needed for this patch? I verified again that casting the result of the inversion triggers the warning, except when casted before doing so, at least by VS2017.

Thanks,
Marvin.

> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Wednesday, February 28, 2018 3:44 AM
> To: Marvin.Haeuser@outlook.com; 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.
> 
> 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