[Mesa-dev] [PATCH] mesa: fixes for DrawRangeElements[BaseVertex] index validation
Roland Scheidegger
sroland at vmware.com
Wed Feb 8 09:41:40 PST 2012
Am 08.02.2012 14:08, schrieb Kenneth Graunke:
> On 02/05/2012 05:08 PM, Roland Scheidegger wrote:
>> In vbo_exec_DrawRangeElementsBaseVertex, take into account the
>> basevertex.
>> As far as I can tell it is completely ok (though maybe stupid) to have
>> start/end of 100/199, with _MaxElement being 100, if the basevertex
>> is -100 (since the start/end are prior to adding basevertex). The
>> opposite
>> is also true (0/99 is not ok if _MaxElement is 100 and and basevertex
>> is 100).
>> Previously we would have issued a warning in such cases and worse
>> probably
>> "fixed" end to a bogus value.
>> Untested, found by code inspection when looking at bug 40361 (which
>> seems unrelated after all).
>>
>> v2: As per mailing list discussion and the suggestion by Kenneth
>> Graunke just drop
>> the questionable effort to fix up the range and revert to have no
>> range instead.
>> Rearrange some (disabled) debug bits a bit for slightly less complex
>> code.
>> ---
>> src/mesa/vbo/vbo_exec_array.c | 63
>> +++++++++++++++++++---------------------
>> 1 files changed, 30 insertions(+), 33 deletions(-)
>
> I have to admit, I'm not a huge fan of this patch...it really does a lot
> of things at once:
>
> 1. Fix bounds checks to take BaseVertex into account
> 2. Work around broken apps that give us start >= _MaxElement
> 3. Remove 'end' clamping in favor of ignoring the range
> 4. Make the warnings print BaseVertex as well
> 5. Debugging code motion
>
> I'd much rather see small patches that do one thing at a time, so we can
> bisect across them if we break things.
>
> Also: I agree with Jose, the warning for end >= _MaxElement is just not
> useful. I'd rather scrap it instead of trying to fix it up. But we
> -do- want a warning for start >= _MaxElement, since that's clearly an
> application bug (as Jose mentioned).
>
> I'll send out an alternate proposal (4-patch series) here in a moment.
> Let me know what you think.
Fair enough I mostly did it in one patch because all these issues really
seemed closely related (except the warning messages which were just
wrong in case of Basevertex and likely adding only confusion in this
case). Should have done the harmless changes separately I agree.
Roland
>
>> diff --git a/src/mesa/vbo/vbo_exec_array.c
>> b/src/mesa/vbo/vbo_exec_array.c
>> index d6b4d61..1f62e3a 100644
>> --- a/src/mesa/vbo/vbo_exec_array.c
>> +++ b/src/mesa/vbo/vbo_exec_array.c
>> @@ -885,17 +885,33 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>> end = MIN2(end, 0xffff);
>> }
>>
>> - if (end>= ctx->Array.ArrayObj->_MaxElement) {
>> - /* the max element is out of bounds of one or more enabled
>> arrays */
>> + if (0) {
>> + printf("glDrawRangeElements{,BaseVertex}"
>> + "(start %u, end %u, type 0x%x, count %d) ElemBuf %u, "
>> + "base %d\n",
>> + start, end, type, count,
>> + ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
>> + basevertex);
>> + }
>> +
>> +#if 0
>> + check_draw_elements_data(ctx, count, type, indices);
>> +#else
>> + (void) check_draw_elements_data;
>> +#endif
>> +
>> + if ((int)(start + basevertex)< 0 ||
>> + end + basevertex>= ctx->Array.ArrayObj->_MaxElement) {
>> + /* the max (or min) element is out of bounds of one or more
>> enabled arrays */
>> warnCount++;
>>
>> if (warnCount< 10) {
>> - _mesa_warning(ctx, "glDraw[Range]Elements(start %u, end %u,
>> count %d, "
>> - "type 0x%x, indices=%p)\n"
>> + _mesa_warning(ctx, "glDrawRangeElements[BaseVertex](start
>> %u, end %u, count %d, "
>> + "type 0x%x, indices=%p, base %d)\n"
>> "\tend is out of bounds (max=%u) "
>> "Element Buffer %u (size %d)\n"
>> "\tThis should probably be fixed in the
>> application.",
>> - start, end, count, type, indices,
>> + start, end, count, type, indices, basevertex,
>> ctx->Array.ArrayObj->_MaxElement - 1,
>>
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
>> (int)
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Size);
>> @@ -913,14 +929,14 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>> if (0) {
>> GLuint max = _mesa_max_buffer_index(ctx, count, type, indices,
>>
>> ctx->Array.ArrayObj->ElementArrayBufferObj);
>> - if (max>= ctx->Array.ArrayObj->_MaxElement) {
>> + if (max + basevertex>= ctx->Array.ArrayObj->_MaxElement) {
>> if (warnCount< 10) {
>> - _mesa_warning(ctx, "glDraw[Range]Elements(start %u,
>> end %u, "
>> - "count %d, type 0x%x, indices=%p)\n"
>> + _mesa_warning(ctx,
>> "glDrawRangeElements[BaseVertex](start %u, end %u, "
>> + "count %d, type 0x%x, indices=%p, base
>> %d)\n"
>> "\tindex=%u is out of bounds (max=%u) "
>> "Element Buffer %u (size %d)\n"
>> "\tSkipping the glDrawRangeElements()
>> call",
>> - start, end, count, type, indices, max,
>> + start, end, count, type, indices,
>> basevertex, max,
>> ctx->Array.ArrayObj->_MaxElement - 1,
>>
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
>> (int)
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Size);
>> @@ -928,34 +944,15 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>> }
>> /* XXX we could also find the min index and compare to 'start'
>> * to see if start is correct. But it's more likely to get
>> the
>> - * upper bound wrong.
>> + * upper bound wrong, at least for the non-BaseVertex
>> variant...
>> */
>> }
>> -
>> - /* Set 'end' to the max possible legal value */
>> - assert(ctx->Array.ArrayObj->_MaxElement>= 1);
>> - end = ctx->Array.ArrayObj->_MaxElement - 1;
>> -
>> - if (end< start) {
>> - return;
>> - }
>> - }
>> -
>> - if (0) {
>> - printf("glDraw[Range]Elements{,BaseVertex}"
>> - "(start %u, end %u, type 0x%x, count %d) ElemBuf %u, "
>> - "base %d\n",
>> - start, end, type, count,
>> - ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
>> - basevertex);
>> + /* range is bogus, instead of trying to fix it up just revert
>> to no range */
>> + vbo_validated_drawrangeelements(ctx, mode, GL_FALSE, ~0, ~0,
>> + count, type, indices,
>> basevertex, 1);
>> + return;
>> }
>>
>> -#if 0
>> - check_draw_elements_data(ctx, count, type, indices);
>> -#else
>> - (void) check_draw_elements_data;
>> -#endif
>> -
>> vbo_validated_drawrangeelements(ctx, mode, GL_TRUE, start, end,
>> count, type, indices, basevertex, 1);
>> }
>
More information about the mesa-dev
mailing list