[Mesa-dev] [RFC PATCH] mesa: Delete VAO _MaxElement code and index buffer bounds checking.

[Brian Paul]

On 09/15/2014 06:00 PM, Roland Scheidegger wrote:
> Am 15.09.2014 08:31, schrieb Kenneth Graunke:
>> Fredrik's implementation of ARB_vertex_attrib_binding introduced new
>> gl_vertex_attrib_array and gl_vertex_buffer_binding structures, and
>> converted Mesa's older gl_client_array to be derived state.  Ultimately,
>> we'd like to drop gl_client_array and use those structures directly.
>>
>> One hitch is that gl_client_array::_MaxElement doesn't correspond to
>> either structure (unlike every other field), so we'd have to figure out
>> where to store it.  The _MaxElement computation uses values from both
>> structures, so it doesn't really belong in either place.  We could put
>> it in the VAO, but we'd have to pass it around everywhere.
>>
>> It turns out that it's only used when ctx->Const.CheckArrayBounds is
>> set, which is only set by the (rarely used) classic swrast driver.
>> It appears that drivers/x11 used to set it as well, which was intended
>> to avoid segmentation faults on out-of-bounds memory access in the X
>> server (probably for indirect GLX clients).  However, ajax deleted that
>> code in 2010 (commit 1ccef926be46dce3b6b5c76e812e2fae4e205ce7).
>>
>> The bounds checking apparently doesn't actually work, either.  Non-VBO
>> attributes arbitrarily set _MaxElement to 2 * 1000 * 1000 * 1000.
>> vbo_save_draw and vbo_exec_draw remark /* ??? */ when setting it, and
>> the i965 code contains a comment noting that _MaxElement is often bogus.
> Well there's not much you can do for the non-vbo case, since you simply
> don't know how large that buffer pointed to by that client pointer you
> were given by the app is...
>
>>
>> Given that the code is complex, rarely used, and dubiously functional,
>> it doesn't seem worth maintaining going forward.  This patch drops it.
>>
>> This will probably mean the classic swrast driver may begin crashing on
>> out of bounds vertex buffer access in some cases, but I believe that is
>> allowed by OpenGL (and probably happened for non-VBO accesses anyway).
>> There do not appear to be any Piglit regressions, either.
>>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>   src/mesa/drivers/dri/i965/brw_draw_upload.c |  5 +--
>>   src/mesa/drivers/dri/swrast/swrast.c        |  3 --
>>   src/mesa/main/api_validate.c                | 66 -----------------------------
>>   src/mesa/main/arrayobj.c                    | 46 --------------------
>>   src/mesa/main/arrayobj.h                    |  4 --
>>   src/mesa/main/attrib.c                      |  1 -
>>   src/mesa/main/context.c                     |  3 --
>>   src/mesa/main/mtypes.h                      | 10 -----
>>   src/mesa/main/state.c                       |  5 ---
>>   src/mesa/main/varray.c                      |  9 +---
>>   src/mesa/main/varray.h                      | 33 ---------------
>>   src/mesa/vbo/vbo_exec_array.c               | 26 +++---------
>>   src/mesa/vbo/vbo_exec_draw.c                |  2 -
>>   src/mesa/vbo/vbo_save_draw.c                |  1 -
>>   src/mesa/vbo/vbo_split_copy.c               |  1 -
>>   15 files changed, 9 insertions(+), 206 deletions(-)
>>
>> Hi Brian, Roland, Eric, all...
>>
>> What do you think of this idea?  Good idea?  Terrible idea?
>>
>> In theory, this seems like a reasonable idea for software drivers, but
>> it doesn't appear that softpipe/llvmpipe use this code, so I'm not sure
>> if it's worth maintaining just for swrast...
> draw does its own validation (according to more strict d3d10 rules,
> even), as long as everything comes in nice buffers with known sizes this
> isn't much of a problem (and it doesn't need to be done as a separate pass).
>
> I am pretty sure though any app used to be able to crash the X server
> (when using indirect rendering) when not doing bounds checking pretty
> easily, but maybe that vanished at some point somehow (luckily, I never
> had to look at indirect rendering for years...).
>
> I think at some point it was also useful for debugging (so you could
> more easily see where that weird segfault was coming from) though again
> of course it did nothing for non-vbo arrays. classic swrast could
> probably reimplement this on its own if nothing else uses it anymore. Of
> course real hw nowadays you just give the buffer sizes and the hw will
> make sure nothing is fetched outside bounds on its own.
>
> So, for me dropping this looks ok, but I'm not really working much in
> that area nowadays.

As Eric pointed out, it doesn't look like there's any glDrawElements or 
VBO code getting used in the X server (the glDrawElements call gets 
"unwound" in the client-side GLX code, AFAICT).

As it is, the CheckArrayBounds code checks for invalid array indexes at 
draw-validation time.  If a bad index is found, the whole draw is 
discarded.  Alternately, a driver/device can discard individual 
primitives while drawing if an array index is out of bounds (if the 
vertex fetch fails, skip the prim).  The TnL (src/tnl/) module has never 
had any support for per-vertex bounds checking but I believe all the 
gallium drivers (should) handle it.  I think this behavior is what's 
expected of OpenGL/Direct3D nowadays so the CheckArrayBounds approach 
isn't very good.

In piglit we have tests/spec/arb_robustness/draw-vbo-bounds.c.  It's not 
included in all.py though so it's probably seldom run.  I tested it with 
the software drivers (non-GLX mode):

swrast: failed assertion, implementation error: bad datatype 0x1010101 
in interpolate_int_colors, plus valgrind errors in the VBO code.  But if 
I enable ctx->Const.CheckArrayBounds, I get warnings but no crashes.

llvmpipe: failed assertion, lp_setup_tri.c:495:do_triangle_ccw

softpipe: pass

I just tried this piglit test in Ubuntu 14 with LIBGL_ALWAYS_INDIRECT 
and I get a glx protocol error (no X crash).  But that's using the 
llvmpipe driver.

Anyway, I think this code can be removed.  Ideally, some index checking 
would be added to the tnl code to try to avoid crashes, but I doubt 
anyone wants to bother with it.

Reviewed-by: Brian Paul <brianp at vmware.com>