This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH]: Use RAX_LP/RDX_LP on SAVE_PTR in sysdeps/x86_64/strtok.S
On Wed, Jun 13, 2012 at 11:21 AM, Roland McGrath <roland@hack.frob.com> wrote:
>> > I need to use -O1 on tst-strtok_r.c. ?Otherwise, it won't fail with
>> > broken strtok.S.
>
> Can you explain why?
>
>> +CFLAGS-tst-strtok_r.c = -O1 -fno-inline -fno-builtin
>
> This needs a comment about why these flags matter (other than -fno-builtin).
Done.
> I suspect this would be done better using -D__NO_STRING_INLINES if that's
> the true issue.
This test is very sensitive to stack layout. -O1 with GCC 4.6 will
create a stack layout such that the test fails on x32 with a broken
strtok_r implementation.
>> --- /dev/null
>> +++ b/string/tst-strtok_r.c
>> @@ -0,0 +1,11 @@
>> +#include <stdlib.h>
>> +#include <string.h>
>
> Give it a copyright header.
Done.
>> +int main()
>
> Use test-skeleton.c.
Done.
>> +{
>> + ?char line[] = "udf 75868 1 - Live 0xffffffffa0bfb000\n";
>
> Use const.
Since strtok_r will update line, we can't use const.
>> + ?char *saveptr, *tok = strtok_r(line, " \t", &saveptr);
>
> Space before paren.
Done.
>> + ?if (strcmp (tok, "udf") != 0)
>> + ? ?abort ();
>> + ?return 0;
>
> No need for abort. ?A failing test can just exit nonzero.
Done.
>> @@ -79,13 +79,12 @@ ENTRY (BP_SYM (FUNCTION))
>>
>> ?#ifdef USE_AS_STRTOK_R
>> ? ? ? /* The value is stored in the third argument. ?*/
>> - ? ? movq %rdx, %rax
>> ? ? ? movq %rdx, %r9 ? ? ? ? ?/* Save value - see def. of SAVE_PTR. ?*/
>
> Might as well make SAVE_PTR %R9_LP and use 'mov %RDX_LP, SAVE_PTR' here.
>
We can't since SAVE_PTR is a memory location, not a register.
Here is the updated patch. OK to install?
Thanks.
--
H.J.
---
[BZ #14229]
* string/Makefile (tests): Add tst-strtok_r.
* string/tst-strtok_r.c: New file.
* sysdeps/x86_64/strtok.S: Use RAX_LP/RDX_LP on SAVE_PTR.
diff --git a/string/Makefile b/string/Makefile
index 80923a2..9587e7e 100644
--- a/string/Makefile
+++ b/string/Makefile
@@ -56,7 +56,7 @@ tests := tester inl-tester noinl-tester testcopy test-ffs \
tst-strtok tst-strxfrm bug-strcoll1 tst-strfry \
bug-strtok1 $(addprefix test-,$(strop-tests)) \
bug-envz1 tst-strxfrm2 tst-endian tst-svc2 \
- bug-strstr1 bug-strchr1
+ bug-strstr1 bug-strchr1 tst-strtok_r
include ../Rules
@@ -74,6 +74,10 @@ CFLAGS-stratcliff.c = -fno-builtin
CFLAGS-test-ffs.c = -fno-builtin
CFLAGS-tst-inlcall.c = -fno-builtin
CFLAGS-bug-strstr1.c = -fno-builtin
+# This test is very sensitive to stack layout. -O1 with GCC 4.6 will
+# create a stack layout such that the test fails on x32 with a broken
+# strtok_r implementation.
+CFLAGS-tst-strtok_r.c = -O1 -fno-inline
ifeq ($(cross-compiling),no)
tests: $(objpfx)tst-svc.out
diff --git a/string/tst-strtok_r.c b/string/tst-strtok_r.c
new file mode 100644
index 0000000..55af351
--- /dev/null
+++ b/string/tst-strtok_r.c
@@ -0,0 +1,30 @@
+/* Test strtok_r
+ Copyright (C) 2012 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <string.h>
+
+static int
+do_test (void)
+{
+ char line[] = "udf 75868 1 - Live 0xffffffffa0bfb000\n";
+ char *saveptr, *tok = strtok_r (line, " \t", &saveptr);
+ return strcmp (tok, "udf");
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/sysdeps/x86_64/strtok.S b/sysdeps/x86_64/strtok.S
index 150f4d6..3ed6521 100644
--- a/sysdeps/x86_64/strtok.S
+++ b/sysdeps/x86_64/strtok.S
@@ -1,6 +1,6 @@
/* strtok (str, delim) -- Return next DELIM separated token from STR.
For AMD x86-64.
- Copyright (C) 1998,2000-2003,2005,2006 Free Software Foundation, Inc.
+ Copyright (C) 1998-2012 Free Software Foundation, Inc.
This file is part of the GNU C Library.
Based on i686 version contributed by Ulrich Drepper
<drepper@cygnus.com>, 1998.
@@ -79,13 +79,12 @@ ENTRY (BP_SYM (FUNCTION))
#ifdef USE_AS_STRTOK_R
/* The value is stored in the third argument. */
- movq %rdx, %rax
- movq %rdx, %r9 /* Save value - see def. of SAVE_PTR. */
- movq (%rax), %rax
+ mov %RDX_LP, %R9_LP /* Save value - see def. of SAVE_PTR. */
+ mov (%rdx), %RAX_LP
#else
/* The value is in the local variable defined above. But
we have to take care for PIC code. */
- movq SAVE_PTR, %rax
+ mov SAVE_PTR, %RAX_LP
#endif
movq %r8, %rdx /* Get start of string. */
@@ -194,7 +193,7 @@ L(8): cmpq %rax, %rdx
cmovne %rcx, %rdx
/* Store the pointer to the next character. */
- movq %rdx, SAVE_PTR
+ mov %RDX_LP, SAVE_PTR
L(epilogue):
/* Remove the stopset table. */
@@ -205,7 +204,7 @@ L(epilogue):
L(returnNULL):
xorl %eax, %eax
/* Store the pointer to the next character. */
- movq %rdx, SAVE_PTR
+ mov %RDX_LP, SAVE_PTR
jmp L(epilogue)
END (BP_SYM (FUNCTION))