[2.6 patch] kill IN_STRING_C

[2.6 patch] kill IN_STRING_C

[Adrian Bunk]

Hi Andi,

some months ago, you invented a IN_STRING_C with the following comment:

<--  snip  -->

gcc 3.4 optimizes sprintf(foo,"%s",string) into strcpy.  

Unfortunately that isn't seen by the inliner and linux/i386 has no 
out-of-line strcpy so you end up with a linker error.

This patch adds out of line copies for most string functions to avoid 
this.
...

<--  snip  -->


I tried 2.6.10-rc1-mm3 with gcc 3.4.2 and the patch below and didn't 
observe the problems you described.


Can you still reproduce this problem?
If not, I'll suggest to apply the patch below which saves a few kB in 
lib/string.o .



Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.10-rc1-mm3-full/include/asm-i386/string.h.old	2004-11-07 13:27:44.000000000 +0100
+++ linux-2.6.10-rc1-mm3-full/include/asm-i386/string.h	2004-11-07 13:28:47.000000000 +0100
@@ -25,7 +25,6 @@
 
 /* AK: in fact I bet it would be better to move this stuff all out of line.
  */
-#if !defined(IN_STRING_C)
 
 #define __HAVE_ARCH_STRCPY
 static inline char * strcpy(char * dest,const char *src)
@@ -180,8 +179,6 @@
 return __res;
 }
 
-#endif
-
 #define __HAVE_ARCH_STRLEN
 static inline size_t strlen(const char * s)
 {
--- linux-2.6.10-rc1-mm3-full/lib/string.c.old	2004-11-07 13:29:00.000000000 +0100
+++ linux-2.6.10-rc1-mm3-full/lib/string.c	2004-11-07 13:29:05.000000000 +0100
@@ -19,8 +19,6 @@
  * -  Kissed strtok() goodbye
  */
 
-#define IN_STRING_C 1
- 
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>

Re: [2.6 patch] kill IN_STRING_C

[Andi Kleen]

> Can you still reproduce this problem?
> If not, I'll suggest to apply the patch below which saves a few kB in 
> lib/string.o .

I would prefer to keep it because there is no guarantee in gcc
that it always inlines all string functions unless you pass
-minline-all-stringops. And with that the code would
be bloated much more than the few out of lined fallback
string functions.

Even if it works for you with your configuration it's just by luck.

-Andi

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 02:44:49PM +0100, Andi Kleen wrote:

> > Can you still reproduce this problem?
> > If not, I'll suggest to apply the patch below which saves a few kB in 
> > lib/string.o .
> 
> I would prefer to keep it because there is no guarantee in gcc
> that it always inlines all string functions unless you pass
> -minline-all-stringops. And with that the code would
> be bloated much more than the few out of lined fallback
> string functions.

If I understand your changelog entry correctly, this wasn't the problem
(the asm string functions are "static inline").

Rethinking it, I don't even understand the sprintf example in your 
changelog entry - shouldn't an inclusion of kernel.h always get it 
right?

> Even if it works for you with your configuration it's just by luck.

The two configurations I tried are one configuration with _everything_ 
except modules enablesd, and the other was _everything_ modular.

That's why I'd like to have an example where it fails to understand the 
problem.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[Andi Kleen]

> Rethinking it, I don't even understand the sprintf example in your 
> changelog entry - shouldn't an inclusion of kernel.h always get it 
> right?

Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str) transparently.

-Andi

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > Rethinking it, I don't even understand the sprintf example in your 
> > changelog entry - shouldn't an inclusion of kernel.h always get it 
> > right?
> 
> Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str) transparently.

Which gcc is "Newer"?

My gcc 3.4.2 didn't show this problem.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[Andi Kleen]

On Mon, Nov 08, 2004 at 05:31:01PM +0100, Adrian Bunk wrote:
> On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > > Rethinking it, I don't even understand the sprintf example in your 
> > > changelog entry - shouldn't an inclusion of kernel.h always get it 
> > > right?
> > 
> > Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str) transparently.
> 
> Which gcc is "Newer"?

I saw it with 3.3-hammer, which had additional optimizations in this 
area at some point. Note that 3.3-hammer is widely used. I don't 
know if 3.4 does it in the same way.

-Andi

Re: [2.6 patch] kill IN_STRING_C

[Paweł Sikora]

On Monday 08 of November 2004 17:31, you wrote:
> On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > > Rethinking it, I don't even understand the sprintf example in your
> > > changelog entry - shouldn't an inclusion of kernel.h always get it
> > > right?
> >
> > Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str)
> > transparently.
>
> Which gcc is "Newer"?
>
> My gcc 3.4.2 didn't show this problem.

#include <stdio.h>
#include <string.h>
char buf[128];
void test(char *str)
{
    sprintf(buf, "%s", str);
}

gcc -Wall sp.c -S -O2 -fomit-frame-pointer -mregparm=3

        .file   "sp.c"
        .text
        .p2align 4,,15
.globl test
        .type   test, @function

