[Mesa-dev] [PATCHES] implement GL_ARB_robustness

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/19/2011 12:23 PM, nobled wrote:
> spec file:
> http://www.opengl.org/registry/specs/ARB/robustness.txt
> 
> The first four parts of this series add infrastructure to support
> bounds-checking client memory buffers by re-using the PBO
> bounds-checking code; the fifth patch adds the actual functions of the
> extension. However, the series is incomplete because I'm not sure how
> to turn the spec file into an xml file for
> src/mapi/glapi/gen/ARB_robustness.xml, and then generate the dispatch
> stuff from it with the python script(*which* python script?) -- the
> docs on this point seem to be a classic combination of completely
> outdated and "hey, can you vague that up for me?":
>> http://cgit.freedesktop.org/mesa/mesa/tree/docs/devinfo.html
>>   In the src/mesa/glapi/ directory, add the new extension functions and
>>   enums to the gl_API.xml file.
>>   Then, a bunch of source files must be regenerated by executing the
>>   corresponding Python scripts.
> 
> I think I covered all the other steps, though. At least enough to be
> faithful to the spec, as long as these two aren't exposed yet on the
> GLX side of things:
> http://www.opengl.org/registry/specs/ARB/glx_create_context.txt
> http://www.opengl.org/registry/specs/ARB/glx_create_context_robustness.txt
> 
> Still todo: adding piglit tests.

It's *REALLY* hard to review patches sent as attachments.  It is much
better and easier for reviewers to put comments alongside the code they
are commenting about.  In the future, could you please send patches
using git-send-email?

Patch 1/5 looks like it's doing to separate things.  It renames / alters
_mesa_validate_pbo_access and it starts using _mesa_is_bufferobj in some
places.  That should probably be split into two patches.  First change
over to using _mesa_is_bufferobj, then make the other changes.

I'm also not sure _mesa_validate_buffer_access is a particularly good
name.   This loses the information that the validation is for pixel
reads / writes.  This also applies to _mesa_map_validate_buffer_dest and
friends in later patches.

For the internal Mesa functions, I think I might also change the
parameter bufSize to clientMemorySize or similar.  Using bufSize in the
API functions (added in patch 5/5) matches the API definition and is fine.

The changes to readpix.c in patch 4/5 looks odd.  If the access is
out-of-bounds and there is PBO, nothing happens but no error is
generated.  Is that what the spec says to do?

It also looks like a lot of the checking for out-of-bounds followed by
checking for being mapped could be refactored.

Patch 5/5 also does two separate things.  It adds data structure
infrastructure for the extension and it adds a bunch of entry points.
Usually gl_context::ResetStatus and gl_constants::ResetStratgy would be
added in a separate patch from adding all the new functions.

In patch 5/5 why

+_mesa_GetMinmax(GLenum target, GLboolean reset, GLenum format, GLenum type,
+                GLvoid *values)
+{
+   const GLsizei bufSize = INT_MAX;
+   _mesa_GetnMinmaxARB(target, reset, format, type, bufSize, values);
+}

instead of

+_mesa_GetMinmax(GLenum target, GLboolean reset, GLenum format, GLenum type,
+                GLvoid *values)
+{
+   _mesa_GetnMinmaxARB(target, reset, format, type, INT_MAX, values);
+}

On a different note, I explicitly *approve* of the use of 'goto
overflow'. :)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2t8+oACgkQX1gOwKyEAw9wGQCgnMkLFee64zCdN/c1UutZ613x
4qIAn2ZKxnEuqClIzemoTuc98bewOY8X
=Qc3X
-----END PGP SIGNATURE-----