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.z...@intel.com> > Sent: Wednesday, February 28, 2018 3:44 AM > To: marvin.haeu...@outlook.com; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Dong, Eric <eric.d...@intel.com>; Zeng, Star <star.z...@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-boun...@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...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Dong, Eric <eric.d...@intel.com>; Zeng, Star <star.z...@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 <ler...@redhat.com> > > Sent: Tuesday, February 27, 2018 7:35 PM > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > de...@lists.01.org > > Cc: ruiyu...@intel.com; eric.d...@intel.com; star.z...@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.haeu...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