test:
        movl    %eax, %edx
        movl    $buf, %eax
        jmp     strcpy

        .size   test, .-test
        .comm   buf,128,32
        .section        .note.GNU-stack,"",@progbits
        .ident  "GCC: (GNU) 3.4.3 (PLD Linux)"

-- 
/* Copyright (C) 2003, SCO, Inc. This is valuable Intellectual Property. */

                           #define say(x) lie(x)

Re: [2.6 patch] kill IN_STRING_C

[linux-os]

On Mon, 8 Nov 2004, Andi Kleen wrote:

>> Rethinking it, I don't even understand the sprintf example in your
>> changelog entry - shouldn't an inclusion of kernel.h always get it
>> right?
>
> Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str) transparently.
>
> -Andi

Hmmm, how does it get the correct return-value and type? I don't
think that a compiler is allowed to change the function(s) called.
If gcc is doing this now, there are many potential problems and
it needs to be fixed. For instance, one can't qualify a
'C' runtime library and then have a compiler decide that it's
not going to call the pre-qualified function.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 07:04:13PM +0100, Pawe?? Sikora wrote:
> On Monday 08 of November 2004 17:31, you wrote:
> > On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > > > Rethinking it, I don't even understand the sprintf example in your
> > > > changelog entry - shouldn't an inclusion of kernel.h always get it
> > > > right?
> > >
> > > Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str)
> > > transparently.
> >
> > Which gcc is "Newer"?
> >
> > My gcc 3.4.2 didn't show this problem.
> 
> #include <stdio.h>
> #include <string.h>
> char buf[128];
> void test(char *str)
> {
>     sprintf(buf, "%s", str);
> }
>...
>         jmp     strcpy
>...


This is the userspace example.

The kernel example is:

#include <linux/string.h>
#include <linux/kernel.h>

char buf[128];
void test(char *str)
{
  sprintf(buf, "%s", str);
}


This results with gcc-3.4 (GCC) 3.4.2 (Debian 3.4.2-3) in:

        .file   "test.c"
        .section        .rodata.str1.1,"aMS",@progbits,1
.LC0:
        .string "%s"
        .text
        .p2align 4,,15
.globl test
        .type   test, @function
test:
        pushl   %eax
        pushl   $.LC0
        pushl   $buf
        call    sprintf
        addl    $12, %esp
        ret
        .size   test, .-test
.globl buf
        .bss
        .align 32
        .type   buf, @object
        .size   buf, 128
buf:
        .zero   128
        .section        .note.GNU-stack,"",@progbits
        .ident  "GCC: (GNU) 3.4.2 (Debian 3.4.2-3)"



cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 06:51:20PM +0100, Andi Kleen wrote:
> On Mon, Nov 08, 2004 at 05:31:01PM +0100, Adrian Bunk wrote:
> > On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > > > Rethinking it, I don't even understand the sprintf example in your 
> > > > changelog entry - shouldn't an inclusion of kernel.h always get it 
> > > > right?
> > > 
> > > Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str) transparently.
> > 
> > Which gcc is "Newer"?
> 
> I saw it with 3.3-hammer, which had additional optimizations in this 
> area at some point. Note that 3.3-hammer is widely used. I don't 
> know if 3.4 does it in the same way.

Is this a -hammer specific problem?
If yes, does a -no-builtin-sprintf fix it?

Or is the problem a missing #include <linux/kernel.h> at the top of 
include/linux/string.h?

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[Andi Kleen]

On Mon, Nov 08, 2004 at 07:34:49PM +0100, Adrian Bunk wrote:
> On Mon, Nov 08, 2004 at 06:51:20PM +0100, Andi Kleen wrote:
> > On Mon, Nov 08, 2004 at 05:31:01PM +0100, Adrian Bunk wrote:
> > > On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > > > > Rethinking it, I don't even understand the sprintf example in your 
> > > > > changelog entry - shouldn't an inclusion of kernel.h always get it 
> > > > > right?
> > > > 
> > > > Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str) transparently.
> > > 
> > > Which gcc is "Newer"?
> > 
> > I saw it with 3.3-hammer, which had additional optimizations in this 
> > area at some point. Note that 3.3-hammer is widely used. I don't 
> > know if 3.4 does it in the same way.
> 
> Is this a -hammer specific problem?

No, I just checked a 4.0 mainline gcc and it does it too.

Note I saw it on x86-64, don't know if it occurs on i386 too.

-Andi

Re: [2.6 patch] kill IN_STRING_C

[Paweł Sikora]

On Monday 08 of November 2004 19:52, you wrote:
> On Mon, Nov 08, 2004 at 07:42:38PM +0100, Pawe?? Sikora wrote:
> >...
> > [~/rpm/BUILD] # gcc -Wall sp.c -S -O2 -fomit-frame-pointer -mregparm=3
> >                     -nostdinc -isystem /usr/src/linux/include
> >
> > sp.c: In function `test':
> > sp.c:7: warning: implicit declaration of function `sprintf'
> >
> > [~/rpm/BUILD] # cat sp.s
> >
> >         .file   "sp.c"
> >         .text
> >         .p2align 4,,15
> > .globl test
> >         .type   test, @function
> > test:
> >         movl    %eax, %edx
> >         movl    $buf, %eax
> >         jmp     strcpy
> >         .size   test, .-test
> >         .comm   buf,128,32
> >         .section        .note.GNU-stack,"",@progbits
> >         .ident  "GCC: (GNU) 3.4.3 (PLD Linux)"
> >
> >
> > What now?
>
> Do a "make V=1" and use the complete gcc call you see there.

