Message ID | 20161018183441.GB9586@flamenco |
---|---|
State | New |
Headers | show |
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. In that regards, changing the type of p from 'void **' (where you stride by the size of a pointer when computing lp) to 'void *' (where you stride by 1 under gcc when computing lp) is WRONG. > I prefer void **p since that matches lp's and l1_map's type. Not just prefer, but require. > > It's true that since we're dealing with void * the compiler won't > complain either way. It's a shame that void* relaxes typing so much, but this is one case where we HAVE to use the right type.
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.
diff --git a/translate-all.c b/translate-all.c index 4200869..6928ace 100644 --- a/translate-all.c +++ 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)); }