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

[Eirik Byrkjeflot Anonsen]

Francisco Jerez <currojerez at riseup.net> writes:

> Erik Faye-Lund <kusmabite at gmail.com> writes:
>
>> On Fri, Jun 26, 2015 at 4:01 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> 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.
>>>>
>>> Also note that even *p1 and *p2 are allowed to overlap even though they
>>> are of different types because of section 6.5 of C99:
>>>
>>> | 7 An object shall have its stored value accessed only by an lvalue
>>> |   expression that has one of the following types:
>>> |[...]
>>> |    - an aggregate or union type that includes one of the aforementioned
>>> |      types among its members (including, recursively, a member of a
>>> |      subaggregate or contained union)[...]
>>
>> I don't see how this wording legitimates the code above. There's no
>> unions involved, so that part is irrelevant.
>
> Yeah, only the "aggregate" part is relevant.
>
>> And "n" isn't an aggregate type, it's a pointer type that happens to
>> point to an aggregate type, no? But even if it were, it needs to
>> include one of the "aforementioned" types among its members, which I
>> cannot see that it does either.
>>
>> Care to explain?
>
> In the example this was replying to, *p1 was an lvalue of aggregate type
> (struct exec_node), and *p2 (or equivalently p2[0]) was an lvalue of
> type "struct exec_node *".  The latter is "a type compatible with the
> effective type of the object", the object being the "next" member of an
> aggregate object of type "struct exec_node", hence the text quoted above
> applies and *p1 and *p2 may legitimately alias the same object.

The argument makes sense. As I said, I haven't been carefully thinking
about aliasing issues, and I have a feeling that there is "obviously
correct code" that probably requires this to work. If my example was
broken, it would probably break stuff that shouldn't break.

eirik