<div dir="ltr">On 25 November 2013 15:02, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
<br>
> In commit 065da16 (glsl: Convert lower_clip_distance_visitor to be an<br>
> ir_rvalue_visitor), we failed to notice that since<br>
> lower_clip_distance_visitor overrides visit_leave(ir_assignment *),<br>
> ir_rvalue_visitor::visit_leave(ir_assignment *) wasn't getting called.<br>
> As a result, clip distance dereferences appearing directly on the<br>
> right hand side of an assignment (not in a subexpression) weren't<br>
> getting properly lowered.  This caused an ir_dereference_variable node<br>
> to be left in the IR that referred to the old gl_ClipDistance<br>
> variable.  However, since the lowering pass replaces gl_ClipDistance<br>
> with gl_ClipDistanceMESA, this turned into a dangling pointer when the<br>
> IR got reparented.<br>
><br>
> Prior to the introduction of geometry shaders, this bug was unlikely<br>
> to arise, because (a) reading from gl_ClipDistance[i] in the fragment<br>
> shader was rare, and (b) when it happened, it was likely that it would<br>
> either appear in a subexpression, or be hoisted into a subexpression<br>
> by tree grafting.<br>
><br>
> However, in a geometry shader, we're likely to see a statement like<br>
> this, which would trigger the bug:<br>
><br>
>     gl_ClipDistance[i] = gl_in[j].gl_ClipDistance[i];<br>
<br>
</div>These two are:<br>
<br>
Reviewed-by: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>><br>
<br>
I didn't follow how gl_in[j].gl_ClipDistance[i] is a bare struct deref<br>
of the thing needing to be lowered, since it reads to me like there's an<br>
array and struct dereference first.  But I assume that's just me not<br>
understanding how gl_in[] works.<br>
</blockquote></div><br></div><div class="gmail_extra">Yeah, it's complicated by the fact that by the time we get to this code, lower_named_interface_blocks.cpp has munged<br><br>gl_ClipDistance[i] = gl_in[j].gl_ClipDistance[i]<br>
<br>into:<br><br>gl_ClipDistance[i] = gl_ClipDistance[j][i]<br><br>(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).<br>
<br>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.<br>
<br></div><div class="gmail_extra">Anyway, thanks for hte review :)<br></div></div>