[Mesa-dev] [PATCH 1/5] util/set: don't compare against deleted entries

[Connor Abbott]

On Mon, Nov 16, 2015 at 6:56 PM, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> On Sat, 2015-11-14 at 21:59 -0500, Connor Abbott wrote:
>> Not sure how this wasn't already caught by valgrind, but it fixes an
>> issue with the vectorizer.
>
> Can you give a more detailed description of the problem that is fixed? I'm
> assuming its something to do with the key_equals_function having issues
> comparing to the deleted_key value?

Yes, that's correct. The pass I was adding tried to dereference the
data contained in deleted_key and segfaulted. I'm surprised that other
things aren't reading past deleted_key and getting uninitialized
memory, or at least getting valgrind errors, but this pass seems to be
the only one that does something sophisticated enough with the key to
trigger this. I'll add that bit, and some more explanation of the
problem to the message.

>
>>
>> Signed-off-by: Connor Abbott <cwabbott0 at gmail.com>
>> ---
>>  src/util/set.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/set.c b/src/util/set.c
>> index f01f869..331ff58 100644
>> --- a/src/util/set.c
>> +++ b/src/util/set.c
>> @@ -282,7 +282,8 @@ set_add(struct set *ht, uint32_t hash, const void *key)
>>         * If freeing of old keys is required to avoid memory leaks,
>>         * perform a search before inserting.
>>         */
>> -      if (entry->hash == hash &&
>> +      if (entry_is_present(entry) &&
>
> You can use !entry_is_deleted(entry) here as free entries will have already
> cased the loop the break.

Ok, fine. The choice is pretty arbitrary, but I guess
entry_is_deleted() will generate slightly better code.

>
> With these two comments addressed this and patch 2 are:
>
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
>
>> +          entry->hash == hash &&
>>            ht->key_equals_function(key, entry->key)) {
>>           entry->key = key;
>>           return entry;