<div dir="ltr">On 6 September 2013 16:21, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 09/06/2013 03:00 PM, Paul Berry wrote: > On 4 September 2013 12:57, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> </div><div class="im">> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote: > > From: Gregory Hainaut <<a href="mailto:gregory.hainaut@gmail.com">gregory.hainaut@gmail.com</a> </div>> <mailto:<a href="mailto:gregory.hainaut@gmail.com">gregory.hainaut@gmail.com</a>>> <div><div class="h5">> > Check the pipeline object state creation. glGenProgramPipelines only > reserves the names. The object does not "exist" until it is bound. > > Also take the opportunity to test negative value on > glGenProgramPipelines and glDeleteProgramPipelines. > > This test fails on AMD's closed-source driver (Catalyst 13.4 on > HD5770). The test crashes when -1 is passed to glGenProgramPipelines. > They are presumably treating the GLsizei as unsigned and generating a > lot of names. They also return GL_TRUE from glIsProgramPipeline > before the object has been bound. > > This test fails on NVIDIA's closed-source driver (unknown version on > GTX 650). The test crashes in glGetProgramPipelineiv for a pipeline > that has never been bound. They are presumably dereferencing a NULL > pointer in some internal data structure. They also return GL_TRUE > from glIsProgramPipeline before the object has been bound. > > V4: split the new test in a separate commit > > V5 (idr): > * Use 'pass = foo() && pass;' idiom instead of 'pass &= foo();'. C's > short-circuit evaulation rules cause these to be different. > * s/GLboolean/bool/g > * Produce no output with -auto. > * Add test to CMakeLists.gl.txt. > * Trivial reformatting. > --- > tests/all.tests | 1 + > .../arb_separate_shader_objects/CMakeLists.gl.txt | 1 + > .../IsProgramPipeline.c | 138 > +++++++++++++++++++++ > 3 files changed, 140 insertions(+) > create mode 100644 > tests/spec/arb_separate_shader_objects/IsProgramPipeline.c > > diff --git a/tests/all.tests b/tests/all.tests > index 9464244..fe24224 100644 > --- a/tests/all.tests > +++ b/tests/all.tests > @@ -1238,6 +1238,7 @@ add_concurrent_test(arb_occlusion_query, > 'occlusion_query_order') > arb_separate_shader_objects = Group() > spec['ARB_separate_shader_objects'] = arb_separate_shader_objects > arb_separate_shader_objects['GetProgramPipelineiv'] = > concurrent_test('arb_separate_shader_object-GetProgramPipelineiv') > +arb_separate_shader_objects['IsProgramPipeline'] = > concurrent_test('arb_separate_shader_object-IsProgramPipeline') > > # Group ARB_sampler_objects > arb_sampler_objects = Group() > diff --git > a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > index 2ea2c80..fa955c6 100644 > --- a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt > @@ -10,3 +10,4 @@ link_libraries ( > ) > > piglit_add_executable > (arb_separate_shader_object-GetProgramPipelineiv GetProgramPipelineiv.c) > +piglit_add_executable (arb_separate_shader_object-IsProgramPipeline > IsProgramPipeline.c) > diff --git > a/tests/spec/arb_separate_shader_objects/IsProgramPipeline.c > b/tests/spec/arb_separate_shader_objects/IsProgramPipeline.c > new file mode 100644 > index 0000000..a0f561a > --- /dev/null > +++ b/tests/spec/arb_separate_shader_objects/IsProgramPipeline.c > @@ -0,0 +1,138 @@ > +/* </div></div>> + * Copyright Š 2013 Gregory Hainaut <<a href="mailto:gregory.hainaut@gmail.com">gregory.hainaut@gmail.com</a> > <mailto:<a href="mailto:gregory.hainaut@gmail.com">gregory.hainaut@gmail.com</a>>> <div><div class="h5">> + * > + * Permission is hereby granted, free of charge, to any person > obtaining a > + * copy of this software and associated documentation files (the > "Software"), > + * to deal in the Software without restriction, including without > limitation > + * the rights to use, copy, modify, merge, publish, distribute, > sublicense, > + * and/or sell copies of the Software, and to permit persons to > whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > the next > + * paragraph) shall be included in all copies or substantial > portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, > DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > OTHER DEALINGS > + * IN THE SOFTWARE. > + */ > + > +/** > + * \name IsProgramPipeline.c > + * Verify correct behavior of glIsProgramPipeline relative to when > a pipeline > + * actually starts to exist. > + * > + * Also verify that glGenProgramPipelines and > glDeleteProgramPipelines with > + * negative counts function correctly. > + * > + * Section 2.11.4 (Program Pipeline Objects) of the OpenGL 4.1 spec > says: > + * > + * "The command > + * > + * void GenProgramPipelines( sizei n, uint *pipelines ); > + * > + * returns n previously unused program pipeline object names in > + * pipelines. These names are marked as used, for the purposes of > + * GenProgramPipelines only, but they acquire state only when > they are > + * first bound." > + */ > + > +#include "piglit-util-gl-common.h" > + > +PIGLIT_GL_TEST_CONFIG_BEGIN > + > + config.supports_gl_compat_version = 10; > + > + config.window_width = 32; > + config.window_height = 32; > + > +PIGLIT_GL_TEST_CONFIG_END > + > +static bool pass; > + > +enum piglit_result > +piglit_display(void) > +{ > + /* UNREACHED */ > + return PIGLIT_FAIL; > +} > + > +static void > +IsProgramPipeline(GLuint pipe, GLboolean expected) > +{ > + GLboolean status = glIsProgramPipeline(pipe); > + > + if (status != expected) { > + pass = GL_FALSE; > + > + fprintf(stderr, > + "Pipeline %d has wrong IsProgramPipeline. " > + "Expected %d, got %d\n", > + pipe, expected, status); > + } > +} > + > +void > +piglit_init(int argc, char **argv) > +{ > + GLuint id[4]; > + int i; > + GLint dummy; > + > + piglit_require_gl_version(20); </div></div>And it looks like this can use the config.supports_gl_compat_version change too. <div class="im"> > + piglit_require_extension("GL_ARB_separate_shader_objects"); > + > + pass = true; > + > + assert(glGetError() == GL_NO_ERROR); > + > + if (!piglit_automatic) > + printf("glGenProgramPipelines with negative n value\n"); > > > As with the previous patch, I think summary messages like these should > be printed regardless of whether we're runing in "automatic" mode. </div>I'm working on a bunch of stuff for the previous patch, but I wanted to reply to this comment sooner rather than later. We /used to/ try to have tests produce as little output as possible with -auto, but generate information helpful to debugging without -auto. </blockquote><div> </div><div>Can you help me understand the motivation for this? I realize that it's nice to have a test print less chatty output when it passes, but it seems to me that conditioning the extra output on automatic/manual mode makes the benefit very tiny, because 99% of the time we are running a test in automatic mode we're running it under piglit-run.py, so its output is being collected in a file. We pretty much only click through to see the output is when the test fails, and in that case the extra chattiness is usually helpful because it helps us determine the exact point in the test where the failure occurred. That's especially important for tests that fail sporadically, because re-running the test in the event of a failure is not always a viable option. In light of that, I'm actually not concerned about this issue on patch 3 anymore, because piglit_check_gl_error prints the filename and line number when there is a failure, so the extra printfs aren't needed to figure out exactly when the failure occurred. Patch 3 gets my reviewed-by as is. I stand by my concern for patch 2 though, because patch 2 calls piglit_check_gl_error from inside functions that get called multiple times, so the printfs really are needed to figure out which stage provoked the failure. In point of fact, patch 2 probably could use an additional printf inside check_stage(). </div><div>I really wish Piglit had a feature like gtest's "SCOPED_TRACE" macro (<a href="http://code.google.com/p/googletest/wiki/AdvancedGuide#Adding_Traces_to_Assertions">http://code.google.com/p/googletest/wiki/AdvancedGuide#Adding_Traces_to_Assertions</a>). That would address my needs without producing extra chatty output in the event of a passing test. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> That said, I actually think this test (and the other ones) should use piglit_report_subtest_result instead. Would that be better? </blockquote><div> </div><div>Using piglit_report_subtest_result would certainly address my concerns, but I don't have strong feelings about it being necessary. For patch 2 I'd be happy either with unconditional printf approach or the subtest approach. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > With that fixed, this patch is: > > Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> </div>> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>> <div><div class="h5">> > > + > + glGenProgramPipelines(-1, id); > + pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass; > + > + if (!piglit_automatic) > + printf("glGenProgramPipelines with correct n value\n"); > + > + glGenProgramPipelines(4, id); > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; > + > + for (i = 0; i < 4 ; i++) { > + IsProgramPipeline(id[i], GL_FALSE); > + } > + > + glBindProgramPipeline(id[0]); > + glUseProgramStages(id[1], GL_ALL_SHADER_BITS, 0); > + glActiveShaderProgram(id[2], 0); > + glGetProgramPipelineiv(id[3], GL_VERTEX_SHADER, &dummy); > + > + /* Flush any errors. The goal is to check that object acquire a > + * state. > + */ > + while (glGetError() != GL_NO_ERROR) > + ; > + > + for (i = 0; i < 4 ; i++) { > + IsProgramPipeline(id[i], GL_TRUE); > + } > + > + if (!piglit_automatic) > + printf("glDeleteProgramPipelines with negative n > value\n"); > + > + glDeleteProgramPipelines(-1, id); > + pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass; > + > + if (!piglit_automatic) > + printf("glDeleteProgramPipelines with correct n > value\n"); > + > + glDeleteProgramPipelines(4, id); > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass; > + > + piglit_present_results(); > + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL); > +} > -- > 1.8.1.4 > > _______________________________________________ > Piglit mailing list </div></div>> <a href="mailto:Piglit@lists.freedesktop.org">Piglit@lists.freedesktop.org</a> <mailto:<a href="mailto:Piglit@lists.freedesktop.org">Piglit@lists.freedesktop.org</a>> > <a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a> </blockquote></div> </div></div>