[~/rpm/BUILD/linux-2.6.10-rc1] #

gcc -nostdinc -iwithprefix include -D__KERNEL__ -Iinclude  -Wall
-Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -O2
-Wdeclaration-after-statement -pipe -msoft-float -mpreferred-stack-boundary=2
-fno-unit-at-a-time -march=pentium3 -Iinclude/asm-i386/mach-default -S sp.c

        .file   "sp.c"
        .text
        .p2align 4,,15
.globl test
        .type   test, @function
test:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $8, %esp
        movl    $buf, (%esp)
        movl    8(%ebp), %eax
        movl    %eax, 4(%esp)
        call    strcpy
        leave
        ret
        .size   test, .-test
        .p2align 4,,15
        .type   strcpy, @function
strcpy:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $12, %esp
        movl    %esi, -8(%ebp)
        movl    8(%ebp), %edx
        movl    12(%ebp), %esi
        movl    %edi, -4(%ebp)
        movl    %edx, %edi
#APP
        1:      lodsb
        stosb
        testb %al,%al
        jne 1b
#NO_APP
        movl    -8(%ebp), %esi
        movl    %edx, %eax
        movl    -4(%ebp), %edi
        movl    %ebp, %esp
        popl    %ebp
        ret
        .size   strcpy, .-strcpy
.globl buf
        .bss
        .align 32
        .type   buf, @object
        .size   buf, 128
buf:
        .zero   128
        .section        .note.GNU-stack,"",@progbits
        .ident  "GCC: (GNU) 3.4.3 (PLD Linux)"

-- 
/* Copyright (C) 2003, SCO, Inc. This is valuable Intellectual Property. */

                           #define say(x) lie(x)

Re: [2.6 patch] kill IN_STRING_C

[linux-os]

On Mon, 8 Nov 2004, Adrian Bunk wrote:

> On Mon, Nov 08, 2004 at 07:04:13PM +0100, Pawe?? Sikora wrote:
>> On Monday 08 of November 2004 17:31, you wrote:
>>> On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
>>>>> Rethinking it, I don't even understand the sprintf example in your
>>>>> changelog entry - shouldn't an inclusion of kernel.h always get it
>>>>> right?
>>>>
>>>> Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str)
>>>> transparently.
>>>
>>> Which gcc is "Newer"?
>>>
>>> My gcc 3.4.2 didn't show this problem.
>>
>> #include <stdio.h>
>> #include <string.h>
>> char buf[128];
>> void test(char *str)
>> {
>>     sprintf(buf, "%s", str);
>> }
>> ...
>>         jmp     strcpy
>> ...
>
>
> This is the userspace example.
>
> The kernel example is:
>
> #include <linux/string.h>
> #include <linux/kernel.h>
>
> char buf[128];
> void test(char *str)
> {
>  sprintf(buf, "%s", str);
> }
>
>
> This results with gcc-3.4 (GCC) 3.4.2 (Debian 3.4.2-3) in:
>
>        .file   "test.c"
>        .section        .rodata.str1.1,"aMS",@progbits,1
> .LC0:
>        .string "%s"
>        .text
>        .p2align 4,,15
> .globl test
>        .type   test, @function
> test:
>        pushl   %eax
>        pushl   $.LC0
>        pushl   $buf
>        call    sprintf
>        addl    $12, %esp
>        ret
>        .size   test, .-test
> .globl buf
>        .bss
>        .align 32
>        .type   buf, @object
>        .size   buf, 128
> buf:
>        .zero   128
>        .section        .note.GNU-stack,"",@progbits
>        .ident  "GCC: (GNU) 3.4.2 (Debian 3.4.2-3)"
>
>
>
> cu
> Adrian
>

On this compiler 3.3.3, -O2 will cause it to use strcpy().

Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.

Re: [2.6 patch] kill IN_STRING_C

[Ryan Cumming]

On Monday 08 November 2004 10:22, linux-os wrote:
> Hmmm, how does it get the correct return-value and type?
Presumably it does it only for sprintf() calls where the return value is 
ignored.

> I don't 
> think that a compiler is allowed to change the function(s) called.
GCC is making the assumption that the functions it's replacing comply with the 
C standard. I personally don't think that's too insane, especially since it 
can be turned off (see below).

> If gcc is doing this now, there are many potential problems and
> it needs to be fixed. For instance, one can't qualify a
> 'C' runtime library and then have a compiler decide that it's
> not going to call the pre-qualified function.
-fno-builtin

