Re: [Mesa-dev] [RFC PATCH] mesa: Delete VAO _MaxElement code and index buffer bounds checking.
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 kenn...@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
Re: [Mesa-dev] [RFC PATCH] mesa: Delete VAO _MaxElement code and index buffer bounds checking.
Am 16.09.2014 17:48, schrieb 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 kenn...@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
[Mesa-dev] [RFC PATCH] mesa: Delete VAO _MaxElement code and index buffer bounds checking.
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 kenn...@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 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. I'm open to suggestions. Thanks! diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c b/src/mesa/drivers/dri/i965/brw_draw_upload.c index 2162624..5a12439 100644 --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c @@ -502,10 +502,7 @@ brw_prepare_vertices(struct brw_context *brw) /* This is a common place to reach if the user mistakenly supplies * a pointer in place of a VBO offset. If we just let it go through, * we may end up dereferencing a pointer beyond the bounds of the - * GTT. We would hope that the VBO's max_index would save us, but - * Mesa appears to hand us min/max values not clipped to the - * array object's _MaxElement, and _MaxElement frequently appears - * to be wrong anyway. + * GTT. * * The VBO spec allows application termination in this case, and it's * probably a service to the poor programmer to do so rather than diff --git a/src/mesa/drivers/dri/swrast/swrast.c b/src/mesa/drivers/dri/swrast/swrast.c index e28991b..e8a2c12 100644 --- a/src/mesa/drivers/dri/swrast/swrast.c +++ b/src/mesa/drivers/dri/swrast/swrast.c @@ -772,9 +772,6 @@ dri_create_context(gl_api api, driContextSetFlags(mesaCtx, flags); -/* do bounds checking to prevent segfaults and server crashes! */ -mesaCtx-Const.CheckArrayBounds = GL_TRUE; - /* create module contexts */ _swrast_CreateContext( mesaCtx ); _vbo_CreateContext( mesaCtx ); diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c index 8f0b199..51a3d1f 100644 --- a/src/mesa/main/api_validate.c +++
Re: [Mesa-dev] [RFC PATCH] mesa: Delete VAO _MaxElement code and index buffer bounds checking.
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 kenn...@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. Roland 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. I'm open to suggestions. Thanks! diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c b/src/mesa/drivers/dri/i965/brw_draw_upload.c index 2162624..5a12439 100644 --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c @@ -502,10 +502,7 @@ brw_prepare_vertices(struct brw_context *brw) /* This is a common place to reach if the user mistakenly supplies
