[PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Geert Uytterhoeven]

Without -ffreestanding, gcc may replace calls to standard C library
functions by calls to other standard C library functions and/or inline
code.  This may cause link errors if the replacement code calls a standard
C library function that's implemented as a macro in the kernel.

E.g. gcc turned

    strncat(name, "%d", 2);

into a call to strlen() and a 16-bit store, causing a link failure, as
arch/m68k/include/asm/string.h provides strlen() using a macro:

    ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!

In addition, this saves ca. 64 bytes of text on a typical kernel build.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
http://kisskb.ellerman.id.au/kisskb/buildresult/8462108/

QUESTION: Should we re-enable -ffreestanding in the main Makefile instead?

It was removed in

commit 6edfba1b33c701108717f4e036320fc39abe1912
Author: Andi Kleen <ak@suse.de>
Date:   Sat Mar 25 16:29:49 2006 +0100

    [PATCH] x86_64: Don't define string functions to builtin

    gcc should handle this anyways, and it causes problems when
    sprintf is turned into strcpy by gcc behind our backs and
    the C fallback version of strcpy is actually defining __builtin_strcpy

    Then drop -ffreestanding from the main Makefile because it isn't
    needed anymore and implies -fno-builtin, which is wrong now.
    (it was only added for x86-64, so dropping it should be safe)

    Noticed by Roman Zippel

    Cc: Roman Zippel <zippel@linux-m68k.org>
    Signed-off-by: Andi Kleen <ak@suse.de>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

Subsequently, it got re-enabled for mips, sh, x86-32, um/x86, xtensa, and
score.

 arch/m68k/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/m68k/Makefile b/arch/m68k/Makefile
index 2f02acf..8e06a38 100644
--- a/arch/m68k/Makefile
+++ b/arch/m68k/Makefile
@@ -58,7 +58,7 @@ cpuflags-$(CONFIG_M5206e)	:= $(call cc-option,-mcpu=5206e,-m5200)
 cpuflags-$(CONFIG_M5206)	:= $(call cc-option,-mcpu=5206,-m5200)
 
 KBUILD_AFLAGS += $(cpuflags-y)
-KBUILD_CFLAGS += $(cpuflags-y) -pipe
+KBUILD_CFLAGS += $(cpuflags-y) -pipe -ffreestanding
 ifdef CONFIG_MMU
 # without -fno-strength-reduce the 53c7xx.c driver fails ;-(
 KBUILD_CFLAGS += -fno-strength-reduce -ffixed-a2
-- 
1.7.0.4

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Michal Marek]

(Added Andi to CC)

On 10.4.2013 11:24, Geert Uytterhoeven wrote:
[...]
> E.g. gcc turned
> 
>     strncat(name, "%d", 2);
> 
> into a call to strlen() and a 16-bit store, causing a link failure, as
> arch/m68k/include/asm/string.h provides strlen() using a macro:
> 
>     ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!
[...]
> QUESTION: Should we re-enable -ffreestanding in the main Makefile instead?
> 
> It was removed in
> 
> commit 6edfba1b33c701108717f4e036320fc39abe1912
> Author: Andi Kleen <ak@suse.de>
> Date:   Sat Mar 25 16:29:49 2006 +0100
> 
>     [PATCH] x86_64: Don't define string functions to builtin

My understanding is, that with -fnobuiltin, the compiler is not allowed
to make assumptions about functions if it does not see their definition,
even if they resemble standard functions. E.g. on x86_64, strlen() is
out-of-line, so gcc would have to assume, that strcmp() has side
effects. How about just naming the m68k inline function 'strlen'?

Michal

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Geert Uytterhoeven]

On Wed, Apr 10, 2013 at 12:18 PM, Michal Marek <mmarek@suse.cz> wrote:
> (Added Andi to CC)
>
> On 10.4.2013 11:24, Geert Uytterhoeven wrote:
> [...]
>> E.g. gcc turned
>>
>>     strncat(name, "%d", 2);
>>
>> into a call to strlen() and a 16-bit store, causing a link failure, as
>> arch/m68k/include/asm/string.h provides strlen() using a macro:
>>
>>     ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!
> [...]
>> QUESTION: Should we re-enable -ffreestanding in the main Makefile instead?
>>
>> It was removed in
>>
>> commit 6edfba1b33c701108717f4e036320fc39abe1912
>> Author: Andi Kleen <ak@suse.de>
>> Date:   Sat Mar 25 16:29:49 2006 +0100
>>
>>     [PATCH] x86_64: Don't define string functions to builtin
>
> My understanding is, that with -fnobuiltin, the compiler is not allowed
> to make assumptions about functions if it does not see their definition,
> even if they resemble standard functions. E.g. on x86_64, strlen() is
> out-of-line, so gcc would have to assume, that strcmp() has side
> effects. How about just naming the m68k inline function 'strlen'?

