[Mesa-dev] [PATCHES] implement GL_ARB_robustness

Tue Apr 19 19:31:41 PDT 2011

On Tue, Apr 19, 2011 at 4:43 PM, Ian Romanick <idr at freedesktop.org> wrote:
> -----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?
>
Sorry about that; I'll send the updated patch series inline. There
tend to be word wrapping issues with 80+ character lines though (like
in extensions.c), so I'll also attach them as files here.

> 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.
Split!
>
> 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.
Kay, fixed.
>
> 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?
Whoops; forgot to re-add something there.

>
> 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);
> +}
Split and fixed!

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

I added an XML file now, too; regenerating didn't work for me though,
because I'm missing some
XORG thing? (Also I had to run `apt-get install indent` first, but
then I hit the missing XORG thing error.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mesa-use-is_bufferobj-helper-function.patch
Type: text/x-patch
Size: 2389 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-mesa-add-bounds-checking-support-for-client-memory-b.patch
Type: text/x-patch
Size: 12531 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-mesa-add-more-bounds-checking-support-for-client-mem.patch
Type: text/x-patch
Size: 8248 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-mesa-standardize-some-bounds-checking-error-messages.patch
Type: text/x-patch
Size: 4597 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-mesa-standardize-more-bounds-checking-error-messages.patch
Type: text/x-patch
Size: 7580 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-mesa-add-context-fields-for-GL_ARB_robustness.patch
Type: text/x-patch
Size: 3272 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-mesa-implement-GL_ARB_robustness-functions.patch
Type: text/x-patch
Size: 31977 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-glapi-add-ARB_robustness-xml.patch
Type: text/x-patch
Size: 8471 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110419/ad8ab42b/attachment-0015.bin>