[Mesa-dev] [PATCH v2] glsls: Modify exec_list to avoid strict-aliasing violations

[Eirik Byrkjeflot Anonsen]

Davin McCall <davmac at davmac.org> writes:

> On 26/06/15 14:31, Eirik Byrkjeflot Anonsen wrote:
>> Erik Faye-Lund <kusmabite at gmail.com> writes:
>>
>>> On Fri, Jun 26, 2015 at 1:23 PM, Davin McCall <davmac at davmac.org> wrote:
>>>> On 26/06/15 12:03, Davin McCall wrote:
>>>>> ... The stored value of 'n' is not accessed by any other type than the
>>>>> type of n itself. This value is then cast to a different pointer type. You
>>>>> are mistaken if you think that the cast accesses the stored value of n. The
>>>>> other "stored value" access that it occurs in that expression is to the
>>>>> object pointed at by the result of the cast. [...]:
>>>>
>>>> I'm sorry, I think that was phrased somewhat abrasively, which I did not
>>>> intend. Let me try this part again. If we by break up the expression in
>>>> order of evaluation:
>>>>
>>>> From:
>>>>     return ((const struct exec_node **)n)[0]
>>>>
>>>> In order of evaluation:
>>>>
>>>> n
>>>> - which accesses the stored value of n, i.e. a value of type 'struct exec
>>>> node *', via n, which is obviously of that type.
>>>>
>>>> (const struct exec_node **)n
>>>>   - which casts that value, after it has been retrieved, to another type. If
>>>> this were an aliasing violation, then casting any pointer variable to
>>>> another type would be an aliasing violation; this is clearly not the case.
>>>>
>>>> ((const struct exec_node **)n)[0]
>>>> - which de-references the result of the above cast, thereby accessing a
>>>> stored value of type 'exec node *' using a glvalue of type 'exec node *'.
>>> I think breaking this up is a mistake, because the strict-aliasing
>>> rules is explicitly about the *combination* of these two things.
>>>
>>> You *are* accessing the underlying memory of 'n' through a different
>>> type, and this is what strict aliasing is all about. But it takes two
>>> steps, a single step isn't enough to do so.
>>>
>>> Those other spec-quotes doesn't undo the strict-aliasing definitions;
>>> knowing how things are laid out in memory doesn't mean the compiler
>>> cannot assume two differently typed variables doesn't overlap.
>> So basically, you're saying that e.g.:
>>
>>     p->next = a;
>>     q = exec_node_get_next_const(p);
>>
>> is equivalent to:
>>
>>     exec_node * p1 = p;
>>     exec_node ** p2 = (exec_node**)p;
>>     p1->next = a;
>>     q = p2[0];
>
> It is, once the patch is applied (or if strict aliasing is disabled).
>
>> And at this point p1 and p2 are different types, so the compiler can
>> freely assume that p1 and p2 are non-overlapping.
>
> p1 and p2 are two separate variables and of course they are 
> non-overlapping, but *p1 and **p2 are the same type and so may overlap.

Hmm, yes unclear language on my side. I meant that *p1 and *p2 are
"known" to be non-overlapping due to the type difference.

>>   Thus the two
>> assignments can be "safely" reordered. Sounds plausible to me.
>
> The assignments are to 'p1->next' and 'p2[0]', which *are* the same type 
> (struct exec_node *) and therefore the assignments *cannot* be 
> reordered. It is exactly this that I rely on in my patch to resolve the 
> aliasing issue with the current code.

But, on the other hand, if *p1 and *p2 are known to be non-overlapping,
it would mean that p1->next and p2[0] are non-overlapping. Thus the two
statements can be reordered. Though I'd actually have to carefully read
the spec to check whether a compiler would be allowed to do that.

>> And note that casting via void* won't help. "p == (void*)p" compares a
>> variable of type "exec_node*" to a variable of type "void*", and thus
>> there's no strict-aliasing problem. But "p == (exec_node**)(void*)p"
>> compares an "exec_node*" to an "exec_node**" and thus the compiler can
>> assume that they are not the same.
>
> The compiler cannot assume pointers are not the same based on their 
> type, unless the pointers are de-referenced, which they are not in the 
> example you just gave. Strictly speaking C99 doesn't even allow the 
> comparison of 'p == (exec_node**)(void*)p', but all the compilers I know 
> of allow it, and AFAIK give the same result as if one operand were cast 
> to the same type as the other.
>
> Davin

Oops, yes. Really badly worded example :)

I did intend some form of dereference there to make sure that the fact
that "p" and "(exec_node**)(void*)p" are aliased pointers of different
type would lead to a strict-aliasing violation.

However, I'm assuming the intent of the rule is that two valid pointers
of different types can never point to the same address. In other words,
when the compiler sees two pointers of different types, it can assume
they point to different memory locations.

Thus, if the compiler thinks:

   p->next = a  becomes  "write a to p+0"
   b = q[0]     becomes  "read b from q+0"

With p and q being known different values (due to having different
types), it is clear that the read statement can not be affected by the
write statement.

Again, I'm not sure that's what the spec actually says, but I suspect
strongly that this was the intention of the strict aliasing rules. And
that precisely these optimizations is what compilers do when they use
the strict aliasing rules.

eirik