Having an inline function named "strlen" is not sufficient, as it needs an
(exported) symbol named "strlen" at link time or module load time.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Michal Marek]

On Wed, Apr 10, 2013 at 12:59:44PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 10, 2013 at 12:18 PM, Michal Marek <mmarek@suse.cz> wrote:
> > On 10.4.2013 11:24, Geert Uytterhoeven wrote:
> > My understanding is, that with -fnobuiltin, the compiler is not allowed
> > to make assumptions about functions if it does not see their definition,
> > even if they resemble standard functions. E.g. on x86_64, strlen() is
> > out-of-line, so gcc would have to assume, that strcmp() has side
> > effects. How about just naming the m68k inline function 'strlen'?
> 
> Having an inline function named "strlen" is not sufficient, as it needs an
> (exported) symbol named "strlen" at link time or module load time.

Really? MIPS and Xtensa both have an inline version of strcpy() and no
exported strcpy symbol. An I would naively assume, that if the compiler
sees a static inline definition of a function, then it will not generate
a function call. Although, the C standard does require the standard
library functions be available as external functions, so the compiler
would not be necessarily wrong :-/. Anyway, can you try if this
(untested) patch fixes the issue?

Thanks,
Michal

>From ac9861ba0bf4257f14d7f7db4b9eb60311986049 Mon Sep 17 00:00:00 2001
From: Michal Marek <mmarek@suse.cz>
Date: Wed, 10 Apr 2013 13:58:55 +0200
Subject: [PATCH] m68k: Do not define string functions as macros

Implement the functions under their real names, so that the compiler can
emit calls to them.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Michal Marek <mmarek@suse.cz>
---
 arch/m68k/include/asm/string.h |   22 ++++++++--------------
 arch/m68k/lib/Makefile         |    2 +-
 arch/m68k/lib/string.c         |   22 ----------------------
 3 files changed, 9 insertions(+), 37 deletions(-)
 delete mode 100644 arch/m68k/lib/string.c

diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
index 3219845..dcd80ae 100644
--- a/arch/m68k/include/asm/string.h
+++ b/arch/m68k/include/asm/string.h
@@ -4,7 +4,7 @@
 #include <linux/types.h>
 #include <linux/compiler.h>
 
-static inline size_t __kernel_strlen(const char *s)
+static inline size_t strlen(const char *s)
 {
 	const char *sc;
 
@@ -13,7 +13,7 @@ static inline size_t __kernel_strlen(const char *s)
 	return sc - s - 1;
 }
 
-static inline char *__kernel_strcpy(char *dest, const char *src)
+static inline char *strcpy(char *dest, const char *src)
 {
 	char *xdest = dest;
 
@@ -25,12 +25,10 @@ static inline char *__kernel_strcpy(char *dest, const char *src)
 	return xdest;
 }
 
-#ifndef __IN_STRING_C
-
 #define __HAVE_ARCH_STRLEN
 #define strlen(s)	(__builtin_constant_p(s) ?	\
 			 __builtin_strlen(s) :		\
-			 __kernel_strlen(s))
+			 strlen(s))
 
 #define __HAVE_ARCH_STRNLEN
 static inline size_t strnlen(const char *s, size_t count)
@@ -53,9 +51,7 @@ static inline size_t strnlen(const char *s, size_t count)
 #define strcpy(d, s)	(__builtin_constant_p(s) &&	\
 			 __builtin_strlen(s) <= 32 ?	\
 			 __builtin_strcpy(d, s) :	\
-			 __kernel_strcpy(d, s))
-#else
-#define strcpy(d, s)	__kernel_strcpy(d, s)
+			 strcpy(d, s))
 #endif
 
 #define __HAVE_ARCH_STRNCPY
@@ -76,10 +72,10 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
 }
 
 #define __HAVE_ARCH_STRCAT
-#define strcat(d, s)	({			\
-	char *__d = (d);			\
-	strcpy(__d + strlen(__d), (s));		\
-})
+static inline char * strcat(char *dest, const char *src)
+{
+	return strcpy(dest + strlen(dest), src);
+}
 
 #ifndef CONFIG_COLDFIRE
 #define __HAVE_ARCH_STRCMP
@@ -114,6 +110,4 @@ extern void *memset(void *, int, __kernel_size_t);
 extern void *memcpy(void *, const void *, __kernel_size_t);
 #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
 
-#endif
-
 #endif /* _M68K_STRING_H_ */
