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

[Erik Faye-Lund]

On Thu, Jun 25, 2015 at 1:48 AM, Davin McCall <davmac at davmac.org> wrote:
> This is an alternative to my earlier patch [1] (and it is now constructed
> properly using git format-patch).
>
> Quick background:
> There is a problem in exec_list due to it directly including a trio
> of 'struct exec_node *' members to implement two overlapping sentinel
> nodes. The sentinel nodes do not exist as exec_node objects and so
> should not be accessed as such, according to C99 6.5 paragraph 7.
> When this strict aliasing rule is violated the compiler may re-order
> reads and writes in unexpected ways, such as demonstrated in another
> email [2].
>
> The problem only manifests if compiling without -fno-strict-aliasing.
>
> This patch addresses the issue by introducing some new methods for
> setting the 'next' and 'prev' members of the exec_node structure, which
> avoid the aliasing restrictions by way of casting the exec_node pointer
> (to an exec_node-pointer-pointer) whenever it may actual point to a
> sentinel node. Essentially an exec_node can be seen as an array of two
> exec_node pointers, and this view is compatible with the sentinel
> structure in exec_list.
>
> Compared to the previous patch, this patch is much less intrusive, and
> does not increase the storage requirements of the exec_list structure.
>
> While I'm not proposing that -fno-strict-aliasing no longer be used for
> Mesa builds, this patch represents a step in that direction. With this
> patch applied, a working Mesa library can be built, although bugs may
> be present (and could be triggered only when using particular compiler
> versions and options). FWIW file sizes with and without strict aliasing:
>
> (gcc 4.8.4, -O3 -fomit-frame-pointer -march=corei7).
>
>             -fno-strict-aliasing:        with strict aliasing:
> libGL.so          699188                  699188    (no change)
> *_dri.so         9575876                 9563104    (-2772)
>
> (dri bundle includes r300, r600, kms_swrast and swrast).
>
> So, not a huge win, size-wise. Dave Airlie reports a 30K difference in
> his r600_dri.so build however [3].
>
> In terms of performance:
>
> (export LIBGL_ALWAYS_SOFTWARE=1; time glmark2)
>
> -fno-strict-aliasing:
>
> glmark2 Score: 244
> real    5m34.707s
> user    11m36.192s
> sys    0m29.596s
>
> with strict aliasing:
>
> glmark2 Score: 247
> real    5m34.438s
> user    11m29.904s
> sys    0m29.556s
>
> Again, only a very small improvement when strict aliasing is enabled.
>
> With the above in mind it is reasonable to question whether this patch
> is worthwhile. However, it's done, and it seems to work, so I offer it
> for review.
>
>
> [1] http://lists.freedesktop.org/archives/mesa-dev/2015-June/087179.html
> [2] http://lists.freedesktop.org/archives/mesa-dev/2015-June/087246.html
> [3] http://lists.freedesktop.org/archives/mesa-dev/2015-June/087206.html
> ---
>  src/glsl/list.h | 123
> ++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 80 insertions(+), 43 deletions(-)
>
> diff --git a/src/glsl/list.h b/src/glsl/list.h
> index 15fcd4a..cfbe5a9 100644
> --- a/src/glsl/list.h
> +++ b/src/glsl/list.h
> @@ -76,6 +76,12 @@
>  #include "util/ralloc.h"
>
>  struct exec_node {
> +   /**
> +    * Accessing these fields directly may be ill-advised; if the
> 'exec_node'
> +    * is actually a sentinel node embedded in the exec_list structure, it
> may
> +    * be a strict-aliasing violation (C99 6.5 paragraph 7). Use the methods
> +    * provided instead.
> +    */
>     struct exec_node *next;
>     struct exec_node *prev;
>
> @@ -140,35 +146,55 @@ exec_node_init(struct exec_node *n)
>     n->prev = NULL;
>  }
>
> +/**
> + * Strict-aliasing safe method for setting the next pointer for any
> + * node, including sentinel nodes.
> + */
> +static inline void
> +exec_node_set_next(struct exec_node *n, struct exec_node *next)
> +{
> +   ((struct exec_node **)n)[0] = next;
> +}
> +
> +/**
> + * Strict-aliasing safe method for setting the next pointer for any
> + * node, including sentinel nodes.
> + */
> +static inline void
> +exec_node_set_prev(struct exec_node *n, struct exec_node *next)
> +{
> +   ((struct exec_node **)n)[1] = next;
> +}
> +
>  static inline const struct exec_node *
>  exec_node_get_next_const(const struct exec_node *n)
>  {
> -   return n->next;
> +   return ((const struct exec_node **)n)[0];
>  }

How exactly is this supposed to be strict-aliasing safe?

Here's the wording from the C++14 spec:

"If a program attempts to access the stored value of an object through
a glvalue of other than one of the following types the behavior is
undefined:
* the dynamic type of the object,
* a cv-qualified version of the dynamic type of the object,
* a type similar (as defined in 4.4) to the dynamic type of the object,
* a that is the signed or unsigned type corresponype tding to the
dynamic type of the object,
* a type that is the signed or unsigned type corresponding to a
cv-qualified version of the dynamic type of the object,
* an aggregate or union type that includes one of the aforementioned
types among its elements or non-static data members (including,
recursively, an element or non-static data member of a subaggregate or
contained union),
* a type that is a (possibly cv-qualified) base class type of the
dynamic type of the object,
* a char or unsigned char type."

You cast an 'struct exec_node *' to 'struct exec_node **', and read
it. Those are different types, and are not among the exceptions listed
above...