-Ryan

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 08:11:46PM +0100, Pawe?? Sikora wrote:
> 
> gcc -nostdinc -iwithprefix include -D__KERNEL__ -Iinclude  -Wall
> -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -O2
> -Wdeclaration-after-statement -pipe -msoft-float -mpreferred-stack-boundary=2
> -fno-unit-at-a-time -march=pentium3 -Iinclude/asm-i386/mach-default -S sp.c
> 
>...
>         call    strcpy
>...
> strcpy:
>         pushl   %ebp
>...

That's no function call.  :-)

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 02:12:18PM -0500, linux-os wrote:
> 
> On this compiler 3.3.3, -O2 will cause it to use strcpy().

Not for me:

        .file   "test.c"
        .section        .rodata.str1.1,"aMS",@progbits,1
.LC0:
        .string "%s"
        .text
        .p2align 4,,15
.globl test
        .type   test, @function
test:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $12, %esp
        movl    %eax, 8(%esp)
        movl    $.LC0, %eax
        movl    %eax, 4(%esp)
        movl    $buf, (%esp)
        call    sprintf
        movl    %ebp, %esp
        popl    %ebp
        ret
        .size   test, .-test
.globl buf
        .bss
        .align 32
        .type   buf, @object
        .size   buf, 128
buf:
        .zero   128
        .section        .note.GNU-stack,"",@progbits
        .ident  "GCC: (GNU) 3.3.5 (Debian 1:3.3.5-2)"



Are you using exactly my example file?
Are you using the complete gcc command line as shown by "make V=1"?
Which gcc 3.3.3 are you using?


> Cheers,
> Dick Johnson

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[linux-os]

On Mon, 8 Nov 2004, Adrian Bunk wrote:

> On Mon, Nov 08, 2004 at 02:12:18PM -0500, linux-os wrote:
>>
>> On this compiler 3.3.3, -O2 will cause it to use strcpy().
>
> Not for me:
>
>        .file   "test.c"
>        .section        .rodata.str1.1,"aMS",@progbits,1
> .LC0:
>        .string "%s"
>        .text
>        .p2align 4,,15
> .globl test
>        .type   test, @function
> test:
>        pushl   %ebp
>        movl    %esp, %ebp
>        subl    $12, %esp
>        movl    %eax, 8(%esp)
>        movl    $.LC0, %eax
>        movl    %eax, 4(%esp)
>        movl    $buf, (%esp)
>        call    sprintf
>        movl    %ebp, %esp
>        popl    %ebp
>        ret
>        .size   test, .-test
> .globl buf
>        .bss
>        .align 32
>        .type   buf, @object
>        .size   buf, 128
> buf:
>        .zero   128
>        .section        .note.GNU-stack,"",@progbits
>        .ident  "GCC: (GNU) 3.3.5 (Debian 1:3.3.5-2)"
>
>
>
> Are you using exactly my example file?
> Are you using the complete gcc command line as shown by "make V=1"?
> Which gcc 3.3.3 are you using?
>

No, I am using (no headers):
-------------------
extern int sprintf(char *, const char *,...);
extern int puts(const char *);
static const char hello[]="Hello";
int xxx(void);
int xxx(){
    char buf[0x100];
    sprintf(buf, "%s", hello);
    puts(buf);
    return 0;
}
--------------------

Compiled as:

gcc -O2 -Wall -S -o xxx xxx.c

I get:

 	.file	"xxx.c"
 	.section	.rodata
 	.type	hello, @object
 	.size	hello, 6
hello:
 	.string	"Hello"
 	.text
 	.p2align 2,,3
.globl xxx
 	.type	xxx, @function
xxx:
 	pushl	%ebp
 	movl	%esp, %ebp
 	pushl	%ebx
 	subl	$268, %esp
 	pushl	$hello
 	leal	-264(%ebp), %ebx
 	pushl	%ebx
 	call	strcpy
 	movl	%ebx, (%esp)
 	call	puts
 	xorl	%eax, %eax
 	movl	-4(%ebp), %ebx
 	leave
 	ret
 	.size	xxx, .-xxx
 	.section	.note.GNU-stack,"",@progbits
 	.ident	"GCC: (GNU) 3.3.3 20040412 (Red Hat Linux 3.3.3-7)"


If I don't use -O2, I get the sprintf() call. If I use
the constant string "Hello" instead of the allocated string,
it just bypasses everything and calls puts() directly.

If I use -fno-builtin, then it doesn't bypass sprintf().
However, there is no ISO C standard of "built-in" so
I don't think any compiler should default to something
that is undefined and decide to do whatever its current
whims are. Certainly, a compiler that has some capabilities,
not defined by a standard, should be able to use those
capabilities, but it certainly shouldn't decide to do
these things on its own.

Reviewing `man gcc` I see where I should be able to
find out what the built-in commands are:

 	gcc -dumpspecs

However, this man page is wrong because I don't get a
listing of any built-in functions, only the linking
and compiler defaults.

