Re: [PATCH] EmbeddedPkg/GdbSerialLib: avoid left shift of negative quantity

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Jun 19, 2018 at 02:51:41PM +0200, Laszlo Ersek wrote:
> On 06/19/18 11:54, Leif Lindholm wrote:
> > On Tue, Jun 19, 2018 at 12:51:49AM +0200, Laszlo Ersek wrote:
> >> On 06/18/18 23:57, Leif Lindholm wrote:
> >>> On Mon, Jun 18, 2018 at 10:49:18PM +0200, Ard Biesheuvel wrote:
> >>>> Clang complains about left shifting a negative value being undefined.
> >>>
> >>> As well it should.
> >>>
> >>>>   EmbeddedPkg/Library/GdbSerialLib/GdbSerialLib.c:151:30:
> >>>>   error: shifting a negative signed value is undefined [-Werror,-Wshift-negative-value]
> >>>>   OutputData = (UINT8)((~DLAB<<7)|((BreakSet<<6)|((Parity<<3)|((StopBits<<2)| Data))));
> >>>>
> >>>> Redefine all bit pattern constants as unsigned to work around this.
> >>>
> >>> So, I'm totally OK with this and
> >>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >>> but ...
> >>> would it be worth fixing up BIT0-31 in Base.h and use those?
> >>
> >> If we started with the BITxx macros now, I'd agree. Given the current
> >> tree, I don't :) I've made an argument against the suggestion earlier:
> >>
> >>   UINT64 Value64;
> >>
> >>   Value64 = ~0x1;
> >>   Value64 = ~0x1U;
> >>
> >> The two assignments produce different results.
> >>
> >> In the first case, the integer constant 0x1 has type "int". Our C
> >> language implementation uses, for type "int", two's complement
> >> representation, 1 sign bit, 31 value bits, and no padding bits. So after
> >> the bitwise complement, we get 0x1111_1111_1111_1110, also with type
> >> int, and with value (-2). Taking UINT64 for "unsigned long int" or
> >> "unsigned long long int", after the conversion implied by the assignment
> >> we get ((UINT64_MAX + 1) + (-2)), aka (UINT64_MAX - 1).
> > 
> > Err, if by UINT64_MAX + 1 you mean 0,
> 
> No, I don't.
> 
> > as a way of introducing the
> > second part, I'm with you. Otherwise I think I need to go back to
> > school.
> 
> I really meant (UINT64_MAX + 1) as the mathematical integer
> 18,446,744,073,709,551,616; without the modular wrap-around that occurs
> in C. The modular wrap-around of C is defined in terms of the
> mathematical values. This is how the ISO C99 standard defines the behavior:
> 
> ------
> 6.3.1.3 Signed and unsigned integers
> 
> 1 When a value with integer type is converted to another integer type
>   other than _Bool, if the value can be represented by the new type, it
>   is unchanged.
> 
> 2 Otherwise, if the new type is unsigned, the value is converted by
>   repeatedly adding or subtracting one more than the maximum value that
>   can be represented in the new type until the value is in the range of
>   the new type. [footnote 49]

So for negative add, for positive subtract? I find the language in the
spec is indicative of the people who write it. And those people spend
a lot of time working on compiler optimisations.

> 3 Otherwise, the new type is signed and the value cannot be represented
>   in it; either the result is implementation-defined or an
>   implementation-defined signal is raised.

*twitch*

> [footnote 49] The rules describe arithmetic on the mathematical value,
> not the value of a given type of expression.
> ------
> 
> Therefore we really have (18,446,744,073,709,551,616 + (-2)) in the
> above reasoning.

Blimey. Live and learn.
Thanks for that - at least the period in school was shorter than I feared.

> >> In the second case, the constant 0x1U has type "unsigned int". After the
> >> bitwise complement, we get (UINT32_MAX - 1), also of type "unsigned
> >> int". Taking UINT64 for "unsigned long int" or "unsigned long long int",
> >> after the conversion implied by assignment we get the exact same value,
> >> (UINT32_MAX - 1).
> > 
> > But these examples make it look like the differences are an
> > unobservable internal side effect. That's not the case.
> 
> Hm, sorry, I don't understand. My point was that the difference was
> extremely observable, because in the first assignment, we end up with
> value 18,446,744,073,709,551,614; in the second, we end up with
> 4,294,967,294.
> 
> Perhaps my expression "exact same value" was misleading.

Yes, that threw me.
Combined with your detestation for sign-extension (which does
help describe the situation without going full spec).

> There I
> referred to 6.3.1.3p1, which I've now quoted above as well -- I meant
> that converting 4,294,967,294 from "unsigned int" to either of "unsigned
> long int" or "unsigned long long int" didn't change the value.

Understood. Thanks a lot!

/
    Leif