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

Davin McCall davmac at davmac.org
Wed Jun 24 16:48:04 PDT 2015


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];
  }

  static inline struct exec_node *
  exec_node_get_next(struct exec_node *n)
  {
-   return n->next;
+   return ((struct exec_node **)n)[0];
  }

  static inline const struct exec_node *
  exec_node_get_prev_const(const struct exec_node *n)
  {
-   return n->prev;
+   return ((const struct exec_node **)n)[1];
  }

  static inline struct exec_node *
  exec_node_get_prev(struct exec_node *n)
  {
-   return n->prev;
+   return ((struct exec_node **)n)[1];
  }

  static inline void
  exec_node_remove(struct exec_node *n)
  {
-   n->next->prev = n->prev;
-   n->prev->next = n->next;
+   exec_node_set_prev(n->next, n->prev);
+   exec_node_set_next(n->prev, n->next);
     n->next = NULL;
     n->prev = NULL;
  }
@@ -213,13 +239,15 @@ exec_node_replace_with(struct exec_node *n, struct 
exec_node *replacement)
  static inline bool
  exec_node_is_tail_sentinel(const struct exec_node *n)
  {
-   return n->next == NULL;
+   // Use a cast to avoid strict aliasing concerns:
+   return ((const struct exec_node **)n)[0] == NULL;
  }

  static inline bool
  exec_node_is_head_sentinel(const struct exec_node *n)
  {
-   return n->prev == NULL;
+   // Use a cast to avoid strict aliasing concerns:
+   return ((const struct exec_node **)n)[1] == NULL;
  }

  #ifdef __cplusplus
@@ -302,6 +330,10 @@ inline bool exec_node::is_head_sentinel() const
  #define exec_node_data(type, node, field) \
     ((type *) (((char *) node) - exec_list_offsetof(type, field, node)))

+#define exec_node_from_data(type, data, field) \
+   ((struct exec_node *) (((char *) data) + exec_list_offsetof(type, 
field, data)))
+
+
  #ifdef __cplusplus
  struct exec_node;
  #endif
@@ -417,7 +449,8 @@ exec_list_length(const struct exec_list *list)
     unsigned size = 0;
     struct exec_node *node;

-   for (node = list->head; node->next != NULL; node = node->next) {
+   for (node = list->head; ! exec_node_is_tail_sentinel(node);
+         node = node->next) {
        size++;
     }

@@ -430,7 +463,7 @@ exec_list_push_head(struct exec_list *list, struct 
exec_node *n)
     n->next = list->head;
     n->prev = (struct exec_node *) &list->head;

-   n->next->prev = n;
+   exec_node_set_prev(n->next, n);
     list->head = n;
  }

@@ -440,7 +473,7 @@ exec_list_push_tail(struct exec_list *list, struct 
exec_node *n)
     n->next = (struct exec_node *) &list->tail;
     n->prev = list->tail_pred;

-   n->prev->next = n;
+   exec_node_set_next(n->prev, n);
     list->tail_pred = n;
  }

@@ -450,7 +483,7 @@ exec_list_push_degenerate_list_at_head(struct 
exec_list *list, struct exec_node
     assert(n->prev->next == n);

     n->prev->next = list->head;
-   list->head->prev = n->prev;
+   exec_node_set_prev(list->head, n->prev);
     n->prev = (struct exec_node *) &list->head;
     list->head = n;
  }
@@ -490,7 +523,7 @@ exec_list_append(struct exec_list *list, struct 
exec_list *source)

     /* Link the first node of the source with the last node of the 
target list.
      */
-   list->tail_pred->next = source->head;
+   exec_node_set_next(list->tail_pred, source->head);
     source->head->prev = list->tail_pred;

     /* Make the tail of the source list be the tail of the target list.
@@ -519,8 +552,8 @@ exec_node_insert_list_before(struct exec_node *n, 
struct exec_list *before)
     before->tail_pred->next = n;
     before->head->prev = n->prev;

-   n->prev->next = before->head;
-   n->prev = before->tail_pred;
+   exec_node_set_next(n->prev, before->head);
+   exec_node_set_prev(n, before->tail_pred);

     exec_list_make_empty(before);
  }
@@ -530,7 +563,7 @@ exec_list_validate(const struct exec_list *list)
  {
     const struct exec_node *node;

-   assert(list->head->prev == (const struct exec_node *) &list->head);
+   assert(exec_node_get_prev_const(list->head) == (const struct 
exec_node *) &list->head);
     assert(list->tail == NULL);
     assert(list->tail_pred->next == (const struct exec_node *) 
&list->tail);

@@ -538,9 +571,9 @@ exec_list_validate(const struct exec_list *list)
      * either require C++ or assume the exec_node is embedded in a 
structure
      * which is not the case for this function.
      */
-   for (node = list->head; node->next != NULL; node = node->next) {
-      assert(node->next->prev == node);
-      assert(node->prev->next == node);
+   for (node = list->head; ! exec_node_is_tail_sentinel(node); node = 
node->next) {
+      assert(exec_node_get_prev(node->next) == node);
+      assert(exec_node_get_next(node->prev) == node);
     }
  }