The compiler is a tool. It should be just like other
tools. Any killer options should be something that
take a special effort to turn ON. It shouldn't default
to firing the bullet out both ends of the barrel!

Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 05:15:14PM -0500, linux-os wrote:
> >...
> >Are you using exactly my example file?
> >Are you using the complete gcc command line as shown by "make V=1"?
> >Which gcc 3.3.3 are you using?
> >
> 
> No, I am using (no headers):
> -------------------
> extern int sprintf(char *, const char *,...);
> extern int puts(const char *);
> static const char hello[]="Hello";
> int xxx(void);
> int xxx(){
>    char buf[0x100];
>    sprintf(buf, "%s", hello);
>    puts(buf);
>    return 0;
> }
> --------------------
> 
> Compiled as:
> 
> gcc -O2 -Wall -S -o xxx xxx.c
>...

If you don't compile the code as it would be compiled inside the kernel, 
that's your fault...

Please reply only if you can reproduce this with
#include <linux/string.h>, #include <linux/kernel.h> _and_ a gcc command
line as it would be in the kernel - everything else is useless.

> Cheers,
> Dick Johnson

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[linux-os]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2211 bytes --]

On Mon, 8 Nov 2004, Adrian Bunk wrote:
>
> If you don't compile the code as it would be compiled inside the kernel,
> that's your fault...
>
> Please reply only if you can reproduce this with
> #include <linux/string.h>, #include <linux/kernel.h> _and_ a gcc command
> line as it would be in the kernel - everything else is useless.

I attached a script that duplicates the gcc command line for the
compile and also generates the 'C' code.

Script started on Mon 08 Nov 2004 05:50:54 PM EST
# sh -v xxx.sh
#!/bin/bash

cat >xxx.c <<EOF
KERN=/usr/src/linux-`uname -r`
uname -r
touch xxx.o.d
gcc -Wp,-MD,xxx.o.d -nostdinc -iwithprefix include -D__KERNEL__ -I${KERN}/include  -Wall -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -O2 -fomit-frame-pointer -Wdeclaration-after-statement -pipe -msoft-float -mpreferred-stack-boundary=2  -march=pentium4 -I${KERN}/include/asm-i386/mach-generic -I${KERN}/include/asm-i386/mach-default -DKVER6 -DMAJOR_NR=178 -DTRUE=1 -DFALSE=0  -DMODULE -DKBUILD_BASENAME=xxx -DKBUILD_MODNAME=Junk -S -o xxx xxx.c
cat xxx
 	.file	"xxx.c"
 	.section	.rodata
 	.type	hello, @object
 	.size	hello, 6
hello:
 	.string	"Hello"
 	.text
.globl xxx
 	.type	xxx, @function
xxx:
 	subl	$268, %esp
 	movl	%ebx, 264(%esp)
 	movl	$hello, 4(%esp)
 	leal	8(%esp), %ebx
 	movl	%ebx, (%esp)
 	call	strcpy
 	movl	%ebx, (%esp)
 	call	printk
 	movl	264(%esp), %ebx
 	xorl	%eax, %eax
 	addl	$268, %esp
 	ret
 	.size	xxx, .-xxx
 	.type	strcpy, @function
strcpy:
 	subl	$8, %esp
 	movl	12(%esp), %ecx
 	movl	%esi, (%esp)
 	movl	%edi, 4(%esp)
 	movl	16(%esp), %esi
 	movl	%ecx, %edi
#APP
 	1:	lodsb
 	stosb
 	testb %al,%al
 	jne 1b
#NO_APP
 	movl	%ecx, %eax
 	movl	(%esp), %esi
 	movl	4(%esp), %edi
 	addl	$8, %esp
 	ret
 	.size	strcpy, .-strcpy
 	.section	.note.GNU-stack,"",@progbits
 	.ident	"GCC: (GNU) 3.3.3 20040412 (Red Hat Linux 3.3.3-7)"

# exit

Script done on Mon 08 Nov 2004 05:51:03 PM EST


It clearly invents strcpy, having never been referenced in the
source.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.

[-- Attachment #2: Type: APPLICATION/x-sh, Size: 751 bytes --]

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Mon, Nov 08, 2004 at 05:57:12PM -0500, linux-os wrote:
>...
> 	call	strcpy
>...
> strcpy:
> 	subl	$8, %esp
>...
> It clearly invents strcpy, having never been referenced in the
> source.

The asm code you sent does _not_ call a global strcpy function.
It calls an asm procedure named "strcpy" it ships itself.

BTW: You are the second person in this thread I have to explain this to...

> Cheers,
> Dick Johnson

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Use -ffreestanding?

[Adrian Bunk]

On Mon, Nov 08, 2004 at 08:01:30PM +0100, Andi Kleen wrote:
> On Mon, Nov 08, 2004 at 07:34:49PM +0100, Adrian Bunk wrote:
> > On Mon, Nov 08, 2004 at 06:51:20PM +0100, Andi Kleen wrote:
> > > On Mon, Nov 08, 2004 at 05:31:01PM +0100, Adrian Bunk wrote:
> > > > On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > > > > > Rethinking it, I don't even understand the sprintf example in your 
> > > > > > changelog entry - shouldn't an inclusion of kernel.h always get it 
> > > > > > right?
> > > > > 
> > > > > Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str) transparently.
> > > > 
> > > > Which gcc is "Newer"?
> > > 
> > > I saw it with 3.3-hammer, which had additional optimizations in this 
> > > area at some point. Note that 3.3-hammer is widely used. I don't 
> > > know if 3.4 does it in the same way.
> > 
> > Is this a -hammer specific problem?
> 
> No, I just checked a 4.0 mainline gcc and it does it too.
> 
> Note I saw it on x86-64, don't know if it occurs on i386 too.

OK, I see the difference:
After removing -fno-unit-at-a-time, I see this problem, too.

Why doesn't the kernel use -ffreestanding which should prevent all such 
problems?

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Use -ffreestanding?

[Andi Kleen]

> Why doesn't the kernel use -ffreestanding which should prevent all such 
> problems?

Because we want most of these optimizations. Also with -ffreestanding
you would need to supply the out of line string functions anyways 
because gcc wouldn't inline them.

-Andi

Re: [2.6 patch] kill IN_STRING_C

[linux-os]

On Tue, 9 Nov 2004, Adrian Bunk wrote:

> On Mon, Nov 08, 2004 at 05:57:12PM -0500, linux-os wrote:
>> ...
>> 	call	strcpy
>> ...
>> strcpy:
>> 	subl	$8, %esp
>> ...
>> It clearly invents strcpy, having never been referenced in the
>> source.
>
> The asm code you sent does _not_ call a global strcpy function.
> It calls an asm procedure named "strcpy" it ships itself.
>
> BTW: You are the second person in this thread I have to explain this to...
>
>> Cheers,
>> Dick Johnson
>
> cu
> Adrian

Explain WHAT? There is NO strcpy in the code. No such procedure
should have been called. Period. The generated code is defective.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.

Re: [2.6 patch] kill IN_STRING_C

[linux-os]

On Tue, 9 Nov 2004, linux-os wrote:

> On Tue, 9 Nov 2004, Adrian Bunk wrote:
>
>> On Mon, Nov 08, 2004 at 05:57:12PM -0500, linux-os wrote:
>>> ...
>>> 	call	strcpy
>>> ...
>>> strcpy:
>>> 	subl	$8, %esp
>>> ...
>>> It clearly invents strcpy, having never been referenced in the
>>> source.
>> 
>> The asm code you sent does _not_ call a global strcpy function.
>> It calls an asm procedure named "strcpy" it ships itself.
>> 
>> BTW: You are the second person in this thread I have to explain this to...
>> 
>>> Cheers,
>>> Dick Johnson
>> 
>> cu
>> Adrian
>
> Explain WHAT? There is NO strcpy in the code. No such procedure
> should have been called. Period. The generated code is defective.
>

Here is more information showing the defective code generation.

Output of `nm`

00000000 r hello
          U printk
00000039 t strcpy
00000000 T xxx

Clearly shows a strcpy symbol when strcpy() was never referenced
in the 'C' code.

Output of objdump

xxx.o:     file format elf32-i386

Disassembly of section .text:

00000000 <xxx>:
    0:	81 ec 0c 01 00 00    	sub    $0x10c,%esp
    6:	89 9c 24 08 01 00 00 	mov    %ebx,0x108(%esp)
    d:	c7 44 24 04 00 00 00 	movl   $0x0,0x4(%esp)
   14:	00
   15:	8d 5c 24 08          	lea    0x8(%esp),%ebx
   19:	89 1c 24             	mov    %ebx,(%esp)
   1c:	e8 18 00 00 00       	call   39 <strcpy>
   21:	89 1c 24             	mov    %ebx,(%esp)
   24:	e8 fc ff ff ff       	call   25 <xxx+0x25>
   29:	8b 9c 24 08 01 00 00 	mov    0x108(%esp),%ebx
   30:	31 c0                	xor    %eax,%eax
   32:	81 c4 0c 01 00 00    	add    $0x10c,%esp
   38:	c3                   	ret

00000039 <strcpy>:
   39:	83 ec 08             	sub    $0x8,%esp
   3c:	8b 4c 24 0c          	mov    0xc(%esp),%ecx
   40:	89 34 24             	mov    %esi,(%esp)
   43:	89 7c 24 04          	mov    %edi,0x4(%esp)
   47:	8b 74 24 10          	mov    0x10(%esp),%esi
   4b:	89 cf                	mov    %ecx,%edi
   4d:	ac                   	lods   %ds:(%esi),%al
   4e:	aa                   	stos   %al,%es:(%edi)
   4f:	84 c0                	test   %al,%al
   51:	75 fa                	jne    4d <strcpy+0x14>
   53:	89 c8                	mov    %ecx,%eax
   55:	8b 34 24             	mov    (%esp),%esi
   58:	8b 7c 24 04          	mov    0x4(%esp),%edi
   5c:	83 c4 08             	add    $0x8,%esp
   5f:	c3                   	ret

[SNIPPED...]

Clearly shows a GLOBAL procedure called strcpy when
no such procedure was ever referenced in the code.

Now I will show that the strcpy() procedure is NOT local.
It's global and can screw up anything:

Another program....


char printk;				// Just quiet the reference
 					// ... just a label, never called.
