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

[Eric Anholt]

Kenneth Graunke <kenneth at whitecape.org> writes:

> 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.
>
> 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...

I thought this was important for the X Server's indirect, but since he X
Server doesn't use DrawElements or VBOs and just gives pointers to its
own malloced memory with known vertex counts, I don't see any need in
swrast.

> I know the vc4 driver also needs to perform bounds checking on memory
> access since it uses physical memory directly, but the kernel driver has
> to perform that, for safety.  Plus, I think it wants something more
> reliable and probably lower level, so it doesn't seem like dropping this
> code will hurt there, either.

If I had the _MaxElement in gallium and it was reliable (got maintained
even when rewriting of my index buffer happens for various purposes), it
would actually be useful since I have to declare a maximum valid index
value in my draws (which then limits how far anything will be read in
the vertex buffers, and the kernel checks that that matches up with the
buffer lengths).

However, I'm pretty sure it's easier to just calculate after the fact
inside of my driver (+13, -5).  Basically, I won't miss it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140915/6378c249/attachment.sig>