<div dir="ltr">On 21 August 2013 09:08, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>> 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 <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>> </blockquote><div> </div><div>I'm not a fan of the one-word function names in this patch, especially "munge". How about something like: </div><div>munge -> fill_with_garbage </div><div>check -> check_overrun </div><div>try -> test_params </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- 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) </blockquote></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + 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(); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } 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) </blockquote><div> </div><div>This function would be a lot easier to read if we renamed pname1 and pname2 to reflect how they are used, e.g.: </div><div>pname1 -> num_binary_formats_enum pname2 -> binary_formats_enum </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + 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 </blockquote><div> </div><div>With those changes, the patch is: Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div></div></div> </div>