kill IN_STRING_C

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 .

@@ -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)
 {

@@ -19,8 +19,6 @@
  * -  Kissed strtok() goodbye
  */
 
-#define IN_STRING_C 1
- 
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>

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

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?

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.

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

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

-Andi

Which gcc is "Newer"?

My gcc 3.4.2 didn't show this problem.

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

#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)

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

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.

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?

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

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.

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.

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

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.

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...

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

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.

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.

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

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?

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

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

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?

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

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

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.

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

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.

@@ -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 \

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

Linus

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

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

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

   -bill davidsen ([email protected])
"The secret to procrastination is to put things off until the
 last possible moment - but no longer"  -me

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.

Presumably it does it only for sprintf() calls where the return value is ignored.

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).

-fno-builtin

-Ryan

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 <><

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

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

[~/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)

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