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

Davin McCall davmac at davmac.org
Wed Jul 1 09:33:28 PDT 2015


Hi Neil,

On 01/07/15 17:08, Neil Roberts wrote:
> If we wanted to avoid growing the size of exec_list to four pointers
> instead of three, maybe we could store it in a union like below:
>
> struct exec_list {
>     union {
>        struct {
>           struct exec_node head_sentinel;
>           struct exec_node *dummy_pointer_a;
>        };
>        struct {
>           struct exec_node *dummy_pointer_b;
>           struct exec_node tail_sentinel;
>        };
>     };
>     /* ... */
> };
>
> This is using anonymous structs and unions so that you wouldn't have to
> otherwise modify the patch.

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).

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.

> [...]
>
> It looks like this might also require removing the constructor for
> exec_node because you can't have objects with constructors in a union
> apparently. If the constructor is just a safety net anyway then maybe
> this won't cause any trouble. I don't know.

I suspect it is just a safety net, but I'm not sure.

>
> Also just to note, your patch doesn't apply with git-am. Have you
> cut-and-paste the patch file into an email? It looks like something has
> done some word wrapping which has corrupted it. Eg:
>
> [...]

Yes, I pasted into Thunderbird. I didn't realise it was wrapping lines, sorry. I'll re-post it once I figure out how to prevent the wrapping (setting up "git send-email" has proved difficult, I don't know how to get it to accept my self-signed SMTP server certificate).

Thanks,

Davin




More information about the mesa-dev mailing list