diff --git a/arch/m68k/lib/Makefile b/arch/m68k/lib/Makefile
index a9d782d..fcd8eb1 100644
--- a/arch/m68k/lib/Makefile
+++ b/arch/m68k/lib/Makefile
@@ -6,7 +6,7 @@
 lib-y	:= ashldi3.o ashrdi3.o lshrdi3.o muldi3.o \
 	   memcpy.o memset.o memmove.o
 
-lib-$(CONFIG_MMU) += string.o uaccess.o
+lib-$(CONFIG_MMU) += uaccess.o
 lib-$(CONFIG_CPU_HAS_NO_MULDIV64) += mulsi3.o divsi3.o udivsi3.o
 lib-$(CONFIG_CPU_HAS_NO_MULDIV64) += modsi3.o umodsi3.o
 
diff --git a/arch/m68k/lib/string.c b/arch/m68k/lib/string.c
deleted file mode 100644
index b9a57ab..0000000
--- a/arch/m68k/lib/string.c
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file COPYING in the main directory of this archive
- * for more details.
- */
-
-#define __IN_STRING_C
-
-#include <linux/module.h>
-#include <linux/string.h>
-
-char *strcpy(char *dest, const char *src)
-{
-	return __kernel_strcpy(dest, src);
-}
-EXPORT_SYMBOL(strcpy);
-
-char *strcat(char *dest, const char *src)
-{
-	return __kernel_strcpy(dest + __kernel_strlen(dest), src);
-}
-EXPORT_SYMBOL(strcat);
-- 
1.7.10.4

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Geert Uytterhoeven]

Hi Michal,

On Wed, Apr 10, 2013 at 2:26 PM, Michal Marek <mmarek@suse.cz> wrote:
> On Wed, Apr 10, 2013 at 12:59:44PM +0200, Geert Uytterhoeven wrote:
>> On Wed, Apr 10, 2013 at 12:18 PM, Michal Marek <mmarek@suse.cz> wrote:
>> > On 10.4.2013 11:24, Geert Uytterhoeven wrote:
>> > My understanding is, that with -fnobuiltin, the compiler is not allowed
>> > to make assumptions about functions if it does not see their definition,
>> > even if they resemble standard functions. E.g. on x86_64, strlen() is
>> > out-of-line, so gcc would have to assume, that strcmp() has side
>> > effects. How about just naming the m68k inline function 'strlen'?
>>
>> Having an inline function named "strlen" is not sufficient, as it needs an
>> (exported) symbol named "strlen" at link time or module load time.
>
> Really? MIPS and Xtensa both have an inline version of strcpy() and no
> exported strcpy symbol. An I would naively assume, that if the compiler
> sees a static inline definition of a function, then it will not generate
> a function call. Although, the C standard does require the standard
> library functions be available as external functions, so the compiler
> would not be necessarily wrong :-/. Anyway, can you try if this
> (untested) patch fixes the issue?

Thanks for the patch, but modpost still fails with

ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Michal Marek]

On Wed, Apr 10, 2013 at 04:19:14PM +0200, Geert Uytterhoeven wrote:
> Thanks for the patch, but modpost still fails with
> 
> ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!

OK OK, I am convinced. Then how about this? If you prefer speed over
size, the the other option is to add

size_t strlen(const char *s)
{
       return __kernel_strlen(s);
}
EXPORT_SYMBOL(strlen);

to arch/m68k/lib/string.c. On a related note, arch/m68k/lib/string.c is
built only in the CONFIG_MMU case, whereas asm/string.h is shared.

Michal


>From 1b89666d246bcc2bfc4c9e1bdfd1dbd1dafe2e2f Mon Sep 17 00:00:00 2001
From: Michal Marek <mmarek@suse.cz>
Date: Wed, 10 Apr 2013 16:45:21 +0200
Subject: [PATCH] m68k: Remove inline strlen() implementation

GCC can replace a strncat() call with constant second argument into a
strlen + store, which results in a link error:

ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!

The inline function is a simple for loop in C. Other architectures
either use an asm optimized variant, or use the generic function from
lib/string.c.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Michal Marek <mmarek@suse.cz>
---
 arch/m68k/include/asm/string.h |   14 --------------
 arch/m68k/lib/string.c         |    2 +-
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
index 3219845..9aea9f1 100644
--- a/arch/m68k/include/asm/string.h
+++ b/arch/m68k/include/asm/string.h
@@ -4,15 +4,6 @@
 #include <linux/types.h>
 #include <linux/compiler.h>
 
