Re: [Qemu-devel] [PATCH v9 26/37] qapi: Simplify excess input reporting in input visitors

[Laszlo Ersek <lersek@redhat.com>]

On 01/25/16 10:27, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 01/22/2016 12:24 PM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> When reporting that an unvisited member remains at the end of an
>>>> input visit for a struct, we were using g_hash_table_find()
>>>> coupled with a callback function that always returns true, to
>>>> locate an arbitrary member of the hash table.  But if all we
>>>> need is an arbitrary entry, we can get that from a single-use
>>>> iterator, without needing a tautological callback function.
>>>
>>> Good idea.
>>>
>>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> Whoops, it's even mine!  I forgot... %-)

Side remark:

https://developer.gnome.org/glib/stable/glib-Hash-Tables.html

g_hash_table_find (): Since: 2.4
g_hash_table_iter_next (): Since: 2.16

I can't prove that when I wrote this code, g_hash_table_iter_next()
wasn't available, but I may easily have googled & looked at GLib
*documentation* that lacked g_hash_table_iter_next(). :)

>>>
>>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> ---
>>
>>>>      GQueue *any;
>>>>
>>>>      if (--ov->depth > 0) {
>>>> @@ -174,8 +168,8 @@ opts_end_struct(Visitor *v, Error **errp)
>>>>      }
>>>>
>>>>      /* we should have processed all (distinct) QemuOpt instances */
>>>> -    any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
>>>> -    if (any) {
>>>> +    g_hash_table_iter_init(&iter, ov->unprocessed_opts);
>>>> +    if (g_hash_table_iter_next(&iter, NULL, (void **)&any)) {
>>>
>>> Is this cast kosher?
>>
>> You may have a point that it violates some corner of C99, but I'm not
>> the first such user:
>>
>> hw/i386/intel_iommu.c:    while (g_hash_table_iter_next (&bus_it, NULL,
>> (void**)&vtd_bus)) {
>> qom/object.c:    while (g_hash_table_iter_next(&iter, NULL, (gpointer
>> *)&prop)) {
>>
>> Conceptually, it seems fine - void* can be assigned to any pointer, and
>> 'any' qualifies as such a pointer.  We are then taking the address of
>> that (or void**) to pass by reference.
> 
> Yes, any pointer can be converted to and from void *.  Doesn't mean the
> conversion results in the same bits.  It does on machines with a single,
> uniform pointer format, i.e. any sane machine.
> 
> (void **)iter converts iter, not *iter.  *iter gets reinterpreted on
> dereference.
> 
> You're right in that this kind of type punning already occurs in QEMU.
> Also in other programs.  We can hope it's common enough to deter
> optimizers from screwing it up even on sane machines.
> 
>> I suppose a stricter version would be:
>>
>> void *wrap;
>> GQueue *any;
>> if (g_hash_table_iter_next(&iter, NULL, &wrap)) {
>>     any = wrap;
>> ...
>>
>> but is it worth the bother?  Put another way, will a compiler ever do
>> the wrong thing to us because C99 might have some corner case?  Laszlo,
>> what's your take?
> 
> Perhaps Laszlo can confirm or refute my reading of the standard.

The concern is justified; the expression "(void **)&any" does not
convert a pointer-to-void to pointer-to-GQueue, it causes the bit
pattern to be reinterpreted (it is stored as pointer-to-void and
reinterpreted as pointer-to-GQueue).

6.2.5 Types p27 says "A pointer to void shall have the same
representation and alignment requirements as a
pointer to a character type. [...] All pointers to structure types shall
have the same representation and alignment requirements as each other."

(I'll assume that GQueue is a struct.) But, they need not match each
other. At worst, "any" might not even have enough storage for a
pointer-to-void.

I'll mention that edk2 is chock-full of the above style cast. I think
even the UEFI spec is, in code examples. But, edk2 builds with
-fno-strict-aliasing & friends.

... I think it doesn't matter in practice, but if we're trying to
prevent compilers from outsmarting us, I'd feel better about "any = wrap".

Thanks
Laszlo


> 
>>>>          if (top_ht) {
>>>> -            if (g_hash_table_size(top_ht)) {
>>>> -                const char *key;
>>>> -                g_hash_table_find(top_ht, always_true, &key);
>>>> +            GHashTableIter iter;
>>>> +            const char *key;
>>>> +
>>>> +            g_hash_table_iter_init(&iter, top_ht);
>>>> +            if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
>>>
>>> Is this cast kosher?
>>
>> Here, in addition to the above argument, we also needed to cast away const.