On 11/10/2014 06:09 AM, Ian Romanick wrote:
> On 11/06/2014 11:40 PM, Kenneth Graunke wrote:
>> On Thursday, November 06, 2014 08:09:18 PM Ian Romanick wrote:
>>> While working on some other things, I came across some bounds checking
>>> code in _mesa_validate_DrawElements (and related functions) in
>>> api_validate.c.
>>>
>>> /* use indices in the buffer object */
>>> /* make sure count doesn't go outside buffer bounds */
>>> if (index_bytes(type, count) > ctx->Array.VAO->IndexBufferObj->Size) {
>>> _mesa_warning(ctx, "glDrawElements index out of buffer bounds");
>>> return GL_FALSE;
>>> }
>>>
>>> index_bytes calculates how many bytes of data "count" indices will
>>> occupy based on the type. The problem is that this doesn't consider
>>> the base pointer. As far as I can tell, if I had a 64 byte buffer
>>> object for my index data, and I did
>>>
>>> glDrawElements(GL_POINTS, 16, GL_UNSIGNED_INT, 60);
>>>
>>> _mesa_validate_DrawElements would say, "Ok!"
>>>
>>> Am I missing something, or is this just broken?
>>
>> It sure seems broken to me - but, thankfully, in a conservative fashion.
>> (It
>> will say some invalid things are OK, but won't say legal things are invalid.)
>>
>> Software drivers may be relying on this working to avoid a crash.
>>
>> I checked the Ivybridge documentation, and found:
>>
>> "Software is responsible for ensuring that accesses outside the IB do not
>> occur. This is possible as software can compute the range of IB values
>> referenced by a 3DPRIMITIVE command (knowing the StartVertexLocation,
>> InstanceCount, and VerticesPerInstance values) and can then compare this
>> range to the IB extent."
>>
>> which makes it sound like an accurate computation is necessary. But, right
>> below that, it says:
>>
>> "this field contains the address of the last valid byte in the index buffer.
>> Any index buffer reads past this address returns an index value of 0 (as if
>> the index buffer was zero-extended)."
>>
>> So the earlier statement is false; i965 will draw the in-bounds elements
>> correctly, and then repeat element 0 over and over for any out-of-bounds
>> data,
>> resulting in one strange primitive and a lot of degenerate ones.
>>
>> It's proabbly worth fixing, but I doubt it's critical either.
>
> Hmm... I came across this while looking at cachegrind traces of GL
> applications. Time spent in _mesa_validate_Draw* was non-trivial.
> Since at least some hardware doesn't need this check, I think I want to
> move it down into drivers that actually need the check... which is kind
> of a bummer since I came up with a clever optimization for index_bytes.
>
>> A more interesting thing to fix, I think, would be enforcing alignment
>> restrictions (i.e. your offset has to be a multiple of the IB element size).
>
> That would probably be useful in debug builds, but I'm pretty sure the
> GL spec says the behavior is undefined specifically to avoid the check
> in the hot path.
You may be right, actually. Section 6.2 (Creating and Modifying Buffer
Object Data Stores) of the 4.5 spec says:
"Clients must align data elements consistent with the requirements
of the client platform, with an additional base-level requirement
that an offset within a buffer to a datum comprising N basic
machine units be a multiple of N."
Of course, it doesn't say what to do. I don't see anything in this
section or in section 2.3.1 (Errors) that suggests an error to generate.
Just ignore the command? Halt and catch fire?
We have some lovely re-base code in brw_upload_indices that maybe we can
delete.
>> --Ken
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
