Skip to contentSkip to conversation

kstrtox: drop kstrtol()/kstrtoul() when possible

PATCH In review Alexey Dobriyan 3 participants 6 messages May 20, 2011

Subsystems (2)
Kbuild Library code
lore
Share & tools
lore

Alexey Dobriyan

author
#0 lore raw

PATCH 1/1 kstrtox: drop kstrtol()/kstrtoul() when possible

If "long" and "long long" types are identical at runtime, kstrtol() can be aliased to kstrtoll().

Unfortunately, one can't write

	#if sizeof(long) == sizeof(long long) ...

To solve this, generate header file with sizes and alignment of interesting types like we do with bounds.h and asm-offsets.h.

Everything above applies to "unsigned long" and kstrtoul().

Signed-off-by Alexey Dobriyan <[email protected]>

---

 Kbuild                 |   45 +++++++++++++++++++++++++++++++++++++++------
 include/linux/kernel.h |   27 ++++++++-------------------
 kernel/sizeof.c        |   10 ++++++++++
 lib/kstrtox.c          |   19 +++++++++++++------
 4 files changed, 70 insertions(+), 31 deletions(-)
Kbuild +39 -6
@@ -5,13 +5,16 @@
 # 2) Generate asm-offsets.h (may need bounds.h)
 # 3) Check for missing system calls
 
+always :=
+targets :=
+
 #####
-# 1) Generate bounds.h
+# Generate bounds.h
 
 bounds-file := include/generated/bounds.h
 
-always  := $(bounds-file)
-targets := $(bounds-file) kernel/bounds.s
+always  += $(bounds-file)
+targets += $(bounds-file) kernel/bounds.s
 
 quiet_cmd_bounds = GEN     $@
 define cmd_bounds

@@ -39,8 +42,38 @@ $(obj)/$(bounds-file): kernel/bounds.s Kbuild
 	$(Q)mkdir -p $(dir $@)
 	$(call cmd,bounds)
 
+# generated/sizeof.h
+sizeof-file := include/generated/sizeof.h
+
+always  += $(sizeof-file)
+targets += $(sizeof-file) kernel/sizeof.s
+
+quiet_cmd_sizeof = GEN     $@
+define cmd_sizeof
+	(	\
+	set -e;	\
+	echo "#ifndef _LINUX_SIZEOF_H";	\
+	echo "#define _LINUX_SIZEOF_H";	\
+	echo "/*";			\
+	echo " * This file is automatically generated from kernel/sizeof.c .";\
+	echo " * Modifying is futile.";	\
+	echo " */";			\
+	sed -ne $(sed-y) $<;		\
+	echo "#endif"			\
+	) >$@
+endef
+
+# We use internal kbuild rules to avoid the "is up to date" message from make
+kernel/sizeof.s: kernel/sizeof.c FORCE
+	$(Q)mkdir -p $(dir $@)
+	$(call if_changed_dep,cc_s_c)
+
+$(obj)/$(sizeof-file): kernel/sizeof.s Kbuild
+	$(Q)mkdir -p $(dir $@)
+	$(call cmd,sizeof)
+
 #####
-# 2) Generate asm-offsets.h
+# Generate asm-offsets.h
 #
 
 offsets-file := include/generated/asm-offsets.h

