[07/11] Fix strcmp bug for little endian target

Message ID 20250123134308.1785777-9-aleksandar.rakic@htecgroup.com (mailing list archive)
State New
Headers
Series Improve Mips target |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent

Commit Message

Aleksandar Rakic Jan. 23, 2025, 1:43 p.m. UTC
  Strcmp gives incorrect result for little endian targets under
the following conditions:
1. Length of 1st string is 1 less than a multiple of 4 (i.e len%4=3)
2. First string is a prefix of the second string
3. The first differing character in the second string is extended
   ASCII (that is > 127)

Cherry-picked 7c709e878f836069bbdbf42979937794623cfa68
from https://github.com/MIPS/glibc

Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>
---
 sysdeps/mips/strcmp.S | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
  

Comments

Joseph Myers Jan. 23, 2025, 4:20 p.m. UTC | #1
On Thu, 23 Jan 2025, Aleksandar Rakic wrote:

> Strcmp gives incorrect result for little endian targets under
> the following conditions:
> 1. Length of 1st string is 1 less than a multiple of 4 (i.e len%4=3)
> 2. First string is a prefix of the second string
> 3. The first differing character in the second string is extended
>    ASCII (that is > 127)

Is there a test in the glibc testsuite that fails before and passes after 
this patch?  If not, one should be added.
  
Adhemerval Zanella Netto Jan. 23, 2025, 6:23 p.m. UTC | #2
On 23/01/25 10:43, Aleksandar Rakic wrote:
> Strcmp gives incorrect result for little endian targets under
> the following conditions:
> 1. Length of 1st string is 1 less than a multiple of 4 (i.e len%4=3)
> 2. First string is a prefix of the second string
> 3. The first differing character in the second string is extended
>    ASCII (that is > 127)
> 
> Cherry-picked 7c709e878f836069bbdbf42979937794623cfa68
> from https://github.com/MIPS/glibc
> 
> Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
> Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>
> ---
>  sysdeps/mips/strcmp.S | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/mips/strcmp.S b/sysdeps/mips/strcmp.S
> index 4878cd3aac..8d1bab12ec 100644
> --- a/sysdeps/mips/strcmp.S
> +++ b/sysdeps/mips/strcmp.S
> @@ -225,10 +225,13 @@ L(worddiff):
>  	beq	a0, zero, L(wexit01)
>  	bne	a0, a1, L(wexit01)
>  
> -	/* The other bytes are identical, so just subract the 2 words
> -	  and return the difference.  */
> +# if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +	srl a0, a2, 24
> +	srl a1, a3, 24
> +# else /* __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ */
>  	move a0, a2
>  	move a1, a3
> +# endif /* __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ */
>  
>  L(wexit01):
>  	subu	v0, a0, a1

Can't you use the generic implementation instead?  If I understand correctly,
mips optimizes only the aligned case, while generic code also do word size
read for unaligned case (with the MERGE and shift tricks).  The only trick
I see that mips implementation does is loop unrolling, which I think you
can do by adding some compiler flags on mips Makefile.
  

Patch

diff --git a/sysdeps/mips/strcmp.S b/sysdeps/mips/strcmp.S
index 4878cd3aac..8d1bab12ec 100644
--- a/sysdeps/mips/strcmp.S
+++ b/sysdeps/mips/strcmp.S
@@ -225,10 +225,13 @@  L(worddiff):
 	beq	a0, zero, L(wexit01)
 	bne	a0, a1, L(wexit01)
 
-	/* The other bytes are identical, so just subract the 2 words
-	  and return the difference.  */
+# if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	srl a0, a2, 24
+	srl a1, a3, 24
+# else /* __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ */
 	move a0, a2
 	move a1, a3
+# endif /* __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ */
 
 L(wexit01):
 	subu	v0, a0, a1