[Mesa-dev] [RFC PATCH] mesa: Delete VAO _MaxElement code and index buffer bounds checking.
Brian Paul
brianp at vmware.com
Tue Sep 16 08:48:28 PDT 2014
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>
More information about the mesa-dev
mailing list