@@ -85,7 +118,7 @@ $(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s Kbuild
 	$(call cmd,offsets)
 
 #####
-# 3) Check for missing system calls
+# Check for missing system calls
 #
 
 quiet_cmd_syscalls = CALL    $<

@@ -96,4 +129,4 @@ missing-syscalls: scripts/checksyscalls.sh FORCE
 	$(call cmd,syscalls)
 
 # Keep these two files during make clean
-no-clean-files := $(bounds-file) $(offsets-file)
+no-clean-files := $(bounds-file) $(offsets-file) $(sizeof-file)
include/linux/kernel.h +8 -19
@@ -8,6 +8,7 @@
 #define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
 
 #ifdef __KERNEL__
+#include <generated/sizeof.h>
 
 #include <stdarg.h>
 #include <linux/linkage.h>

@@ -194,32 +195,20 @@ int __must_check _kstrtol(const char *s, unsigned int base, long *res);
 
 int __must_check kstrtoull(const char *s, unsigned int base, unsigned long long *res);
 int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
+#if SIZEOF_LONG == SIZEOF_LONG_LONG && ALIGNOF_LONG == ALIGNOF_LONG_LONG
 static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(unsigned long, unsigned long long) = 0.
-	 */
-	if (sizeof(unsigned long) == sizeof(unsigned long long) &&
-	    __alignof__(unsigned long) == __alignof__(unsigned long long))
-		return kstrtoull(s, base, (unsigned long long *)res);
-	else
-		return _kstrtoul(s, base, res);
+	return kstrtoull(s, base, (unsigned long long *)res);
 }
 
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
 {
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(long, long long) = 0.
-	 */
-	if (sizeof(long) == sizeof(long long) &&
-	    __alignof__(long) == __alignof__(long long))
-		return kstrtoll(s, base, (long long *)res);
-	else
-		return _kstrtol(s, base, res);
+	return kstrtoll(s, base, (long long *)res);
 }
-
+#else
+int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res);
+int __must_check kstrtol(const char *s, unsigned int base, long *res);
+#endif
 int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res);
 int __must_check kstrtoint(const char *s, unsigned int base, int *res);

new file mode 100644

kernel/sizeof.c new file +10 -0
@@ -0,0 +1,10 @@
+#include <linux/kbuild.h>
+
+void f(void)
+{
+	DEFINE(SIZEOF_LONG, sizeof(long));
+	DEFINE(ALIGNOF_LONG, __alignof__(long));
+
+	DEFINE(SIZEOF_LONG_LONG, sizeof(long long));
+	DEFINE(ALIGNOF_LONG_LONG, __alignof__(long long));
+}
lib/kstrtox.c +13 -6
@@ -11,6 +11,7 @@
  *
  * If -E is returned, result is not touched.
  */
+#include <generated/sizeof.h>
 #include <linux/ctype.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>

@@ -101,8 +102,14 @@ int kstrtoll(const char *s, unsigned int base, long long *res)
 }
 EXPORT_SYMBOL(kstrtoll);
 
-/* Internal, do not use. */
-int _kstrtoul(const char *s, unsigned int base, unsigned long *res)
+#if SIZEOF_LONG == SIZEOF_LONG_LONG && ALIGNOF_LONG == ALIGNOF_LONG_LONG
+/*
+ * Shortcut function call if types are indistinguishable at runtime.
+ * FYI, both __builtin_types_compatible_p(unsigned long, unsigned long long)
+ * and __builtin_types_compatible_p(long, long long) are always 0.
+ */
+#else
+int kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
 	unsigned long long tmp;
 	int rv;

@@ -115,10 +122,9 @@ int _kstrtoul(const char *s, unsigned int base, unsigned long *res)
 	*res = tmp;
 	return 0;
 }
-EXPORT_SYMBOL(_kstrtoul);
+EXPORT_SYMBOL(kstrtoul);
 
-/* Internal, do not use. */
-int _kstrtol(const char *s, unsigned int base, long *res)
+int kstrtol(const char *s, unsigned int base, long *res)
 {
 	long long tmp;
 	int rv;

@@ -131,7 +137,8 @@ int _kstrtol(const char *s, unsigned int base, long *res)
 	*res = tmp;
 	return 0;
 }
