On 21 August 2013 09:08, Ian Romanick <i...@freedesktop.org> wrote: > From: Ian Romanick <ian.d.roman...@intel.com> > > This test works with both GL_PROGRAM_BINARY_FORMATS (from > ARB_get_program_binary) and GL_SHADER_BINARY_FORMATS (from > ARB_ES2_compatibility). Mesa fails GL_SHADER_BINARY_FORMATS because it > generates an error (!) for the queries. Fix the error causes it to > over-run the buffer. I suspect the over-run will happen in an ES2 with > Mesa as-is. > > Signed-off-by: Ian Romanick <ian.d.roman...@intel.com> >
I'm not a fan of the one-word function names in this patch, especially "munge". How about something like: munge -> fill_with_garbage check -> check_overrun try -> test_params > --- > tests/all.tests | 7 + > tests/spec/CMakeLists.txt | 1 + > .../spec/arb_get_program_binary/CMakeLists.gl.txt | 14 ++ > tests/spec/arb_get_program_binary/CMakeLists.txt | 1 + > tests/spec/arb_get_program_binary/overrun.c | 156 > +++++++++++++++++++++ > 5 files changed, 179 insertions(+) > create mode 100644 tests/spec/arb_get_program_binary/CMakeLists.gl.txt > create mode 100644 tests/spec/arb_get_program_binary/CMakeLists.txt > create mode 100644 tests/spec/arb_get_program_binary/overrun.c > > diff --git a/tests/all.tests b/tests/all.tests > index ab75f99..fa3812f 100644 > --- a/tests/all.tests > +++ b/tests/all.tests > @@ -1031,6 +1031,13 @@ arb_es2_compatibility['FBO blit to missing > attachment (ES2 completeness rules)'] > arb_es2_compatibility['FBO blit from missing attachment (ES2 completeness > rules)'] = PlainExecTest(['fbo-missing-attachment-blit', '-auto', 'es2', > 'from']) > add_fbo_formats_tests('spec/ARB_ES2_compatibility', > 'GL_ARB_ES2_compatibility') > add_texwrap_format_tests(arb_es2_compatibility, > 'GL_ARB_ES2_compatibility') > +arb_es2_compatibility['NUM_SHADER_BINARY_FORMATS over-run check'] = > concurrent_test('arb_get_program_binary-overrun shader') > + > + > +# Group ARB_get_program_binary > +arb_get_program_binary = Group() > +spec['ARB_get_program_binary'] = arb_get_program_binary > +arb_get_program_binary['NUM_PROGRAM_BINARY_FORMATS over-run check'] = > concurrent_test('arb_get_program_binary-overrun program') > > arb_depth_clamp = Group() > spec['ARB_depth_clamp'] = arb_depth_clamp > diff --git a/tests/spec/CMakeLists.txt b/tests/spec/CMakeLists.txt > index 77ed821..9016c9e 100644 > --- a/tests/spec/CMakeLists.txt > +++ b/tests/spec/CMakeLists.txt > @@ -7,6 +7,7 @@ add_subdirectory (arb_es2_compatibility) > add_subdirectory (arb_framebuffer_object) > add_subdirectory (arb_framebuffer_srgb) > add_subdirectory (arb_geometry_shader4) > +add_subdirectory (arb_get_program_binary) > add_subdirectory (arb_instanced_arrays) > add_subdirectory (arb_internalformat_query) > add_subdirectory (arb_map_buffer_alignment) > diff --git a/tests/spec/arb_get_program_binary/CMakeLists.gl.txt > b/tests/spec/arb_get_program_binary/CMakeLists.gl.txt > new file mode 100644 > index 0000000..be52044 > --- /dev/null > +++ b/tests/spec/arb_get_program_binary/CMakeLists.gl.txt > @@ -0,0 +1,14 @@ > +include_directories( > + ${GLEXT_INCLUDE_DIR} > + ${OPENGL_INCLUDE_PATH} > +) > + > +link_libraries ( > + piglitutil_${piglit_target_api} > + ${OPENGL_gl_LIBRARY} > + ${OPENGL_glu_LIBRARY} > +) > + > +piglit_add_executable (arb_get_program_binary-overrun overrun.c) > + > +# vim: ft=cmake: > diff --git a/tests/spec/arb_get_program_binary/CMakeLists.txt > b/tests/spec/arb_get_program_binary/CMakeLists.txt > new file mode 100644 > index 0000000..144a306 > --- /dev/null > +++ b/tests/spec/arb_get_program_binary/CMakeLists.txt > @@ -0,0 +1 @@ > +piglit_include_target_api() > diff --git a/tests/spec/arb_get_program_binary/overrun.c > b/tests/spec/arb_get_program_binary/overrun.c > new file mode 100644 > index 0000000..8416677 > --- /dev/null > +++ b/tests/spec/arb_get_program_binary/overrun.c > @@ -0,0 +1,156 @@ > +/* > + * Copyright © 2012 Intel Corporation > + * > + * 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. > + */ > + > +/** > + * \file overrun.c > + * Verify that queries don't over-run the size of the supplied buffer. > + */ > + > +#include "piglit-util-gl-common.h" > +#include <stdlib.h> > + > +PIGLIT_GL_TEST_CONFIG_BEGIN > + > + config.supports_gl_compat_version = 10; > + config.window_visual = PIGLIT_GL_VISUAL_RGB; > + > +PIGLIT_GL_TEST_CONFIG_END > + > +enum piglit_result > +piglit_display(void) > +{ > + return PIGLIT_FAIL; > +} > + > +/** > + * Fill the real buffer and the scratch buffer with the same set of > + * garbage data. > + */ > +static void > +munge(void *buffer, void *buffer_scratch, size_t buffer_size_in_bytes) > +{ > + unsigned i; > + uint32_t *scratch = (uint32_t *) buffer; > + > + assert(buffer_size_in_bytes % sizeof(uint32_t) == 0); > + for (i = 0; i < (buffer_size_in_bytes / sizeof(uint32_t)); i++) > + do { > + scratch[i] = rand(); > In my experience, tests that rely on rand() are generally a bad idea because they can lead to inconsistent results from one run to the next due to concidences. I realize that's unlikely with this test, but I would feel much more comfortable if we would just fill the buffer with a recognizable bit pattern (e.g. 0xcc) rather than random data. > + } while (scratch[i] == 0); > + > + memcpy(buffer_scratch, buffer, buffer_size_in_bytes); > +} > + > +static bool > +check(const void *buffer, const void *buffer_scratch, > + size_t buffer_size_in_bytes, size_t data_size_in_bytes, > + const char *getter_name, const char *enum_name) > +{ > + if (memcmp(buffer + data_size_in_bytes, > + buffer_scratch + data_size_in_bytes, > + buffer_size_in_bytes - data_size_in_bytes) == 0) > + return true; > + > + fprintf(stderr, "%s(%s) over-ran the buffer\n", > + getter_name, enum_name); > + return false; > +} > + > +static bool > +try(GLenum pname1, GLenum pname2) > This function would be a lot easier to read if we renamed pname1 and pname2 to reflect how they are used, e.g.: pname1 -> num_binary_formats_enum pname2 -> binary_formats_enum > +{ > + bool pass = true; > + GLint count = 0; > + char *buffer; > + char *buffer_scratch; > + unsigned buffer_size_in_elements; > + size_t buffer_size_in_bytes; > + > + > + glGetIntegerv(pname1, &count); > + > + if (!piglit_check_gl_error(0)) > + return false; > + > + if (count < 0) { > + fprintf(stderr, "%s returned %d\n", > + piglit_get_gl_enum_name(pname1), > + count); > + return false; > + } > + > + /* Both of the queries can return zero, so make sure that at least > one > + * element is allocated. We need to check that when the first > query > + * returns zero, the second query doesn't write any data. > + */ > + buffer_size_in_elements = (count + 1) * 2; > + buffer_size_in_bytes = buffer_size_in_elements * sizeof(GLint64); > + buffer = malloc(buffer_size_in_bytes); > + buffer_scratch = malloc(buffer_size_in_bytes); > + > +#define TEST_CASE(fn, type) \ > + do { \ > + munge(buffer, buffer_scratch, buffer_size_in_bytes); \ > + fn (pname2, (type *) buffer); \ > + pass = piglit_check_gl_error(0) && pass; \ > + pass = check(buffer, buffer_scratch, \ > + buffer_size_in_bytes, \ > + sizeof(type) * count, \ > + # fn, \ > + piglit_get_gl_enum_name(pname2)) \ > + && pass; \ > + } while (0) > + > + TEST_CASE(glGetBooleanv, GLboolean); > + TEST_CASE(glGetIntegerv, GLint); > + TEST_CASE(glGetInteger64v, GLint64); > + TEST_CASE(glGetFloatv, GLfloat); > + TEST_CASE(glGetDoublev, GLdouble); > + > + free(buffer); > + free(buffer_scratch); > + return pass; > +} > + > +void > +piglit_init(int argc, char **argv) > +{ > + bool pass = true; > + > + if (argc == 1) { > + fprintf(stderr, "Usage: %s [shader|program]\n", argv[0]); > + piglit_report_result(PIGLIT_FAIL); > + } > + > + if (strcmp(argv[1], "shader") == 0) { > + piglit_require_extension("GL_ARB_ES2_compatibility"); > + pass = try(GL_NUM_SHADER_BINARY_FORMATS, > + GL_SHADER_BINARY_FORMATS); > + } else { > + piglit_require_extension("GL_ARB_get_program_binary"); > + pass = try(GL_NUM_PROGRAM_BINARY_FORMATS, > + GL_PROGRAM_BINARY_FORMATS); > + } > + > + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL); > +} > -- > 1.8.1.4 > With those changes, the patch is: Reviewed-by: Paul Berry <stereotype...@gmail.com>
_______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit