This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
I test on x86_64 (AMD Opteron 285).
#include <string.h>
char *A = "0000";
void test_memcmp(void) {
memcmp(A, "TEST", 4);
}
void test_strn(void) {
strncmp(A, "TEST", 4);
}
# gcc -c -O2 test.c
# objdump -d test.o
...
0000000000000020 <test_strncmp>:
20: f3 c3 repz retq
22: 66 66 66 66 66 2e 0f data32 data32 data32 data32 nopw
%cs:0x0(%rax,%rax,1)
29: 1f 84 00 00 00 00 00
0000000000000030 <test_memcmp>:
30: f3 c3 repz retq
Wow, minus one commad :)
--
Pavel.
2010/11/29 Pavel Vasilyev <[email protected]>:
>
> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
>
> I test on x86_64 (AMD Opteron 285).
>
> #include <string.h>
> char *A = "0000";
> void test_memcmp(void) {
> memcmp(A, "TEST", 4);
> }
> void test_strn(void) {
> strncmp(A, "TEST", 4);
> }
> # gcc -c -O2 test.c
> # objdump -d test.o
> ...
>
> 0000000000000020 <test_strncmp>:
> 20: f3 c3 repz retq
> 22: 66 66 66 66 66 2e 0f data32 data32 data32 data32 nopw
> %cs:0x0(%rax,%rax,1)
> 29: 1f 84 00 00 00 00 00
>
> 0000000000000030 <test_memcmp>:
> 30: f3 c3 repz retq
>
> Wow, minus one commad :)
>
Wow, good, I test that patch ... can't see any performance
improvements. but... works fine. and mem* generally better than str*
one, right?
Hi,
2010/11/29 Pavel Vasilyev <[email protected]>:
>
> ? ?This patch replace all strncmp(a, b, c) by ?memcmp(a, b, c).
>
> I test on x86_64 (AMD Opteron 285).
In fact, memcmp doesn't handle case of tail of string, so
it is not safe to replace strncmp with memcmp
thanks,
--
Lei Ming
On Mon, Nov 29, 2010 at 05:09:10AM +0300, Pavel Vasilyev wrote:
>
> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
>
>I test on x86_64 (AMD Opteron 285).
>
>#include <string.h>
>char *A = "0000";
>void test_memcmp(void) {
> memcmp(A, "TEST", 4);
>}
>void test_strn(void) {
> strncmp(A, "TEST", 4);
>}
This is plain wrong, memcmp() doesn't handle the case that 'a'
is shorter than 'b', it may access beyond '\0'.
On 29.11.2010 05:29, Ming Lei wrote:
> Hi,
>
> 2010/11/29 Pavel Vasilyev <[email protected]>:
>> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
>>
>> I test on x86_64 (AMD Opteron 285).
> In fact, memcmp doesn't handle case of tail of string, so
> it is not safe to replace strncmp with memcmp
>
#include <stdio.h>
#include <errno.h>
int main() {
char *STR = "XXXX\0";
char *XXX = "XXXX";
int a, b;
errno = 0; a = memcmp(STR, XXX, 5); a += errno;
errno = 0; b = strncmp(STR, XXX, 5); b += errno;
printf("5 chars: %d %d \n", a, b);
errno = 0; a = memcmp(STR, XXX, 4); a += errno;
errno = 0; b = strncmp(STR, XXX, 4); b += errno;
printf("4 chars: %d %d \n", a, b);
printf("SWAP STRINGS\n");
errno = 0; a = memcmp(XXX, STR, 5); a += errno;
errno = 0; b = strncmp(XXX, STR, 5); b += errno;
printf("5 chars: %d %d \n", a, b);
errno = 0; a = memcmp(XXX, STR, 4); a += errno;
errno = 0; b = strncmp(XXX, STR, 4); b += errno;
printf("4 chars: %d %d \n", a, b);
return 0;
}
----
# ./a.out
5 chars: 0 0
4 chars: 0 0
SWAP STRINGS
5 chars: 0 0
4 chars: 0 0
But I think the same thing ;)
--
Pavel.
On Mon, Nov 29, 2010 at 06:11:21AM +0300, Pavel Vasilyev wrote:
> On 29.11.2010 05:29, Ming Lei wrote:
> > Hi,
> >
> > 2010/11/29 Pavel Vasilyev <[email protected]>:
> >> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
> >>
> >> I test on x86_64 (AMD Opteron 285).
> > In fact, memcmp doesn't handle case of tail of string, so
> > it is not safe to replace strncmp with memcmp
> >
> #include <stdio.h>
> #include <errno.h>
>
> int main() {
>
> char *STR = "XXXX\0";
> char *XXX = "XXXX";
Try comparing:
"XXXX\0YYYY" and
"XXXX\0ZZZZ"
and observe the difference.
--
Dmitry
2010-11-28 (Sun) 20:13 -0800, Dmitry Torokhov wrote:
> On Mon, Nov 29, 2010 at 06:11:21AM +0300, Pavel Vasilyev wrote:
> > On 29.11.2010 05:29, Ming Lei wrote:
> > > Hi,
> > >
> > > 2010/11/29 Pavel Vasilyev <[email protected]>:
> > >> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
> > >>
> > >> I test on x86_64 (AMD Opteron 285).
> > > In fact, memcmp doesn't handle case of tail of string, so
> > > it is not safe to replace strncmp with memcmp
> > >
> > #include <stdio.h>
> > #include <errno.h>
> >
> > int main() {
> >
> > char *STR = "XXXX\0";
> > char *XXX = "XXXX";
>
> Try comparing:
>
> "XXXX\0YYYY" and
> "XXXX\0ZZZZ"
>
> and observe the difference.
>
Yes, if both of the strings are NOT known to have enough length.
It is safe to replace strncmp(a,b,n) with memcmp(a,b,n)
if a or b is/are known to have enough length; strlen(a) >= n ||
strlen(b) >= n.
I think some of the replacements in the original patch are valid,
but for even those valid replacement, I think it is worth doing
that in hot code paths only.
--yoshfuji
Pavel Vasilyev <[email protected]> writes:
> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
Those two calls are not equivalent. Think what happens when a or b
are shorter than n and one of them is at the end of a page
before a memory hole.
-Andi
--
[email protected] -- Speaking for myself only.
On 29.11.2010 08:26, YOSHIFUJI Hideaki wrote:
> 2010-11-28 (Sun) 20:13 -0800, Dmitry Torokhov wrote:
>> On Mon, Nov 29, 2010 at 06:11:21AM +0300, Pavel Vasilyev wrote:
>>> On 29.11.2010 05:29, Ming Lei wrote:
>>>> 2010/11/29 Pavel Vasilyev <[email protected]>:
>>>>> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
>>>>> I test on x86_64 (AMD Opteron 285).
>>>> In fact, memcmp doesn't handle case of tail of string, so
>>>> it is not safe to replace strncmp with memcmp
>>> #include <stdio.h>
>>> #include <errno.h>
>>> int main() {
>>> char *STR = "XXXX\0";
>>> char *XXX = "XXXX";
>> Try comparing:
>> "XXXX\0YYYY" and
>> "XXXX\0ZZZZ"
>>
>> and observe the difference.
> Yes, if both of the strings are NOT known to have enough length.
>
> It is safe to replace strncmp(a,b,n) with memcmp(a,b,n)
> if a or b is/are known to have enough length; strlen(a) >= n ||
> strlen(b) >= n.
>
> I think some of the replacements in the original patch are valid,
> but for even those valid replacement, I think it is worth doing
> that in hot code paths only.
In my fact, the system has been running for two days, with this
configuration (in attach)
--
Pavel.
On Mon, 2010-11-29 at 05:09 +0300, Pavel Vasilyev wrote:
> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
But these are not the same. strncmp() will stop when a or b hit a null.
I'm not sure if memcmp() must do so, It may for some reason check
anything within the memory of a+c-1 or b+c-1. What happens if a or b are
right at the end of a vmalloc page, and is just a single character and
null?
x = vmalloc(32);
strcpy(x, "some 31 byte string + null");
call_func(x + 31);
in call_func we have:
call_func(char *a) {
strncmp(a, "this is some big string", 23);
With strncmp() when we hit a+1, it will stop comparing because a+1 is
null. With memcmp there's no such guarantee. We can then take a kernel
oops.
That will be a nice thing to try to debug.
Yes the above is contrived, but it demonstrates a possible problem with
this conversion.
-- Steve
>
> I test on x86_64 (AMD Opteron 285).
>
> #include <string.h>
> char *A = "0000";
> void test_memcmp(void) {
> memcmp(A, "TEST", 4);
> }
> void test_strn(void) {
> strncmp(A, "TEST", 4);
> }
> # gcc -c -O2 test.c
> # objdump -d test.o
> ...
>
> 0000000000000020 <test_strncmp>:
> 20: f3 c3 repz retq
> 22: 66 66 66 66 66 2e 0f data32 data32 data32 data32 nopw
> %cs:0x0(%rax,%rax,1)
> 29: 1f 84 00 00 00 00 00
>
> 0000000000000030 <test_memcmp>:
> 30: f3 c3 repz retq
>
> Wow, minus one commad :)
>
On 29.11.2010 17:58, Steven Rostedt wrote:
> On Mon, 2010-11-29 at 05:09 +0300, Pavel Vasilyev wrote:
>> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
> But these are not the same. strncmp() will stop when a or b hit a null.
> I'm not sure if memcmp() must do so, It may for some reason check
> anything within the memory of a+c-1 or b+c-1. What happens if a or b are
> right at the end of a vmalloc page, and is just a single character and
> null?
>
> x = vmalloc(32);
> strcpy(x, "some 31 byte string + null");
>
> call_func(x + 31);
>
> in call_func we have:
>
> call_func(char *a) {
>
> strncmp(a, "this is some big string", 23);
>
> With strncmp() when we hit a+1, it will stop comparing because a+1 is
> null. With memcmp there's no such guarantee. We can then take a kernel
> oops.
>
> That will be a nice thing to try to debug.
>
> Yes the above is contrived, but it demonstrates a possible problem with
> this conversion.
#include <stdio.h>
#include <errno.h>
char STR[5] = {'X','X','\0','X','X'};
char *XXX = "XX\0XX";
int main ()
{
int a, b;
a = memcmp (XXX, STR, 5);
b = strcmp (XXX, STR);
printf (": %d %d \n", a, b);
return 0;
}
./a.out
0 0
:)
#gdb ./a.out
(gdb) b main
Breakpoint 1 at 0x4005dc: file test.c, line 10.
(gdb) run
Starting program: /tmp/a.out
Breakpoint 1, main () at test.c:10
10 a = memcmp (STR, XXX, 5);
(gdb) print XXX
$1 = 0x400731 "XX"
(gdb) print STR
$2 = "XX\000XX"
....
Oops, variable XXX set to XX, var. STR not changed.
Seems to me, that they into strsmp() and memcmp() already gets without
the null character.
P.S.
pavel@suse64:/tmp> gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/4.5/lto-wrapper
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info
--mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64
--enable-languages=c,c++,objc,fortran,obj-c++,java,ada
--enable-checking=release --with-gxx-include-dir=/usr/include/c++/4.5
--enable-ssp --disable-libssp --disable-plugin
--with-bugurl=http://bugs.opensuse.org/ --with-pkgversion='SUSE Linux'
--disable-libgcj --disable-libmudflap --with-slibdir=/lib64
--with-system-zlib --enable-__cxa_atexit
--enable-libstdcxx-allocator=new --disable-libstdcxx-pch
--enable-version-specific-runtime-libs --program-suffix=-4.5
--enable-linux-futex --without-system-libunwind --enable-gold
--with-plugin-ld=/usr/bin/gold --with-arch-32=i586 --with-tune=generic
--build=x86_64-suse-linux
Thread model: posix
gcc version 4.5.1 20101116 [gcc-4_5-branch revision 166793] (SUSE Linux
--
Pavel.
On Mon, 2010-11-29 at 22:41 +0300, Pavel Vasilyev wrote:
> On 29.11.2010 17:58, Steven Rostedt wrote:
> > On Mon, 2010-11-29 at 05:09 +0300, Pavel Vasilyev wrote:
> >> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
> > But these are not the same. strncmp() will stop when a or b hit a null.
> > I'm not sure if memcmp() must do so, It may for some reason check
> > anything within the memory of a+c-1 or b+c-1. What happens if a or b are
> > right at the end of a vmalloc page, and is just a single character and
> > null?
> >
> > x = vmalloc(32);
> > strcpy(x, "some 31 byte string + null");
> >
> > call_func(x + 31);
> >
> > in call_func we have:
> >
> > call_func(char *a) {
> >
> > strncmp(a, "this is some big string", 23);
> >
> > With strncmp() when we hit a+1, it will stop comparing because a+1 is
> > null. With memcmp there's no such guarantee. We can then take a kernel
> > oops.
> >
> > That will be a nice thing to try to debug.
> >
> > Yes the above is contrived, but it demonstrates a possible problem with
> > this conversion.
> #include <stdio.h>
> #include <errno.h>
>
> char STR[5] = {'X','X','\0','X','X'};
> char *XXX = "XX\0XX";
>
> int main ()
> {
> int a, b;
> a = memcmp (XXX, STR, 5);
> b = strcmp (XXX, STR);
> printf (": %d %d \n", a, b);
> return 0;
> }
> ./a.out
> 0 0
>
> :)
Um, do you realize that the kernel does not always use the same memcmp
as gcc.
>
> #gdb ./a.out
> (gdb) b main
> Breakpoint 1 at 0x4005dc: file test.c, line 10.
> (gdb) run
> Starting program: /tmp/a.out
>
> Breakpoint 1, main () at test.c:10
> 10 a = memcmp (STR, XXX, 5);
> (gdb) print XXX
> $1 = 0x400731 "XX"
> (gdb) print STR
> $2 = "XX\000XX"
> ....
> Oops, variable XXX set to XX, var. STR not changed.
> Seems to me, that they into strsmp() and memcmp() already gets without
> the null character.
>
> P.S.
> pavel@suse64:/tmp> gcc -v
This is meaningless, because the kernel may have its own memcmp and
strncmp.
And you seem to be missing my point. I'm not saying that it will give
you an incorrect result, I'm saying that if memcmp simply reads the
memory that is not mapped in, then you might cause the kernel to crash.
This is fine if all implementations of memcmp() reads the data
sequentially and stops when not equal. If for some reason there's an
implementation that compares, not from the beginning of the data, but
say from the end, then this can cause a fault.
Note, that strncmp and memcmp act differently for:
(a, b, c) where c is larger than strlen(a)+1 and strlen(b)+1,
and the two match. Try it:
char *a = "abc\0 123";
char *b = "abc\0 456";
printf("%d %d\n", strncmp(a, b, 10), memcmp(a, b, 10));
-- Steve
>
> Using built-in specs.
> COLLECT_GCC=gcc
> COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/4.5/lto-wrapper
> Target: x86_64-suse-linux
> Configured with: ../configure --prefix=/usr --infodir=/usr/share/info
> --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64
> --enable-languages=c,c++,objc,fortran,obj-c++,java,ada
> --enable-checking=release --with-gxx-include-dir=/usr/include/c++/4.5
> --enable-ssp --disable-libssp --disable-plugin
> --with-bugurl=http://bugs.opensuse.org/ --with-pkgversion='SUSE Linux'
> --disable-libgcj --disable-libmudflap --with-slibdir=/lib64
> --with-system-zlib --enable-__cxa_atexit
> --enable-libstdcxx-allocator=new --disable-libstdcxx-pch
> --enable-version-specific-runtime-libs --program-suffix=-4.5
> --enable-linux-futex --without-system-libunwind --enable-gold
> --with-plugin-ld=/usr/bin/gold --with-arch-32=i586 --with-tune=generic
> --build=x86_64-suse-linux
> Thread model: posix
> gcc version 4.5.1 20101116 [gcc-4_5-branch revision 166793] (SUSE Linux
On Mon, 2010-11-29 at 17:18 -0500, Steven Rostedt wrote:
> Um, do you realize that the kernel does not always use the same memcmp
> as gcc.
>
Note, I'm not against the change, because in 99% of the cases, memcmp()
can and will be faster, and we don't need to worry about these strange
cases.
What I'm against is a blind s/strncmp/memcmp/ of the kernel, because it
may have side effects that you may not have thought of.
-- Steve
On 30.11.2010 01:26, Steven Rostedt wrote:
> On Mon, 2010-11-29 at 17:18 -0500, Steven Rostedt wrote:
>
>> Um, do you realize that the kernel does not always use the same memcmp
>> as gcc.
>>
> Note, I'm not against the change, because in 99% of the cases, memcmp()
> can and will be faster, and we don't need to worry about these strange
> cases.
>
> What I'm against is a blind s/strncmp/memcmp/ of the kernel, because it
> may have side effects that you may not have thought of.
>
Then we have to hang a large banner that is visible to all developers.
"Change wherever possible strncmp to memcmp," :)
--
Pavel.
On 11/30/2010 11:26 AM, Steven Rostedt wrote:
> On Mon, 2010-11-29 at 17:18 -0500, Steven Rostedt wrote:
>
>> Um, do you realize that the kernel does not always use the same memcmp
>> as gcc.
>>
>
> Note, I'm not against the change, because in 99% of the cases, memcmp()
> can and will be faster, and we don't need to worry about these strange
> cases.
It can still break things in subtle ways. Lots of the replacements are
of the form:
if (strncmp(string, "foo", 3) == 0)
Which can only be replaced with memcmp if the minimum length of string
is _always_ 3. This may be true for some callsites (with careful audit),
but in general I doubt it is and it will lead to subtle bugs.
I hardly think it is worth auditing a bunch of strncmp calls to ensure
that the minimum length of the checked string is always n in order to
remove a single instruction. Making such a change will also introduce
subtle bugs if the rules for the string ever change, eg. a change is
made to allow string = "".
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
Hi,
On Sun, Nov 28, 2010 at 9:09 PM, Pavel Vasilyev <[email protected]> wrote:
>
> ? ?This patch replace all strncmp(a, b, c) by ?memcmp(a, b, c).
>
> I test on x86_64 (AMD Opteron 285).
>
> #include <string.h>
>
Could you at least do your "performance/code analysis" [NDLR: ahem...]
in a kernel environment ? <string.h> only belongs to userland and the
"K" of lkml stands for "kernel".
Thanks,
- Arnaud
ps: please don't send a +500k on the list, that's rude to many people.
On Tue, Nov 30, 2010 at 01:49:43AM +0300, Pavel Vasilyev wrote:
>On 30.11.2010 01:26, Steven Rostedt wrote:
>> On Mon, 2010-11-29 at 17:18 -0500, Steven Rostedt wrote:
>>
>>> Um, do you realize that the kernel does not always use the same memcmp
>>> as gcc.
>>>
>> Note, I'm not against the change, because in 99% of the cases, memcmp()
>> can and will be faster, and we don't need to worry about these strange
>> cases.
>>
>> What I'm against is a blind s/strncmp/memcmp/ of the kernel, because it
>> may have side effects that you may not have thought of.
>>
>Then we have to hang a large banner that is visible to all developers.
>"Change wherever possible strncmp to memcmp," :)
>
Why? Strings are strings, they are ended by '\0', well-deserved to use str*()
functions rather than mem*().
On Die, 2010-11-30 at 11:51 +1300, Ryan Mallon wrote:
[...]
> It can still break things in subtle ways. Lots of the replacements are
> of the form:
>
> if (strncmp(string, "foo", 3) == 0)
>
> Which can only be replaced with memcmp if the minimum length of string
> is _always_ 3. This may be true for some callsites (with careful audit),
> but in general I doubt it is and it will lead to subtle bugs.
>
> I hardly think it is worth auditing a bunch of strncmp calls to ensure
> that the minimum length of the checked string is always n in order to
> remove a single instruction. Making such a change will also introduce
> subtle bugs if the rules for the string ever change, eg. a change is
> made to allow string = "".
On the conceptual level:
And it will confuse people if strings (read: '\0' terminated char arrays
specified by a pointer to the start) with raw memory (read: memory with
arbitrary content - including '\0' not at the end - specified by a
pointer to the start and the valid size).
Even more confusing is that raw memory *may* hold a string ...
Don't get me wrong: every seasoned C programmer should know that. But
not everyone is one ...
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at
On Mon, 2010-11-29 at 05:09 +0300, Pavel Vasilyev wrote:
> This patch replace all strncmp(a, b, c) by memcmp(a, b, c).
>
> I test on x86_64 (AMD Opteron 285).
>
> #include <string.h>
> char *A = "0000";
> void test_memcmp(void) {
> memcmp(A, "TEST", 4);
> }
> void test_strn(void) {
> strncmp(A, "TEST", 4);
> }
[...]
You use constant parameters with user-space functions defined by the
C-standard on one architecture (and CPU?) - and even worse the width is
4 so - for the memcmp() case - it boilsdown to a comparison of unsigned
ints.
So that example is (also) completely worthless.
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at