Re: [Patch] PcAtChipsetPkg/SerialIoLib: Remove negative value shift

[Laszlo Ersek <lersek@redhat.com>]

On 05/18/17 19:03, Michael Kinney wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=553
> 
> Remove left shift of negative values that always evaluate
> to 0 to address build errors from the llvm/clang compiler
> used in the XCODE5 tool chain.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c b/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c
> index 95e0db7..0a2e20c 100644
> --- a/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c
> +++ b/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c
> @@ -102,7 +102,7 @@ SerialPortInitialize (
>    //
>    // Switch back to bank 0
>    //
> -  OutputData = (UINT8) ((~DLAB << 7) | (gBreakSet << 6) | (gParity << 3) | (gStop << 2) | Data);
> +  OutputData = (UINT8) ( (gBreakSet << 6) | (gParity << 3) | (gStop << 2) | Data);
>    IoWrite8 (gUartBase + LCR_OFFSET, OutputData);
>  
>    return RETURN_SUCCESS;
> @@ -481,7 +481,7 @@ SerialPortSetAttributes (
>    //
>    // Switch back to bank 0
>    //
> -  OutputData = (UINT8) ((~DLAB << 7) | (gBreakSet << 6) | (LcrParity << 3) | (LcrStop << 2) | LcrData);
> +  OutputData = (UINT8) ((gBreakSet << 6) | (LcrParity << 3) | (LcrStop << 2) | LcrData);
>    IoWrite8 (gUartBase + LCR_OFFSET, OutputData);
>  
>    return RETURN_SUCCESS;
> 

The patch looks correct, but the commit message is not.

As you write in the BZ, DLAB is defined as

#define DLAB                    0x01

After macro expansion, the value 0x01 has type "int".

On all edk2 platforms, "int" has 1 sign bit, 31 value bits, 0 padding
bits, and the representation is two's complement. After applying the
bit-neg operator (which is itself *not* undefined), the result has value
-2 (bit pattern 0xFFFF_FFFE).

When "int" value (-2) is left-shifted, the behavior is undefined. This
is why clang complains (justifiedly).

The fact that all left-shifted nonzero bits would be thrown away anyway,
 due to the final conversion to UINT8, does not save the undefined
behavior in the left-shift of (-2). That argument would only matter if
we had:

  ~(UINT32)DLAB

or else DLAB were defined as

#define DLAB                    0x01u

So, I would put the commit message as follows:

----
Clang rightfully complains about left-shifting ~DLAB. DLAB is #defined
as 0x01 (an "int"), hence ~DLAB has value (-2) on all edk2 platforms.
Left-shifting a negative int is undefined behavior.

Rather than replacing ~DLAB with ~(UINT32)DLAB, realize that the nonzero
bits of (~(UINT32)DLAB << 7) would all be truncated away in the final
conversion to UINT8 anyway. So just remove (~DLAB << 7).
----

With a commit message like this,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo