Re: [Qemu-devel] [PATCH 1/2] target-i386: Use 1UL for bit shift

[Paolo Bonzini <pbonzini@redhat.com>]

On 02/10/2015 13:14, Laszlo Ersek wrote:
> On 10/02/15 10:34, Paolo Bonzini wrote:
>> On 01/10/2015 21:17, Laszlo Ersek wrote:
>>> - In the firmware, allocate an array of bytes, dynamically. This array
>>>   will have no declared type.
>>>
>>> - Populate the array byte-wise, from fw_cfg. Because the stores happen
>>>   through character-typed lvalues, they do not "imbue" the target
>>>   object with any effective type, for further accesses that do not
>>>   modify the value. (I.e., for further reads.)
>>>
>>> - Get a (uint8_t*) into the array somewhere, and cast it to
>>>   (struct acpi_table_hdr *). Read fields through the cast pointer.
>>>   Assuming no out-of-bounds situation (considering the entire
>>>   pointed to acpi_table_hdr struct), and assuming no alignment
>>>   violations for the fields (which is implementation-defined), these
>>>   accesses will be fine.
>>>
>>> *However*. If in point 2 you populate the array with uint64_t accesses,
>>> that *does* imbue the array elements with an effective type that is
>>> binding for further read accesses.
>>
>> Then don't do it.  Use memcpy from uint64_t to the array.
> 
> It won't work; memcpy() propagates the effective type.

Doh.  I guess that's another "not in practice" case.  Saying "memcpy to
{,u}int8_t doesn't propagate the effective type" would probably go to
great lengths towards fixing this.

> So, I guess the idea is that you'd like to stay in "int" as much as
> possible.

Yes.  Except move to 64-bit as early as possible if it will be necessary
to do that.

> (And, with respect to the above point, both uint8_t and
> uint16_t are promoted to int (=== int32_t), on all platforms that matter.)

Yes, but uint8_t arithmetic cannot overflow as easily as uint16_t.
int16_t is fine, but not as useful as uint16_t could be.

> In comparison, I'm a huge fan of unsigned-only, both in variables /
> fields and in constants. :)
> 
> One random example: (a - b). If "a" and "b" are unsigned, then (1)
> wrapping is well-defined, (2) if you don't want it

Sorry for snipping your derivation (which I did read) but... checking
for overflow is not the common case.  The common case is that you want
to cast "a" and "b" to a 64-bit type. :)

And if you already have an int64_t, that is also not the common case: it
is not too useful to _store_ int64_t's.  uint64_t's are useful because
they are size_t's.  But ptrdiff_t overflows usually result from
multiplication, not from addition or subtractions.

I know these are sweeping generalizations, but generalizations are what
we use to unload our brains from the nitty-gritty details.

> ... Given that we almost never need negative integer values, I'd rather
> stick with unsigned variables, unsigned constants, and write (a<b), in
> order to check against wrapping, than use the above monstrosity.

It's not a panacea, for example

   for (i = 0; i <= j; i++)

can be an infinite loop for unsigned but not for signed (and this,
again, has an effect on what optimizations the compiler can do).

Since I'm not a precise person, I wouldn't expect that to be a possibly
infinite loop.  Using "int" makes the compiler's behavior match my
intuition more closely.

Paolo

> Sure, we can always cast to int64_t first... and if we're subtracting
> int64_t, we can always cast to Int128 first... :P
> 
> Laszlo
>