[Mesa-dev] [PATCH 1/2] glsl: Fix lowering of direct assignment in lower_clip_distance.

[Paul Berry]

On 25 November 2013 15:02, Eric Anholt <eric at anholt.net> wrote:

> Paul Berry <stereotype441 at gmail.com> writes:
>
> > In commit 065da16 (glsl: Convert lower_clip_distance_visitor to be an
> > ir_rvalue_visitor), we failed to notice that since
> > lower_clip_distance_visitor overrides visit_leave(ir_assignment *),
> > ir_rvalue_visitor::visit_leave(ir_assignment *) wasn't getting called.
> > As a result, clip distance dereferences appearing directly on the
> > right hand side of an assignment (not in a subexpression) weren't
> > getting properly lowered.  This caused an ir_dereference_variable node
> > to be left in the IR that referred to the old gl_ClipDistance
> > variable.  However, since the lowering pass replaces gl_ClipDistance
> > with gl_ClipDistanceMESA, this turned into a dangling pointer when the
> > IR got reparented.
> >
> > Prior to the introduction of geometry shaders, this bug was unlikely
> > to arise, because (a) reading from gl_ClipDistance[i] in the fragment
> > shader was rare, and (b) when it happened, it was likely that it would
> > either appear in a subexpression, or be hoisted into a subexpression
> > by tree grafting.
> >
> > However, in a geometry shader, we're likely to see a statement like
> > this, which would trigger the bug:
> >
> >     gl_ClipDistance[i] = gl_in[j].gl_ClipDistance[i];
>
> These two are:
>
> Reviewed-by: Eric Anholt <eric at anholt.net>
>
> I didn't follow how gl_in[j].gl_ClipDistance[i] is a bare struct deref
> of the thing needing to be lowered, since it reads to me like there's an
> array and struct dereference first.  But I assume that's just me not
> understanding how gl_in[] works.
>

Yeah, it's complicated by the fact that by the time we get to this code,
lower_named_interface_blocks.cpp has munged

gl_ClipDistance[i] = gl_in[j].gl_ClipDistance[i]

into:

gl_ClipDistance[i] = gl_ClipDistance[j][i]

(note that the "gl_ClipDistance" appearing on the LHS refers to the GS
output var, and the "gl_ClipDistance" appearing on the RHS refers to the GS
input var).

Also, I oversimplified a bit when I said "clip distance dereferences
appearing directly on the right hand side of an assignment"; what I really
meant was "either an array_deref of gl_ClipDistance out, or an array_deref
of an array_deref of gl_ClipDistance in".  But I figured that going into
that level of detail would create more confusion than it solved.

Anyway, thanks for hte review :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131125/af713171/attachment.html>