Re: [Qemu-devel] [PATCH 2/2] translate-all: Use proper type

[Pranith Kumar]

Eric Blake writes:

> On 10/18/2016 01:34 PM, Emilio G. Cota wrote:
>> +++ b/translate-all.c
>> @@ -405,23 +405,23 @@ static void page_init(void)
>>  static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>>  {
>>      PageDesc *pd;
>>      void **lp;
>>      int i;
>> 
>>      /* Level 1.  Always allocated.  */
>>      lp = l1_map + ((index >> V_L1_SHIFT) & (V_L1_SIZE - 1));
>> 
>>      /* Level 2..N-1.  */
>>      for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
>> -        void **p = atomic_rcu_read(lp);
>> +        void *p = atomic_rcu_read(lp);
>> 
>>          if (p == NULL) {
>>              if (!alloc) {
>>                  return NULL;
>>              }
>>              p = g_new0(void *, V_L2_SIZE);
>>              atomic_rcu_set(lp, p);
>>          }
>> 
>>          lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));
>
> Pointer addition of 'void *' plus an offset is undefined (gcc, and
> presumably clang, have an extension that treats it the same as computing
> an offset to a 'char *'; but some compilers choke); this is because
> sizeof(void) is unknown, so you don't know what stride to make for each
> offset.  Or put another way, 'p + offset' is the same as
> '&((*p)[offset])', but (*p)[offset] is ill-defined when p is the opaque
> type void.
>
> Pointer addition of 'void **' plus an offset is well-defined, because
> sizeof(void*) is well-defined and therefore the stride (4 or 8) makes
> sense.  Or in array notation, computing '&((*p)[offset]) means we are
> skipping to the offset array entry where p is the start of the array of
> void* pointers.
>

Indeed. I missed that crucial detail. I would prefer explicitly casting to
'void **' for p, since that is not the type of what is being returned by
atomic_rcu_read().

The joys of void pointer arithmetic, TIL.

-- 
Pranith