@@ -634,16 +667,16 @@ inline void exec_node::insert_before(exec_list 
*before)
  /**
   * This version is safe even if the current node is removed.
   */
-#define foreach_in_list_safe(__type, __node, __list) \
-   for (__type *__node = (__type *)(__list)->head,   \
-               *__next = (__type *)__node->next;     \
-        __next != NULL;                              \
+#define foreach_in_list_safe(__type, __node, __list)           \
+   for (__type *__node = (__type *)(__list)->head,             \
+               *__next = (__type *)exec_node_get_next(__node); \
+        __next != NULL;                                        \
          __node = __next, __next = (__type *)__next->next)

-#define foreach_in_list_reverse_safe(__type, __node, __list) \
-   for (__type *__node = (__type *)(__list)->tail_pred,      \
-               *__prev = (__type *)__node->prev;             \
-        __prev != NULL;                                      \
+#define foreach_in_list_reverse_safe(__type, __node, __list)   \
+   for (__type *__node = (__type *)(__list)->tail_pred,        \
+               *__prev = (__type *)exec_node_get_prev(__node); \
+        __prev != NULL;                                        \
          __node = __prev, __prev = (__type *)__prev->prev)

  #define foreach_in_list_use_after(__type, __inst, __list) \
@@ -656,27 +689,29 @@ inline void exec_node::insert_before(exec_list 
*before)
   *
   * This is safe against either current node being removed or replaced.
   */
-#define foreach_two_lists(__node1, __list1, __node2, __list2) \
-   for (struct exec_node * __node1 = (__list1)->head,         \
-                         * __node2 = (__list2)->head,         \
-                         * __next1 = __node1->next,           \
-                         * __next2 = __node2->next            \
-    ; __next1 != NULL && __next2 != NULL                  \
-    ; __node1 = __next1,                                  \
-          __node2 = __next2,                                  \
-          __next1 = __next1->next,                            \
-          __next2 = __next2->next)
+#define foreach_two_lists(__node1, __list1, __node2, __list2)     \
+   for (struct exec_node * __node1 = (__list1)->head,             \
+                         * __node2 = (__list2)->head,             \
+                         * __next1 = exec_node_get_next(__node1), \
+                         * __next2 = exec_node_get_next(__node2)  \
+    ; __next1 != NULL && __next2 != NULL                      \
+    ; __node1 = __next1,                                      \
+          __node2 = __next2,                                      \
+          __next1 = exec_node_get_next(__next1),                  \
+          __next2 = exec_node_get_next(__next2))

  #define foreach_list_typed(__type, __node, __field, __list)  \
     for (__type * __node =                        \
         exec_node_data(__type, (__list)->head, __field);        \
-    (__node)->__field.next != NULL;                 \
+        ! exec_node_is_tail_sentinel(exec_node_from_data(__type,        \
+           __node, __field));                                            \
      (__node) = exec_node_data(__type, (__node)->__field.next, __field))

  #define foreach_list_typed_reverse(__type, __node, __field, 
__list)        \
-   for (__type * __node =                                                \
-           exec_node_data(__type, (__list)->tail_pred, __field);        \
-        (__node)->__field.prev != NULL;                                 \
+   for (__type * __node 
=                                                  \
+           exec_node_data(__type, (__list)->tail_pred, 
__field);           \
+        ! 
exec_node_is_head_sentinel(exec_node_from_data(__type,           \
+           __node, 
__field));                                              \
          (__node) = exec_node_data(__type, (__node)->__field.prev, 
__field))

  #define foreach_list_typed_safe(__type, __node, __field, 
__list)           \
@@ -684,7 +719,8 @@ inline void exec_node::insert_before(exec_list *before)
             exec_node_data(__type, (__list)->head, 
__field),                \
                 * __next 
=                                                  \
             exec_node_data(__type, (__node)->__field.next, 
__field);        \
-        (__node)->__field.next != 
NULL;                                    \
+        ! 
exec_node_is_tail_sentinel(exec_node_from_data(__type,           \
+           __node, 
__field));                                              \
          __node = __next, __next 
=                                          \
             exec_node_data(__type, (__next)->__field.next, __field))

@@ -693,7 +729,8 @@ inline void exec_node::insert_before(exec_list *before)
             exec_node_data(__type, (__list)->tail_pred, 
__field),           \
                 * __prev 
=                                                  \
             exec_node_data(__type, (__node)->__field.prev, 
__field);        \
-        (__node)->__field.prev != 
NULL;                                    \
+        ! 
exec_node_is_tail_sentinel(exec_node_from_data(__type,           \
+           __node, 
__field));                                              \
          __node = __prev, __prev 
=                                          \
             exec_node_data(__type, (__prev)->__field.prev, __field))

-- 
2.3.1



More information about the mesa-dev mailing list