[Mesa-dev] [PATCH] draw-robustness: Test robustness for out-of-bounds vertex fetches.

[José Fonseca]

On 03/31/2011 09:45 PM, Ian Romanick wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 03/31/2011 06:46 AM, jfonseca at vmware.com wrote:
>    
>> From: José Fonseca<jfonseca at vmware.com>
>>
>> Not added to the standard test lists given that ARB_vertex_buffer_object
>> allows program termination out-of-bounds vertex buffer object fetches
>> occur.
>>      
> In anticipation of making real GL_ARB_robustness tests, I'd suggest
> putting this in tests/spec/ARB_robustness.  We can add it to the general
> test list once it has ARB_robustness support, which I see listed as a
> todo item.
>    
OK.
> There are a couple comments below.
>
>    
>> ---
>>   tests/general/CMakeLists.gl.txt |    1 +
>>   tests/general/draw-robustness.c |  201 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 202 insertions(+), 0 deletions(-)
>>   create mode 100644 tests/general/draw-robustness.c
>>
>> diff --git a/tests/general/CMakeLists.gl.txt b/tests/general/CMakeLists.gl.txt
>> index bbe6507..d373e35 100644
>> --- a/tests/general/CMakeLists.gl.txt
>> +++ b/tests/general/CMakeLists.gl.txt
>> @@ -36,6 +36,7 @@ ENDIF (UNIX)
>>   add_executable (draw-elements-vs-inputs draw-elements-vs-inputs.c)
>>   add_executable (draw-instanced draw-instanced.c)
>>   add_executable (draw-instanced-divisor draw-instanced-divisor.c)
>> +add_executable (draw-robustness draw-robustness.c)
>>   add_executable (draw-vertices draw-vertices.c)
>>   add_executable (draw-vertices-half-float draw-vertices-half-float.c)
>>   add_executable (fog-modes fog-modes.c)
>> diff --git a/tests/general/draw-robustness.c b/tests/general/draw-robustness.c
>> new file mode 100644
>> index 0000000..a13f568
>> --- /dev/null
>> +++ b/tests/general/draw-robustness.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * Copyright (C) 2011 VMware, Inc.
>> + * Copyright (C) 2010 Marek Olšák<maraeo at gmail.com>
>> + *
>> + * 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.
>> + *
>> + * Authors:
>> + *   Jose Fonseca<jfonseca at vmware.com>
>> + *   Based on code from Marek Olšák<maraeo at gmail.com>
>> + */
>> +
>> +/* Test whether out-of-bounds vertex buffer object cause termination.
>> + *
>> + * Note that the original ARB_vertex_buffer_object extension explicitly states
>> + * program termination is allowed when out-of-bounds vertex buffer object
>> + * fetches occur.  The ARB_robustness extension does provides an enbale to
>> + * guarantee that out-of-bounds buffer object accesses by the GPU will have
>> + * deterministic behavior and preclude application instability or termination
>> + * due to an incorrect buffer access.  But regardless of ARB_robustness
>> + * extension support it is a good idea not to crash.  For example,  viewperf
>> + * doesn't properly detect NV_primitive_restart and emits 0xffffffff indices
>> + * which can result in crashes.
>> + *
>> + * TODO:
>> + * - test out-of-bound index buffer object access
>> + * - test more vertex/element formats
>> + * - test non-aligned offsets
>> + * - provide a command line option to actually enable ARB_robustness
>> + */
>> +
>> +#include "piglit-util.h"
>> +
>> +int piglit_width = 320, piglit_height = 320;
>> +int piglit_window_mode = GLUT_RGB;
>> +
>> +void piglit_init(int argc, char **argv)
>> +{
>> +    piglit_ortho_projection(piglit_width, piglit_height, GL_FALSE);
>> +
>> +    if (!GLEW_VERSION_1_5) {
>> +        printf("Requires OpenGL 1.5\n");
>> +        piglit_report_result(PIGLIT_SKIP);
>> +    }
>> +
>> +    glShadeModel(GL_FLAT);
>> +    glClearColor(0.2, 0.2, 0.2, 1.0);
>> +}
>> +
>> +static void
>> +random_vertices(GLsizei offset, GLsizei stride, GLsizei count)
>> +{
>> +    GLsizei size = offset + (count - 1)*stride + 2 * sizeof(GLfloat);
>> +    GLubyte *vertices;
>> +    GLsizei i;
>> +
>> +    assert(offset % sizeof(GLfloat) == 0);
>> +    assert(stride % sizeof(GLfloat) == 0);
>> +
>> +    vertices = malloc(size);
>> +    assert(vertices);
>> +    if (!vertices) {
>> +        return;
>> +    }
>>      
> Since this is with a VBO, why not use MapBuffer/UnmapBuffer instead of
> malloc/free?
OK.
>    
>> +    if (0) {
>> +        fprintf(stderr, "vertex_offset = %i\n", vertex_offset);
>> +        fprintf(stderr, "vertex_stride = %i\n", vertex_stride);
>> +        fprintf(stderr, "vertex_count = %i\n", vertex_count);
>> +        fprintf(stderr, "index_offset = %i\n", index_offset);
>> +        fprintf(stderr, "index_count = %i\n", index_count);
>> +        fprintf(stderr, "min_index = %u\n", min_index);
>> +        fprintf(stderr, "max_index = %u\n", max_index);
>> +        fprintf(stderr, "\n");
>> +    }
>>      
> This is useful information.  I would suggest:
>
>   - Guard it with 'if (!piglit_automatic)'
>
>   - Print to stdout
>
>   - Add 'fflush(stdout)' to the end of the block.
>    
Ok. will do.
>> +
>> +    glGenBuffers(1,&vertex_buffer);
>> +    glGenBuffers(1,&index_buffer);
>> +    glBindBuffer(GL_ARRAY_BUFFER, vertex_buffer);
>> +    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, index_buffer);
>> +
>> +    random_vertices(vertex_offset, vertex_stride, vertex_count);
>> +
>> +    if (0) {
>> +        /* Generate valid indices only */
>> +        random_ushort_indices(index_offset, index_count, min_index, max_index);
>> +    } else {
>> +        /* Generate out-of-range indices */
>> +        random_ushort_indices(index_offset, index_count, 0, 2*vertex_count - 1);
>> +    }
>> +
>> +    glVertexPointer(2, GL_FLOAT, vertex_stride, (const void*)(intptr_t)vertex_offset);
>> +    glDrawRangeElements(GL_TRIANGLES,
>> +                        min_index,
>> +                        max_index,
>> +                        index_count,
>> +                        GL_UNSIGNED_SHORT,
>> +                        (const void*)(intptr_t)index_offset);
>> +    assert(glGetError() == GL_NO_ERROR);
>> +    glFinish();
>>      
> Why?  Both for this finish and the one below.
>
>    

It helps matching the above printf output with the crashes.  Because 
unlike most other tests this test does not use glReadPixels, so unless 
we call glFinish the draw can be batched and the cpu crash / gpu crash 
could happen much later.


Thanks for all the reviews. I'll follow the suggestions and commit.

Jose