extern int puts(const char *);			// No header funnies
extern char *strcpy(char *, const char *);
int main(){
    char buf[0x100];
    strcpy(buf, "WTF?");				// Going to call it!
    puts(buf);
    return 0;
}

Show code generation.

$ gcc -S -o aaa zzz.c

 	.file	"zzz.c"
 	.section	.rodata
.LC0:
 	.string	"WTF?"
 	.text
.globl main
 	.type	main, @function
main:
 	pushl	%ebp
 	movl	%esp, %ebp
 	subl	$264, %esp
 	andl	$-16, %esp
 	movl	$0, %eax
 	subl	%eax, %esp
 	subl	$8, %esp
 	pushl	$.LC0
 	leal	-264(%ebp), %eax
 	pushl	%eax
 	call	strcpy			# Clearly calls it.
 	addl	$16, %esp
 	subl	$12, %esp
 	leal	-264(%ebp), %eax
 	pushl	%eax
 	call	puts
 	addl	$16, %esp
 	movl	$0, %eax
 	leave
 	ret
 	.size	main, .-main
 	.comm	printk,1,1
 	.section	.note.GNU-stack,"",@progbits
 	.ident	"GCC: (GNU) 3.3.3 20040412 (Red Hat Linux 3.3.3-7)"

Now compile and link with the file containing ROGUE strcpy().

$ gcc -o zzz zzz.c xxx.o

$ ./zzz
WTF?

Executes, calling a ROGUE function that MUST NOT exist.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.

Re: [2.6 patch] kill IN_STRING_C

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

> On Mon, Nov 08, 2004 at 02:44:49PM +0100, Andi Kleen wrote:
> 
> > > Can you still reproduce this problem?
> > > If not, I'll suggest to apply the patch below which saves a few kB in 
> > > lib/string.o .
> > 
> > I would prefer to keep it because there is no guarantee in gcc
> > that it always inlines all string functions unless you pass
> > -minline-all-stringops. And with that the code would
> > be bloated much more than the few out of lined fallback
> > string functions.
> 
> If I understand your changelog entry correctly, this wasn't the problem
> (the asm string functions are "static inline").

Actually, shouldn't the string functions be "extern inline" then?
That would mean we use the copy from lib/string.c instead of generating
a new copy for each file in which gcc decides not to inline the function.
It would also let you get rid of the IN_STRING_C hack that is needed
to avoid the clash of the "static inline" and the "extern" version.

	Arnd <><



[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[2.6 patch] Use -ffreestanding?

[Adrian Bunk]

On Tue, Nov 09, 2004 at 06:01:07AM +0100, Andi Kleen wrote:
> > Why doesn't the kernel use -ffreestanding which should prevent all such 
> > problems?
> 
> Because we want most of these optimizations. Also with -ffreestanding

Do we really want the compiler to silently replace in-kernel functions 
with built-ins?

You can still do an explicit
  #define strlen __builtin_strlen
if you want to use a gcc built-in function.

> you would need to supply the out of line string functions anyways 
> because gcc wouldn't inline them.

At least with gcc 3.4.2 on i386 adding -ffreestanding and your 
(i386-specific) IN_STRING_C hack removed compiles fine.

> -Andi


I'm open for examples why this actually doesn't work, but after my 
(limited) testin I'd suggest the patch below for inclusion in the next 
-mm.


Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.10-rc1-mm4-full-ffreestanding/Makefile.old	2004-11-09 22:27:06.000000000 +0100
+++ linux-2.6.10-rc1-mm4-full-ffreestanding/Makefile	2004-11-09 22:27:47.000000000 +0100
@@ -349,7 +349,8 @@
 CPPFLAGS        := -D__KERNEL__ $(LINUXINCLUDE)
 
 CFLAGS 		:= -Wall -Wstrict-prototypes -Wno-trigraphs \
-	  	   -fno-strict-aliasing -fno-common
+	  	   -fno-strict-aliasing -fno-common \
+		   -ffreestanding
 AFLAGS		:= -D__ASSEMBLY__
 
 export	VERSION PATCHLEVEL SUBLEVEL EXTRAVERSION LOCALVERSION KERNELRELEASE \

Re: [2.6 patch] Use -ffreestanding?

[Linus Torvalds]

On Wed, 10 Nov 2004, Adrian Bunk wrote:
> 
> I'm open for examples why this actually doesn't work, but after my 
> (limited) testin I'd suggest the patch below for inclusion in the next 
> -mm.

When was -ffreestanding introduced? Iow, it might not work with all 
compiler versions.. Apart from that, I think it makes sense. 

		Linus

Re: [2.6 patch] Use -ffreestanding?

[Adrian Bunk]

On Tue, Nov 09, 2004 at 05:51:41PM -0800, Linus Torvalds wrote:
> 
> On Wed, 10 Nov 2004, Adrian Bunk wrote:
> > 
> > I'm open for examples why this actually doesn't work, but after my 
> > (limited) testin I'd suggest the patch below for inclusion in the next 
> > -mm.
> 
> When was -ffreestanding introduced? Iow, it might not work with all 
> compiler versions.. Apart from that, I think it makes sense. 

It's supported in gcc 2.95 (which is the oldest compiler supported by 
kernel 2.6).

