[Mesa-dev] [PATCH] Fix strict-aliasing violations in GLSL shader list implementation

[Ilia Mirkin]

On Wed, Jun 24, 2015 at 5:05 PM, Davin McCall <davmac at davmac.org> wrote:
> Hi - I'm new here.
>
> I've recently started poking the Mesa codebase for little reason other than
> personal interest. In the "help wanted" section of the website it mentions
> aliasing violations as a target for newcomers to fix, so with that in mind
> I've attached a patch (against git head) which resolves a few of them, by
> targeting the linked list implementation (list.h) used in the GLSL
> compiler/optimizers. This change slightly increases the storage requirements
> for a list (adds one word) but resolves the blatant aliasing violation that
> was caused by the trick used to conserve that word in the first place.
>
> (I toyed with another approach - using a single sentinel node for both the
> head and tail of a list - but this was much more invasive, and meant that
> you could no longer check whether a particular node was a sentinel node
> unless you had a reference to the list, so I gave up and went with this
> simpler approach).
>
> The most essential change is in the 'exec_list' structure. Three fields
> 'head', 'tail' and 'tail_pred' are removed, and two separate sentinel nodes
> are inserted in their place. The old 'head' is replaced by
> 'head_sentinel.next', 'tail_pred' by 'tail_sentinel.prev', and tail (always
> NULL) by 'head_sentinel.prev' and 'tail_sentinel.next' (both always NULL).
>
> With this patch, I can build a working (though perhaps not 100% bug-free)
> Mesa without using -fno-strict-aliasing during compilation. Before the
> patch, applications using Mesa would hang at runtime.
>
> I don't really know the process you have in place so if I need to do
> anything else to get this patch into the codebase please let me know. Any
> comments and criticisms welcome.

The biggest part of the process is to send a proper commit to the
mailing list. There are two issues with your mailing (without
commenting on your actual changes) --

(a) The patch is attached, not inlined
(b) You sent a diff, not a commit

Both of these are easily resolved by using 'git send-email' for your
patches, but that's not a strict requirement. See
http://mesa3d.org/devinfo.html#submitting .

As for feedback on your actual patch, you may want to explain why you
did the things you did, what the old world was and what the new world
is, and why the old world was wrong. You did that to some extent, and
perhaps I'm just too unfamiliar with exec_list to make sense of it
while others will "get it". Perhaps specific warnings generated by GCC
may be able to better illustrate what problem you're trying to solve.

Since you're increasing the storage requirements of a pretty basic
unit of storage in mesa, it may also be interesting to see memory
usage of some non-trivial trace or game before & after. While I think
that's optional, it's definitely a nice-to-have.

Cheers,

  -ilia