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

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

[Paolo Bonzini]

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--) {
-- 
2.5.0

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

[Richard Henderson]

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?

I think this would be a lot simpler if we simply followed the pseudocode in 
table 4-3, doing overrideIfDataInvalid after comparison.


r~

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

[Paolo Bonzini]

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