> 		Linus

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kill IN_STRING_C

[Adrian Bunk]

On Tue, Nov 09, 2004 at 02:58:34PM +0100, Arnd Bergmann wrote:
> > On Mon, Nov 08, 2004 at 02:44:49PM +0100, Andi Kleen wrote:
> > 
> > > > Can you still reproduce this problem?
> > > > If not, I'll suggest to apply the patch below which saves a few kB in 
> > > > lib/string.o .
> > > 
> > > I would prefer to keep it because there is no guarantee in gcc
> > > that it always inlines all string functions unless you pass
> > > -minline-all-stringops. And with that the code would
> > > be bloated much more than the few out of lined fallback
> > > string functions.
> > 
> > If I understand your changelog entry correctly, this wasn't the problem
> > (the asm string functions are "static inline").
> 
> Actually, shouldn't the string functions be "extern inline" then?
> That would mean we use the copy from lib/string.c instead of generating
> a new copy for each file in which gcc decides not to inline the function.
>...

In the kernel, we #define inline to __attribute__((always_inline)) [1]
leaving gcc no room for a decision to not inline it.

> 	Arnd <><

cu
Adrian

[1] only for gcc >= 3.1, but I don't remember problems with older gcc 
    versions

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] Use -ffreestanding?

[Bill Davidsen]

Linus Torvalds wrote:
> 
> On Wed, 10 Nov 2004, Adrian Bunk wrote:
> 
>>I'm open for examples why this actually doesn't work, but after my 
>>(limited) testin I'd suggest the patch below for inclusion in the next 
>>-mm.
> 
> 
> When was -ffreestanding introduced? Iow, it might not work with all 
> compiler versions.. Apart from that, I think it makes sense. 

 From RH 7.3:
   gcc version 2.96 20000731 (Red Hat Linux 7.3 2.96-113)

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: [2.6 patch] kill IN_STRING_C

[Paweł Sikora]

On Monday 08 of November 2004 19:31, you wrote:
> On Mon, Nov 08, 2004 at 07:04:13PM +0100, Pawe?? Sikora wrote:
> > On Monday 08 of November 2004 17:31, you wrote:
> > > On Mon, Nov 08, 2004 at 05:19:35PM +0100, Andi Kleen wrote:
> > > > > Rethinking it, I don't even understand the sprintf example in your
> > > > > changelog entry - shouldn't an inclusion of kernel.h always get it
> > > > > right?
> > > >
> > > > Newer gcc rewrites sprintf(buf,"%s",str) to strcpy(buf,str)
> > > > transparently.
> > >
> > > Which gcc is "Newer"?
> > >
> > > My gcc 3.4.2 didn't show this problem.
> >
> > #include <stdio.h>
> > #include <string.h>
> > char buf[128];
> > void test(char *str)
> > {
> >     sprintf(buf, "%s", str);
> > }
> >...
> >         jmp     strcpy
> >...
>
> This is the userspace example.
>
> The kernel example is:
>
> #include <linux/string.h>
> #include <linux/kernel.h>
>
> char buf[128];
> void test(char *str)
> {
>   sprintf(buf, "%s", str);
> }
>
>
> This results with gcc-3.4 (GCC) 3.4.2 (Debian 3.4.2-3) in:
>
>         .file   "test.c"
>         .section        .rodata.str1.1,"aMS",@progbits,1
> .LC0:
>         .string "%s"
>         .text
>         .p2align 4,,15
> .globl test
>         .type   test, @function
> test:
>         pushl   %eax
>         pushl   $.LC0
>         pushl   $buf
>         call    sprintf
>         addl    $12, %esp
>         ret
>         .size   test, .-test
> .globl buf
>         .bss
>         .align 32
>         .type   buf, @object
>         .size   buf, 128
> buf:
>         .zero   128
>         .section        .note.GNU-stack,"",@progbits
>         .ident  "GCC: (GNU) 3.4.2 (Debian 3.4.2-3)"

[~/rpm/BUILD] # cat sp.c

#include <linux/string.h>
#include <linux/kernel.h>

char buf[128];
void test(char *str)
{
    sprintf(buf, "%s", str);
}

[~/rpm/BUILD] # gcc -Wall sp.c -S -O2 -fomit-frame-pointer -mregparm=3
                    -nostdinc -isystem /usr/src/linux/include

sp.c: In function `test':
sp.c:7: warning: implicit declaration of function `sprintf'

[~/rpm/BUILD] # cat sp.s

        .file   "sp.c"
        .text
        .p2align 4,,15
.globl test
        .type   test, @function
test:
        movl    %eax, %edx
        movl    $buf, %eax
        jmp     strcpy
        .size   test, .-test
        .comm   buf,128,32
        .section        .note.GNU-stack,"",@progbits
        .ident  "GCC: (GNU) 3.4.3 (PLD Linux)"


What now?

-- 
/* Copyright (C) 2003, SCO, Inc. This is valuable Intellectual Property. */

                           #define say(x) lie(x)