Bug 97232

I played around with this on gen6. Unfortunately the attached trace is unusable there due to the #version 400 selection, however building dolphin from the referenced PR was able to reproduce the issue.

Not sure if it's related, but I'm seeing a read-out-of-bounds when running valgrind on a trace generated on gen6:

==18850== Memcheck, a memory error detector
==18850== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==18850== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==18850== Command: glretrace dolphin-emu-nogui.trace
==18850== 
==18850== Invalid read of size 4
==18850==    at 0xA687DEA: brw::vec4_visitor::var_range_end(unsigned int, unsigned int) const (brw_vec4_live_variables.cpp:335)
==18850==    by 0xA67B77C: brw::vec4_visitor::opt_cse_local(bblock_t*) (brw_vec4_cse.cpp:249)
==18850==    by 0xA67BD7A: brw::vec4_visitor::opt_cse() (brw_vec4_cse.cpp:274)
==18850==    by 0xA6779A5: brw::vec4_visitor::run() [clone .part.22] (brw_vec4.cpp:1988)
==18850==    by 0xA678677: brw_compile_vs (brw_vec4.cpp:2173)
==18850==    by 0xA5A85B5: brw_codegen_vs_prog (brw_vs.c:194)
==18850==    by 0xA5A8BDD: brw_vs_precompile (brw_vs.c:405)
==18850==    by 0xA5909B9: brw_shader_precompile (brw_link.cpp:65)
==18850==    by 0xA5909B9: brw_link_shader (brw_link.cpp:283)
==18850==    by 0xA41F715: _mesa_glsl_link_shader (ir_to_mesa.cpp:3070)
==18850==    by 0xA31F9AD: _mesa_link_program.part.22 (shaderapi.c:1093)
==18850==    by 0x58163D: retrace_glLinkProgram(trace::Call&) (in /usr/bin/glretrace)
==18850==    by 0x40CD18: retrace::retraceCall(trace::Call*) (in /usr/bin/glretrace)
==18850==  Address 0xd190280 is 0 bytes after a block of size 320 alloc'd
==18850==    at 0x4C2C070: calloc (vg_replace_malloc.c:623)
==18850==    by 0xA4C6E61: rzalloc_size (ralloc.c:125)
==18850==    by 0xA6878EC: brw::vec4_visitor::calculate_live_intervals() (brw_vec4_live_variables.cpp:242)
==18850==    by 0xA67BD5D: brw::vec4_visitor::opt_cse() (brw_vec4_cse.cpp:271)
==18850==    by 0xA6779A5: brw::vec4_visitor::run() [clone .part.22] (brw_vec4.cpp:1988)
==18850==    by 0xA678677: brw_compile_vs (brw_vec4.cpp:2173)
==18850==    by 0xA5A85B5: brw_codegen_vs_prog (brw_vs.c:194)
==18850==    by 0xA5A8BDD: brw_vs_precompile (brw_vs.c:405)
==18850==    by 0xA5909B9: brw_shader_precompile (brw_link.cpp:65)
==18850==    by 0xA5909B9: brw_link_shader (brw_link.cpp:283)
==18850==    by 0xA41F715: _mesa_glsl_link_shader (ir_to_mesa.cpp:3070)
==18850==    by 0xA31F9AD: _mesa_link_program.part.22 (shaderapi.c:1093)
==18850==    by 0x58163D: retrace_glLinkProgram(trace::Call&) (in /usr/bin/glretrace)

However commenting out opt_cse does not resolve the (underlying) issue.

A separate thought is that only gl_ClipDistance[0] is written, and there is some indication that if any of the clip distances are NaN, the whole primitive gets rejected. However also writing distances 1,2,3 didn't help, nor did writing 4..7. (Although it did appear to stop flickering a wrong primitive for one frame in that situation... all black now.)

