[Mesa-dev] [PATCH] draw: fix key comparison with uninitialized value

[Roland Scheidegger]

Am 12.01.2016 um 10:49 schrieb Jose Fonseca:
> On 12/01/16 02:11, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> Discovered by accident, valgrind was complaining (could have possibly
>> caused
>> us to create redundant geometry shader variants).
> 
> Great fine.
> 
>> ---
>>   src/gallium/auxiliary/draw/draw_llvm.c | 3 +++
>>   src/gallium/auxiliary/draw/draw_llvm.h | 5 +++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c
>> b/src/gallium/auxiliary/draw/draw_llvm.c
>> index f25dafe..faa6e40 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.c
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
>> @@ -2315,6 +2315,9 @@ draw_gs_llvm_make_variant_key(struct draw_llvm
>> *llvm, char *store)
>>
>>      key = (struct draw_gs_llvm_variant_key *)store;
>>
>> +   assert(offsetof(struct draw_gs_llvm_variant_key, samplers[0]) == 4);
>> +   key->pad1 = 0;
>> +
>>      key->num_outputs = draw_total_gs_outputs(llvm->draw);
>>
>>      /* All variants of this shader will have the same value for
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.h
>> b/src/gallium/auxiliary/draw/draw_llvm.h
>> index f617a29..67f2170 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.h
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.h
>> @@ -332,6 +332,11 @@ struct draw_gs_llvm_variant_key
>>      unsigned nr_samplers:8;
>>      unsigned nr_sampler_views:8;
>>      unsigned num_outputs:8;
>> +   /*
>> +    * it is important there are no holes in this struct
>> +    * (and all padding gets zeroed). 32 bit pad should be enough...
>> +    */
>> +   unsigned pad1:8;
>>
>>      struct draw_sampler_static_state samplers[1];
>>   };
>>
> 
> The change looks OK, but I wonder if it wouldn't be more future proof to
> just memset the whole structure to zero?
> 
> And then ensure we always use memcpy to copy, memcmp to compare.  I
> believe this is the pattern we've been following elsewhere.
Certainly in some places (at least those which include pipe structs
directly in the keys, there's no good option in any case otherwise there
as those don't have padding).
I got this pattern here from the vs key handling in the same file. Could
certainly switch both (we do already memset the variable parts of the
key, so those including the samplers and for vs also the input
elements), this is just a aminimal change so I only had to change one.

Roland


> 
> Either way,
> 
> Reviewed-by: Jose Fonseca <jfonseca at vmware.com>
> 
> Jose