-EXPORT_SYMBOL(_kstrtol);
+EXPORT_SYMBOL(kstrtol);
+#endif
 
 int kstrtouint(const char *s, unsigned int base, unsigned int *res)
 {
#1 lore raw
Quoted 6 lines · Alexey Dobriyan

If "long" and "long long" types are identical at runtime, kstrtol() can be aliased to kstrtoll().

Unfortunately, one can't write

       #if sizeof(long) == sizeof(long long) ...

One can write #ifdef CONFIG_64BIT instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

author
#2 lore raw
Quoted 9 lines · Geert Uytterhoeven
Quoted 6 lines · Alexey Dobriyan

If "long" and "long long" types are identical at runtime, kstrtol() can be aliased to kstrtoll().

Unfortunately, one can't write

       #if sizeof(long) == sizeof(long long) ...

One can write #ifdef CONFIG_64BIT instead.

And alignment will match, on any arch, now and in future? I don't think so.

maintainer
#3 lore raw
Quoted 5 lines · On Fri, 20 May 2011 09:48:27 +0300 Alexey Dobriyan <[email protected]> wrote:
Quoted 4 lines · Geert Uytterhoeven
Quoted 3 lines · Alexey Dobriyan

If "long" and "long long" types are identical at runtime, kstrtol() can be aliased to kstrtoll().

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Unfortunately, one can't write

__ __ __ __#if sizeof(long) == sizeof(long long) ...

One can write #ifdef CONFIG_64BIT instead.

And alignment will match, on any arch, now and in future? I don't think so.

Don't worry about it.

z:/usr/src/linux-2.6.39> grep -r "#[ ]*if.*CONFIG_64BIT" . | wc -l
547

So much other stuff will break that kstrtofoo is a drop in the bucket.

author
#4 lore raw
Quoted 16 lines · Andrew Morton
Quoted 5 lines · On Fri, 20 May 2011 09:48:27 +0300 Alexey Dobriyan <[email protected]> wrote:
Quoted 4 lines · Geert Uytterhoeven
Quoted 3 lines · Alexey Dobriyan

If "long" and "long long" types are identical at runtime, kstrtol() can be aliased to kstrtoll().

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Unfortunately, one can't write

__ __ __ __#if sizeof(long) == sizeof(long long) ...

One can write #ifdef CONFIG_64BIT instead.

And alignment will match, on any arch, now and in future? I don't think so.

Quoted 6 lines

Don't worry about it.

z:/usr/src/linux-2.6.39> grep -r "#[ ]*if.*CONFIG_64BIT" . | wc -l
547

So much other stuff will break that kstrtofoo is a drop in the bucket.

Meh.
The point was that patch is obviously correct and will work in future. CONFIG_64BIT means many things and you guys ask me to overload CONFIG_64BIT one more time.

On X32, CONFIG_64BIT trick already doesn't technically work.

#5 lore raw
Quoted 22 lines · Alexey Dobriyan
Quoted 21 lines · Andrew Morton
Quoted 5 lines · On Fri, 20 May 2011 09:48:27 +0300 Alexey Dobriyan <[email protected]> wrote:
Quoted 4 lines · Geert Uytterhoeven
Quoted 3 lines · Alexey Dobriyan

If "long" and "long long" types are identical at runtime, kstrtol() can be aliased to kstrtoll().

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Unfortunately, one can't write

__ __ __ __#if sizeof(long) == sizeof(long long) ...

One can write #ifdef CONFIG_64BIT instead.

And alignment will match, on any arch, now and in future? I don't think so.

Don't worry about it.

z:/usr/src/linux-2.6.39> grep -r "#[    ]*if.*CONFIG_64BIT" . | wc -l 547

Quoted 6 lines
Quoted 1 line

So much other stuff will break that kstrtofoo is a drop in the bucket.

Meh.
The point was that patch is obviously correct and will work in future. CONFIG_64BIT means many things and you guys ask me to overload CONFIG_64BIT one more time.

In Linux, CONFIG_64BIT means that sizeof(long) == 8. Alignment of 8 byte longs and 8 byte long long should be identical, as w.r.t. the CPU they're the same type.

Quoted 1 line

On X32, CONFIG_64BIT trick already doesn't technically work.

Why? On X32, CONFIG_64bit is not set, and sizeof(long) == 4 and sizeof(long long) == 8.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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