(In reply to Ilia Mirkin from comment #1)
> A separate thought is that only gl_ClipDistance[0] is written, and there is
> some indication that if any of the clip distances are NaN, the whole
> primitive gets rejected. However also writing distances 1,2,3 didn't help,
> nor did writing 4..7. (Although it did appear to stop flickering a wrong
> primitive for one frame in that situation... all black now.)

I've only enabled the first clipping plane. According to the specs: https://www.opengl.org/sdk/docs/man/html/gl_ClipDistance.xhtml

    Values written into gl_ClipDistance planes that are not enabled have no effect.

So I highly doubt this is affecting anything, unless the spec is not implemented correctly.

(In reply to Jules Blok from comment #2)
> So I highly doubt this is affecting anything, unless the spec is not
> implemented correctly.

It's been known to happen :)

Taking a closer look, it appears that you write gl_ClipDistance[0] in the vertex shader but not in the geometry shader. I don't think that's supposed to work -- you have to write the clip distance in the last pre-rast stage in order for it to have any effect.

Curiously, with the below patch on top of what's in the dolphin pr, that still fails on i965/snb, and still passes with llvmpipe.

diff --git a/Source/Core/VideoCommon/GeometryShaderGen.cpp b/Source/Core/VideoCommon/GeometryShaderGen.cpp
index 4d90e6f..3cbe9ce 100644
--- a/Source/Core/VideoCommon/GeometryShaderGen.cpp
+++ b/Source/Core/VideoCommon/GeometryShaderGen.cpp
@@ -311,6 +311,9 @@ static void EmitVertex(ShaderCode& out, const geometry_shader_uid_data* uid_data
 
   if (ApiType == APIType::OpenGL)
   {
+    if (g_ActiveConfig.backend_info.bSupportsDepthClamp)
+      out.Write("\tgl_ClipDistance[0] = %s.clipDist;\n", vertex);
+
     out.Write("\tgl_Position = %s.pos;\n", vertex);
     AssignVSOutputMembers(out, "ps", vertex, uid_data->numTexGens, uid_data->pixel_lighting);
   }

(In reply to Ilia Mirkin from comment #5)
> Curiously, with the below patch on top of what's in the dolphin pr, that
> still fails on i965/snb, and still passes with llvmpipe.

I should've mentioned that I've already tried that in the past, it didn't help.

Created attachment 125672 [details]
apitrace file llvmpipe

I've attached a new trace file captured with llvmpipe instead of the nvidia proprietary driver.

In fact, the first capture was done on Windows, not the nvidia driver on Linux.

(In reply to Jules Blok from comment #6)
> (In reply to Ilia Mirkin from comment #5)
> > Curiously, with the below patch on top of what's in the dolphin pr, that
> > still fails on i965/snb, and still passes with llvmpipe.
> 
> I should've mentioned that I've already tried that in the past, it didn't
> help.

Irrespective of that, the current logic in the PR in question is strictly wrong. The fact that it renders anything anywhere when a GS is active is relying on undefined behavior (since clip distance 0 is enabled, and nothing is written to it from the last stage).

I wasn't sure whether I had to pass through the clipping distance, the spec wasn't very clear on that. I've changed the shader according to your patch to conform to that.

(In reply to Ilia Mirkin from comment #9)
> Irrespective of that, the current logic in the PR in question is strictly
> wrong. The fact that it renders anything anywhere when a GS is active is
> relying on undefined behavior (since clip distance 0 is enabled, and nothing
> is written to it from the last stage).

FTR, looks like the most recent version of the PR has fixed that.

(In reply to Jules Blok from comment #10)
> I wasn't sure whether I had to pass through the clipping distance, the spec
> wasn't very clear on that. I've changed the shader according to your patch
> to conform to that.

Well, what would the clip distance value be if you didn't? You're generating any number of fresh vertices, and the clip distance is a per-vertex attrib. There's no real other way for it to work.

(In reply to Ilia Mirkin from comment #12)
> Well, what would the clip distance value be if you didn't? You're generating
> any number of fresh vertices, and the clip distance is a per-vertex attrib.
> There's no real other way for it to work.

I assumed that, since I didn't actually change gl_ClipDistance in the geom shader, it would just continue to use the value I had already set on the input vertex. And it looks like most drivers do handle it like that, but as you said that is likely undefined behavior.

In fact now that I read the reference again, that is explicitly undefined behaviour:

> It is also undefined after the geometry processing stage if the geometry shader executable calls EmitVertex without having written gl_ClipDistance since the last call to EmitVertex (or hasn't written it at all).

Created attachment 125713 [details]
apitrace file i965

Ilia Mirkin found some application bugs, these have been fixed and a new trace has been recorded on i965. It does not fix the issue however.

Through some testing we've found that merely writing to gl_ClipDistance will break the geometry shader. Even when GL_CLIP_DISTANCE0 has been disabled and such writes should be ignored according to the spec.

Created attachment 125715 [details]
apitrace file i965

This trace file was captured with the latest apitrace version.

Created attachment 125716 [details]
apitrace file i965

Looks like the previous file got corrupted somehow.

A curious observation:

tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test

passes on intel. With the following diff:

diff --git a/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test b/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test
index 26ea096..46b09be 100644
--- a/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test
+++ b/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test
@@ -11,8 +11,12 @@ GLSL >= 1.50
 [vertex shader]
 #version 150
 
+out vec4 c;
+
 void main()
 {
+  //gl_ClipDistance[0] = -1;
+  c = vec4(0,1,0,1);
 }
 
 [geometry shader]
@@ -20,7 +24,10 @@ void main()
 
 layout(points) in;
 layout(triangle_strip, max_vertices = 4) out;
-float gl_ClipDistance[8];
+out float gl_ClipDistance[8];
+
+in vec4 c[];
+out vec4 color;
 
 struct vertex_data {
   vec4 position;
@@ -43,16 +50,20 @@ void main()
   for (int i = 0; i < 4; i++) {
     gl_Position = vdata[i].position;
     gl_ClipDistance = vdata[i].clip_distance;
+    color = c[0];
     EmitVertex();
   }
+  EndPrimitive();
 }
 
 [fragment shader]
 #version 150
 
+in vec4 color;
+
 void main()
 {
-  gl_FragColor = vec4(1.0);
+  gl_FragColor = color;
 }
 
 [test]


it still passes. (Well, it fails, because I wrote in a different color, but that's a side issue.)

However uncommenting the gl_ClipDistance[] in the vertex shader makes it render all black. Using gl_ClipDistance[0] = 1 makes it render red. So the VUE map somehow gets messed up when the vertex shader is emitting a clip distance, but that clip distance isn't used due to there being a TES or GS stage in the pipeline.

> but that clip distance isn't used due to there being a TES or GS stage in the pipeline.

So a possible workaround for this issue is to not write to the clip distance in the vertex shader when a GS stage is active?

VS Output VUE map (4 slots, non-SSO)
  [0] VARYING_SLOT_PSIZ
  [1] VARYING_SLOT_POS
  [2] VARYING_SLOT_CLIP_DIST0
  [3] VARYING_SLOT_VAR0

GS Input VUE map (3 slots, non-SSO)
  [0] VARYING_SLOT_PSIZ
  [1] VARYING_SLOT_POS
  [2] VARYING_SLOT_VAR0

Probably not a great sign. [And yes, avoiding setting gl_ClipDistance in VS when there's a GS present would probably work around this bug.]

Thanks for tracking this down, Ilia - the problem became really obvious once you pointed out that it's a VS -> GS varying problem.  I have some fixes locally - I'm testing them out now.

(In reply to Kenneth Graunke from comment #21)
> Thanks for tracking this down, Ilia - the problem became really obvious once
> you pointed out that it's a VS -> GS varying problem.  I have some fixes
> locally - I'm testing them out now.

I'm guessing that the latest iteration of those is at

https://cgit.freedesktop.org/~kwg/mesa/log/?h=vuemap-fixes

Do they need more work, or did this issue just fall off your radar?

Yeah, those patches are broken.

I took another stab at this tonight, and came up with a different approach I think should work better.  Hopefully...

Fixed by:

commit 44fd85d8eb1fba68829917c0cf5ce052095964ee
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Sun Dec 4 23:54:48 2016 -0800

    i965: Unify shader interfaces explicitly.