[Mesa-dev] [PATCH] Mesa: Fix performance query id check

[Robert Bragg]

Since seeing that I had indeed sent out a v2 patch and it was reviewed
by Plamena too (thanks) I've just gone a head and pushed that now
(though with an updated commit message instead of copy pasting the
original message).

Thanks,
- Robert


On Mon, Feb 27, 2017 at 3:43 PM, Robert Bragg <robert at sixbynine.org> wrote:
> On Mon, Feb 27, 2017 at 2:47 PM, Juha-Pekka Heikkila
> <juhapekka.heikkila at gmail.com> wrote:
>> In queryid_valid() fix handling of zero index.
>>
>> CC: Robert Bragg <robert at sixbynine.org>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>>  src/mesa/main/performance_query.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/performance_query.c b/src/mesa/main/performance_query.c
>> index aa10351..142d834 100644
>> --- a/src/mesa/main/performance_query.c
>> +++ b/src/mesa/main/performance_query.c
>> @@ -91,7 +91,7 @@ static inline bool
>>  queryid_valid(const struct gl_context *ctx, unsigned numQueries, GLuint queryid)
>>  {
>>     GLuint index = queryid_to_index(queryid);
>> -   return index >= 0 && index < numQueries;
>> +   return queryid != 0 && index < numQueries;
>>  }
>>
>>  static inline GLuint
>> --
>> 2.7.4
>>
>
> Oh, I thought I'd sent out an updated patch, but apparently I made it
> but got distracted and didn't actually send it :-/
>
> Main differences with mine was that I added a quote from the spec
> about ID zero being reserved:
>
> -   GLuint index = queryid_to_index(queryid);
> -   return index >= 0 && index < numQueries;
> +   /* The GL_INTEL_performance_query spec says:
> +    *
> +    *  "Performance counter ids values start with 1. Performance counter id 0
> +    *  is reserved as an invalid counter."
> +    */
> +   return queryid != 0 && queryid_to_index(queryid) < numQueries;
>
> And dropped the local variable which you say you'd prefer to keep. I
> suppose on the other hand it seems good to me to not pass an index of
> zero to queryid_to_index() before validation in case
> queryid_to_index() might one day gain its own paranoid assertion that
> the queryid is >= 0.
>
> I don't have a strong opinion here though, so your patch looks fine to
> land to me:
> Reviewed-by: Robert Bragg <robert at sixbynine.org>
>
> Br,
> - Robert