i386/strchr.S broken?

i386/strchr.S broken?

[Andreas Jaeger]

Looking at the i386/strchr.S file I noticed:

L(7):	testb %cl, %cl		/* is first byte C? */
	jz L(6)			/* yes => return pointer */
	cmpb %dl, %cl		/* is first byte NUL? */
	je L(2)			/* yes => return NULL */
	incl %eax		/* it's not in the first byte */

This looks broken to me, IMO it should be:
L(7):	testb %cl, %cl		/* is first byte NUL? */
	jz L(2)			/* yes => return NULL */
	cmpb %dl, %cl		/* is first byte C? */
	je L(6)			/* yes => return pointer */
	incl %eax		/* it's not in the first byte */

I'm appending an (untested) patch.  Ok to commit for both branches?

The i586 version looks fine,
Andreas

2002-08-10  Andreas Jaeger  <aj@suse.de>

	* sysdeps/i386/strchr.S: Correct implementation.

============================================================
Index: sysdeps/i386/strchr.S
--- sysdeps/i386/strchr.S	6 Jul 2001 04:55:52 -0000	1.11
+++ sysdeps/i386/strchr.S	10 Aug 2002 16:52:38 -0000
@@ -1,6 +1,6 @@
 /* strchr (str, ch) -- Return pointer to first occurrence of CH in STR.
    For Intel 80x86, x>=3.
-   Copyright (C) 1994, 1995, 1996, 1997, 1999, 2000 Free Software Foundation, Inc.
+   Copyright (C) 1994, 1995, 1996, 1997, 1999, 2000, 2002 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@gnu.ai.mit.edu>
    Some optimisations by Alan Modra <Alan@SPRI.Levels.UniSA.Edu.Au>
@@ -256,23 +256,23 @@ L(71):	addl $4, %eax
 	   But we have to take care of the case that a NUL char is
 	   found before this in the dword.  */
 
-L(7):	testb %cl, %cl		/* is first byte C? */
-	jz L(6)			/* yes => return pointer */
-	cmpb %dl, %cl		/* is first byte NUL? */
+L(7):	testb %cl, %cl		/* is first byte NUL? */
 	je L(2)			/* yes => return NULL */
+	cmpb %dl, %cl		/* is first byte C? */
+	jz L(6)			/* yes => return pointer */
 	incl %eax		/* it's not in the first byte */
 
-	testb %ch, %ch		/* is second byte C? */
-	jz L(6)			/* yes => return pointer */
-	cmpb %dl, %ch		/* is second byte NUL? */
+	testb %ch, %ch		/* is second byte NUL? */
 	je L(2)			/* yes => return NULL? */
+	cmpb %dl, %ch		/* is second byte C? */
+	jz L(6)			/* yes => return pointer */
 	incl %eax		/* it's not in the second byte */
 
 	shrl $16, %ecx		/* make upper byte accessible */
-	testb %cl, %cl		/* is third byte C? */
-	jz L(6)			/* yes => return pointer */
-	cmpb %dl, %cl		/* is third byte NUL? */
+	testb %cl, %cl		/* is third byte NUL? */
 	je L(2)			/* yes => return NULL */
+	cmpb %dl, %cl		/* is third byte C? */
+	jz L(6)			/* yes => return pointer */
 
 	/* It must be in the fourth byte and it cannot be NUL.  */
 	incl %eax

-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.inka.de
    http://www.suse.de/~aj

Re: i386/strchr.S broken?

[Andreas Jaeger]

Ignore the patch - I understand the algorithm now.  But now I'd like
to add a comment to it.

Ok to commit to mainline?
Andreas

2002-08-10  Andreas Jaeger  <aj@suse.de>

	* sysdeps/i386/strchr.S: Add comment.
============================================================
Index: sysdeps/i386/strchr.S
--- sysdeps/i386/strchr.S	6 Jul 2001 04:55:52 -0000	1.11
+++ sysdeps/i386/strchr.S	10 Aug 2002 17:01:19 -0000
@@ -1,6 +1,6 @@
 /* strchr (str, ch) -- Return pointer to first occurrence of CH in STR.
    For Intel 80x86, x>=3.
-   Copyright (C) 1994, 1995, 1996, 1997, 1999, 2000 Free Software Foundation, Inc.
+   Copyright (C) 1994, 1995, 1996, 1997, 1999, 2000, 2002 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@gnu.ai.mit.edu>
    Some optimisations by Alan Modra <Alan@SPRI.Levels.UniSA.Edu.Au>
@@ -254,7 +254,9 @@ L(71):	addl $4, %eax
 
 	/* We now scan for the byte in which the character was matched.
 	   But we have to take care of the case that a NUL char is
-	   found before this in the dword.  */
+	   found before this in the dword.  Note that we XORed the %ecx
+	   with the byte we're looking for, therefore the tests below look
+	   reversed.  */
 
 L(7):	testb %cl, %cl		/* is first byte C? */
 	jz L(6)			/* yes => return pointer */

-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.inka.de
    http://www.suse.de/~aj

Re: i386/strchr.S broken?

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andreas Jaeger wrote:
> Ignore the patch - I understand the algorithm now.  But now I'd like
> to add a comment to it.
> 
> Ok to commit to mainline?

Yes, go on.

- -- 
- ---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9VVTe2ijCOnn/RHQRAlIKAJ9j7UGvRrTpCdjXZfVpRAmQYunJ/QCfdKt2
MHovDK9bwlREs2EUj+6r2ek=
=escG
-----END PGP SIGNATURE-----