[Mesa-dev] Bogus bounds checking in api_validate.c?

[Timothy Arceri]

On Mon, 2014-11-10 at 06:09 -0800, 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.

I'm not sure what applications you looked at in cachegrind but OpenArena
and Nexuiz both spend a lot of time in here. On my Ivybridge:

OpenArena 5.4% out of a total 27.94% in i965_dri.so
Nexuiz 3.28% out of a total of 29.4% in i965_dri.so

For those not up to speed are you able to give a one line explanation of
why some hardware can get away without this check?

> 
> > 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.
> 
> > --Ken
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev