On Thu, Jun 25, 2015 at 1:48 AM, Davin McCall <dav...@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... _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
