[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 26/37] qapi: Simplify excess input reporting
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v9 26/37] qapi: Simplify excess input reporting in input visitors |
Date: |
Mon, 25 Jan 2016 11:43:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
On 01/25/16 10:27, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> On 01/22/2016 12:24 PM, Markus Armbruster wrote:
>>> Eric Blake <address@hidden> 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 <address@hidden>
>>>
>>> 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 <address@hidden>
>>>> Reviewed-by: Marc-André Lureau <address@hidden>
>>>>
>>>> ---
>>
>>>> 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.
[Qemu-devel] [PATCH 0/3] qapi-visit: Unify struct and union visit, Markus Armbruster, 2016/01/27
[Qemu-devel] [PATCH 3/3] qapi-visit: Unify struct and union visit, Markus Armbruster, 2016/01/27
[Qemu-devel] [PATCH 2/3] qapi-visit: Clean up code generated around visit_end_union(), Markus Armbruster, 2016/01/27
Re: [Qemu-devel] [PATCH 2/3] qapi-visit: Clean up code generated around visit_end_union(), Eric Blake, 2016/01/27
Re: [Qemu-devel] [PATCH 2/3] qapi-visit: Clean up code generated around visit_end_union(), Markus Armbruster, 2016/01/27
[Qemu-devel] [PATCH v9 18/37] qapi: Drop unused error argument for list and implicit struct, Eric Blake, 2016/01/19