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 <[email protected]> > Cc: Andrew Fish <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael D Kinney <[email protected]> > --- > 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 <[email protected]> Thanks, Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
