Re: [Qemu-devel] [PATCH] target-i386: fix pcmpxstrx equal-ordered (strstr) mode

[Paolo Bonzini <pbonzini@redhat.com>]

On 13/10/2015 00:16, Richard Henderson wrote:
> On 10/12/2015 08:50 PM, Paolo Bonzini wrote:
>> In this mode, referring an invalid element of the source forces the
>> result to false (table 4-7, last column) but referring an invalid
>> element of the destination forces the result to true, so the outer
>> loop should still be run even if some elements of the destination
>> will be invalid.  They will be culled in the inner loop, which
>> correctly bounds "i" to validd.
>>
>> This fix tst_strstr in glibc 2.17.
>>
>> Reported-by: Florian Weimer <fweimer@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   target-i386/ops_sse.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target-i386/ops_sse.h b/target-i386/ops_sse.h
>> index 7aa693a..268f3e1 100644
>> --- a/target-i386/ops_sse.h
>> +++ b/target-i386/ops_sse.h
>> @@ -2037,7 +2037,7 @@ static inline unsigned pcmpxstrx(CPUX86State
>> *env, Reg *d, Reg *s,
>>           }
>>           break;
>>       case 3:
>> -        for (j = valids - validd; j >= 0; j--) {
>> +        for (j = valids; j >= 0; j--) {
>>               res <<= 1;
>>               v = 1;
>>               for (i = MIN(upper - j, validd); i >= 0; i--) {
> 
> I don't see how the bounding is properly done.  In particular,
> 
>>                 v &= (pcmp_val(s, ctrl, i + j) == pcmp_val(d, ctrl, i));
> 
> We're bounding j by valids, but accessing i+j?

You're absolutely right, the second loop also needs s/upper/valids/.

Paolo