[Mesa-dev] [PATCH v3] glsl: fix some strict aliasing issues in exec_list

[Davin McCall]

On 02/07/15 14:58, Neil Roberts wrote:
> Davin McCall<davmac at davmac.org>  writes:
>
>> I actually had thought about this, but technically, you can only use
>> unions for type aliasing if you perform all accesses (that are not to
>> the 'active' member) through the union. All the list processing code
>> that iterates through all the nodes including the tail sentinel would
>> *technically* still have an aliasing problem, because it doesn't go
>> through the exec_list structure at all (though in practice, I don't
>> think it would ever manifest in as an issue in the compiled code).
> I don't think it matters that the list iterating code doesn't use
> exec_list. If something modifies the list pointers through an exec_node
> then the compiler will know that that potentially aliases the pointers
> in an exec_list because the pointers in exec_list are also wrapped in an
> exec_node.

This is why "in practice, I don't think it would ever manifest as an 
issue in the compiled code". But from a purely theoretical standpoint, 
the fact that the list iteration code will access both the head sentinel 
and tail sentinel struct exec_node without going through the union is an 
issue.

> With your patch there is no type aliasing at all and the
> union modification doesn't alter that.

If by 'type aliasing' you mean that an object is accessed through two 
different types (i.e. type punning), then:
The union modification *does* alter that - since there would be two 
anonymous structs which are punned via the union.

See the GCC documentation on the matter at: 
https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Type-punning - 
"... type-punning is allowed, provided the memory is accessed through 
the union type".

In practice, as I say, it would almost certainly come out ok if you did 
what you've suggested.

>> Why must all accesses go through the union? Consider the case:
>>
>>       int someMethod(float *f, int *i) {
>>           *f = 4.0;    // LINE A
>>           int a = *i;   // LINE B
>>           return a;
>>       }
>>
>> If I had some union 'u' with { float ff; int ii; } then I could call
>> the above method, even if it was in a different module, with:
>>
>>       someMethod(&u.ff, &u.ii).
>>
>> Now, that would mean that the compiler would not be allowed to
>> re-order LINE A and LINE B. But as I said, someMethod might be in a
>> different module, where the compiler does not know that 'i' and 'f'
>> point to members of the same union. In that case it assumes that 'i'
>> and 'f' don't alias. Compare that to:
>>
>>       int someMethod(union u *a, union u *b)
>>       {
>>           u->ff = 4.0;
>>           int a = u->ii;
>>           return a;
>>       }
>>
>> In this version, accesses are through the union, and the compiler
>> knows that they potentially alias.
> I don't think this example is relevant in this case because all of the
> relevant members of the union I suggested are the same type (struct
> exec_node). There is no type aliasing.

Any reference to a member in one of the anonymous structs can in fact 
alias the other anonymous struct (which is a different type) and its 
members.

> Maybe a hypothetical problem with this sort of use could be if you had a
> function like this:
>
> struct exec_node *
> some_method(struct exec_node *a,
>              struct exec_node *b)
> {
>     a->prev = &something;
>     b->next = &something_else;
>     return a->prev;
> }
>
> If you called this with the head and tail sentinels then the compiler
> won't know that a->prev and b->next alias each other so it might return
> &something instead of &something_else. However in practice for this use
> case the only part that is aliased is a NULL pointer that is never
> written to so I don't think it would actually matter.

I agree that it is unlikely to matter in practice; I'm just not sure 
that dicing with the language rules is a good idea (I mean, is it really 
worth resolving the aliasing violation issue by introducing a different 
kind of aliasing violation which we think won't trip up the compiler?) - 
and it's not necessary, in this case, even if it was worthwhile to avoid 
increasing the size of the exec_list structure (see my V2 patch).

Davin