-static inline size_t __kernel_strlen(const char *s)
-{
-	const char *sc;
-
-	for (sc = s; *sc++; )
-		;
-	return sc - s - 1;
-}
-
 static inline char *__kernel_strcpy(char *dest, const char *src)
 {
 	char *xdest = dest;
@@ -27,11 +18,6 @@ static inline char *__kernel_strcpy(char *dest, const char *src)
 
 #ifndef __IN_STRING_C
 
-#define __HAVE_ARCH_STRLEN
-#define strlen(s)	(__builtin_constant_p(s) ?	\
-			 __builtin_strlen(s) :		\
-			 __kernel_strlen(s))
-
 #define __HAVE_ARCH_STRNLEN
 static inline size_t strnlen(const char *s, size_t count)
 {
diff --git a/arch/m68k/lib/string.c b/arch/m68k/lib/string.c
index b9a57ab..4d61fa8 100644
--- a/arch/m68k/lib/string.c
+++ b/arch/m68k/lib/string.c
@@ -17,6 +17,6 @@ EXPORT_SYMBOL(strcpy);
 
 char *strcat(char *dest, const char *src)
 {
-	return __kernel_strcpy(dest + __kernel_strlen(dest), src);
+	return __kernel_strcpy(dest + strlen(dest), src);
 }
 EXPORT_SYMBOL(strcat);
-- 
1.7.10.4

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Geert Uytterhoeven]

On Wed, Apr 10, 2013 at 5:22 PM, Michal Marek <mmarek@suse.cz> wrote:
> Subject: [PATCH] m68k: Remove inline strlen() implementation
>
> GCC can replace a strncat() call with constant second argument into a
> strlen + store, which results in a link error:
>
> ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!
>
> The inline function is a simple for loop in C. Other architectures
> either use an asm optimized variant, or use the generic function from
> lib/string.c.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Michal Marek <mmarek@suse.cz>

Thanks, this one works. Let me do some measurements...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Geert Uytterhoeven]

On Thu, Apr 11, 2013 at 8:04 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Apr 10, 2013 at 5:22 PM, Michal Marek <mmarek@suse.cz> wrote:
>> Subject: [PATCH] m68k: Remove inline strlen() implementation
>>
>> GCC can replace a strncat() call with constant second argument into a
>> strlen + store, which results in a link error:
>>
>> ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!
>>
>> The inline function is a simple for loop in C. Other architectures
>> either use an asm optimized variant, or use the generic function from
>> lib/string.c.
>>
>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Signed-off-by: Michal Marek <mmarek@suse.cz>
>
> Thanks, this one works. Let me do some measurements...

No visible impact on size for a typical build; the increase in .text
is offset by
the decrease in .data:

-      .text : 0x00001000 - 0x002aab64   (2727 KiB)
-      .data : 0x002ad938 - 0x00392148   ( 915 KiB)
+      .text : 0x00001000 - 0x002aac74   (2728 KiB)
+      .data : 0x002ada48 - 0x00392148   ( 914 KiB)

On Wed, Apr 10, 2013 at 5:22 PM, Michal Marek <mmarek@suse.cz> wrote:
> On Wed, Apr 10, 2013 at 04:19:14PM +0200, Geert Uytterhoeven wrote:
>> Thanks for the patch, but modpost still fails with
>>
>> ERROR: "strlen" [net/ipv4/ip_tunnel.ko] undefined!
>
> OK OK, I am convinced. Then how about this? If you prefer speed over
> size, the the other option is to add
>
> size_t strlen(const char *s)
> {
>        return __kernel_strlen(s);
> }
> EXPORT_SYMBOL(strlen);
>
> to arch/m68k/lib/string.c.

Interestingly, this also doesn't work. It turns out none of the
symbols defined in
this file (currently strcpy and strcat) end up in vmlinux.

As we've been stripping code from string.c, we've been left with just
a few symbols
that are typically never referenced, so the whole string.o in
arch/m68k/lib/lib.a
is not included in the final vmlinux.

So more stringectomy to do...

> On a related note, arch/m68k/lib/string.c is
> built only in the CONFIG_MMU case, whereas asm/string.h is shared.

So due to the above, this difference is mostly moot ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[Andi Kleen]

> My understanding is, that with -fnobuiltin, the compiler is not allowed
> to make assumptions about functions if it does not see their definition,
> even if they resemble standard functions. E.g. on x86_64, strlen() is
> out-of-line, so gcc would have to assume, that strcmp() has side
> effects. How about just naming the m68k inline function 'strlen'?

You should always supply an out of line fallback version with 
the standard name. The easiest way is to define the right
define so that lib/string.c does it.

-Andi

Re: [PATCH/RFC] m68k: Add -ffreestanding to KBUILD_CFLAGS

[David Miller]

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Wed, 10 Apr 2013 11:24:39 +0200

> Without -ffreestanding, gcc may replace calls to standard C library
> functions by calls to other standard C library functions and/or inline
> code.  This may cause link errors if the replacement code calls a standard
> C library function that's implemented as a macro in the kernel.

I think you should instead implement the library functions that gcc might
emit during a build.