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

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

[Marvin Häuser]

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/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));
       }
     }
       
-- 
2.16.0.windows.2

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

[Laszlo Ersek]

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/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

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

[Marvin Häuser]

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

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

[Zeng, Star]

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

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

[Laszlo Ersek]

On 02/27/18 19:55, Marvin Häuser wrote:
> 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

With the other (pre-requisite) change applied, such that

#define  BIT0     0x00000001u
#define  BIT1     0x00000002u

it's even less comprehensible why VS2015x86 warns about the original
code, namely:

  (UINT16)~(BIT0 | BIT1)

In this case we apply bit-or to two "unsigned int" operands, and then
apply bit-neg to the "unsigned int" result. Finally we explicitly cast
the expression to another unsigned integer type. All well-defined.

The MSDN reference writes, "The type conversion causes a constant to
exceed the space allocated for it." I don't understand how that does
*not* apply to the post-patch code, namely

  (UINT16)~(UINT16)(BIT0 | BIT1)

Again, in this variant (with the BIT macros being unsigned), we
basically have

  (UINT16)(-4)

How on earth does the constant "minus 4" (-4) fit in the UINT16 "space"?
It totally doesn't.

Sorry, VS2015x86 boggles my mind. I think it's a compiler bug, plain and
simple. Please just do whatever you have to do to shut it up. I give up.

Thanks,
Laszlo

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

[Marvin Häuser]

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

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

[Zeng, Star]

Marvin,

Sorry.

What is the warning message?
We still need to use "(UINT16)~(UINT16)(BIT0 | BIT1)"?
All the truncations want to get the value "0xFFFC", right?
So could we try to directly use "0xFFFC"?

Liming and Mike,
Do you have any concern about it?


Thanks,
Star
-----Original Message-----
From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com] 
Sent: Friday, March 2, 2018 1:35 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; 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.

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