[kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path

[kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path

[Nikos Nikoleris]

This set of patches fixes the way we parse the stdout-path
property in the DT. The stdout-path property is used to set up
the console. Prior to this, the code ignored the fact that
stdout-path is made of the path to the uart node as well as
parameters. As a result, it would fail to find the relevant DT
node. In addition to minor fixes in the device tree code, this
series pulls a new version of libfdt from upstream.

v1: https://lore.kernel.org/kvm/20210316152405.50363-1-nikos.nikoleris@arm.com/

Changes in v2:
  - Added strtoul and minor fix in strrchr
  - Fixes in libfdt_clean
  - Minor fix in lib/libfdt/README

Thanks,

Nikos

Nikos Nikoleris (4):
  lib/string: Add strnlen, strrchr and strtoul
  libfdt: Pull v1.6.0
  Makefile: Remove overriding recipe for libfdt_clean
  devicetree: Parse correctly the stdout-path

 lib/libfdt/README            |   5 +-
 Makefile                     |  16 +-
 arm/Makefile.common          |   2 +-
 lib/libfdt/Makefile.libfdt   |  10 +-
 powerpc/Makefile.common      |   2 +-
 lib/libfdt/version.lds       |  24 +-
 lib/libfdt/fdt.h             |  53 +--
 lib/libfdt/libfdt.h          | 766 +++++++++++++++++++++++++-----
 lib/libfdt/libfdt_env.h      | 109 ++---
 lib/libfdt/libfdt_internal.h | 206 +++++---
 lib/stdlib.h                 |  12 +
 lib/string.h                 |   5 +-
 lib/devicetree.c             |  15 +-
 lib/libfdt/fdt.c             | 200 +++++---
 lib/libfdt/fdt_addresses.c   | 101 ++++
 lib/libfdt/fdt_check.c       |  74 +++
 lib/libfdt/fdt_empty_tree.c  |  48 +-
 lib/libfdt/fdt_overlay.c     | 881 +++++++++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c          | 512 +++++++++++++++-----
 lib/libfdt/fdt_rw.c          | 231 +++++----
 lib/libfdt/fdt_strerror.c    |  53 +--
 lib/libfdt/fdt_sw.c          | 297 ++++++++----
 lib/libfdt/fdt_wip.c         |  90 ++--
 lib/string.c                 |  77 ++-
 24 files changed, 2948 insertions(+), 841 deletions(-)
 create mode 100644 lib/stdlib.h
 create mode 100644 lib/libfdt/fdt_addresses.c
 create mode 100644 lib/libfdt/fdt_check.c
 create mode 100644 lib/libfdt/fdt_overlay.c

-- 
2.25.1

[kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Nikos Nikoleris]

This change adds two functions from <string.h> and one from <stdlib.h>
in preparation for an update in libfdt.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/stdlib.h | 12 +++++++++
 lib/string.h |  4 ++-
 lib/string.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 77 insertions(+), 9 deletions(-)
 create mode 100644 lib/stdlib.h

diff --git a/lib/stdlib.h b/lib/stdlib.h
new file mode 100644
index 0000000..e8abe75
--- /dev/null
+++ b/lib/stdlib.h
@@ -0,0 +1,12 @@
+/*
+ * Header for libc stdlib functions
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#ifndef _STDLIB_H_
+#define _STDLIB_H_
+
+unsigned long int strtoul(const char *nptr, char **endptr, int base);
+
+#endif /* _STDLIB_H_ */
diff --git a/lib/string.h b/lib/string.h
index 493d51b..8da687e 100644
--- a/lib/string.h
+++ b/lib/string.h
@@ -7,12 +7,14 @@
 #ifndef __STRING_H
 #define __STRING_H
 
-extern unsigned long strlen(const char *buf);
+extern size_t strlen(const char *buf);
+extern size_t strnlen(const char *buf, size_t maxlen);
 extern char *strcat(char *dest, const char *src);
 extern char *strcpy(char *dest, const char *src);
 extern int strcmp(const char *a, const char *b);
 extern int strncmp(const char *a, const char *b, size_t n);
 extern char *strchr(const char *s, int c);
+extern char *strrchr(const char *s, int c);
 extern char *strstr(const char *haystack, const char *needle);
 extern void *memset(void *s, int c, size_t n);
 extern void *memcpy(void *dest, const void *src, size_t n);
diff --git a/lib/string.c b/lib/string.c
index 75257f5..f77881f 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -6,16 +6,26 @@
  */
 
 #include "libcflat.h"
+#include "stdlib.h"
 
-unsigned long strlen(const char *buf)
+size_t strlen(const char *buf)
 {
-    unsigned long len = 0;
+    size_t len = 0;
 
     while (*buf++)
 	++len;
     return len;
 }
 
+size_t strnlen(const char *buf, size_t maxlen)
+{
+    const char *sc;
+
+    for (sc = buf; maxlen-- && *sc != '\0'; ++sc)
+        /* nothing */;
+    return sc - buf;
+}
+
 char *strcat(char *dest, const char *src)
 {
     char *p = dest;
@@ -55,6 +65,16 @@ char *strchr(const char *s, int c)
     return (char *)s;
 }
 
+char *strrchr(const char *s, int c)
+{
+    const char *last = NULL;
+    do {
+        if (*s == (char)c)
+            last = s;
+    } while (*s++);
+    return (char *)last;
+}
+
 char *strstr(const char *s1, const char *s2)
 {
     size_t l1, l2;
@@ -135,13 +155,21 @@ void *memchr(const void *s, int c, size_t n)
     return NULL;
 }
 
-long atol(const char *ptr)
+static int isspace(int c)
+{
+    return c == ' ' || c == '\t' || c == '\f' || c == '\n' || c == '\r';
+}
+
+unsigned long int strtoul(const char *nptr, char **endptr, int base)
 {
     long acc = 0;
-    const char *s = ptr;
+    const char *s = nptr;
     int neg, c;
 
-    while (*s == ' ' || *s == '\t')
+    if (base < 0 || base == 1 || base > 32)
+        goto out; // errno = EINVAL
+
+    while (isspace(*s))
         s++;
     if (*s == '-'){
         neg = 1;
@@ -152,20 +180,46 @@ long atol(const char *ptr)
             s++;
     }
 
+    if (base == 0 || base == 16) {
+        if (*s == '0') {
+            s++;
+            if (*s == 'x') {
+                 s++;
+                 base = 16;
+            } else if (base == 0)
+                 base = 8;
+        } else if (base == 0)
+            base = 10;
+    }
+
     while (*s) {
-        if (*s < '0' || *s > '9')
+        if (*s >= '0' && *s < '0' + base && *s <= '9')
+            c = *s - '0';
+        else if (*s >= 'a' && *s < 'a' + base - 10)
+            c = *s - 'a' + 10;
+        else if (*s >= 'A' && *s < 'A' + base - 10)
+            c = *s - 'A' + 10;
+        else
             break;
-        c = *s - '0';
-        acc = acc * 10 + c;
+        acc = acc * base + c;
         s++;
     }
 
     if (neg)
         acc = -acc;
 
+ out:
+    if (endptr)
+        *endptr = (char *)s;
+
     return acc;
 }
 
+long atol(const char *ptr)
+{
+    return strtoul(ptr, NULL, 10);
+}
+
 extern char **environ;
 
 char *getenv(const char *name)
-- 
2.25.1

[kvm-unit-tests PATCH v2 3/4] Makefile: Remove overriding recipe for libfdt_clean

[Nikos Nikoleris]

libfdt_clean is now defined in the libfdt Makefile and as a result, we
don't need to redefine it in the top Makefile. In addition we define
some variables for the remaining libfdt_clean rule.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 Makefile                | 16 +++++++---------
 arm/Makefile.common     |  2 +-
 powerpc/Makefile.common |  2 +-
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index e0828fe..24b7917 100644
--- a/Makefile
+++ b/Makefile
@@ -40,8 +40,6 @@ cflatobjs := \
 LIBFDT_objdir = lib/libfdt
 LIBFDT_srcdir = $(SRCDIR)/lib/libfdt
 LIBFDT_archive = $(LIBFDT_objdir)/libfdt.a
-LIBFDT_include = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_INCLUDES))
-LIBFDT_version = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_VERSION))
 
 OBJDIRS += $(LIBFDT_objdir)
 
@@ -90,6 +88,10 @@ $(LIBFDT_archive): CFLAGS += -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/li
 $(LIBFDT_archive): $(addprefix $(LIBFDT_objdir)/,$(LIBFDT_OBJS))
 	$(AR) rcs $@ $^
 
+libfdt_clean: VECHO = echo " "
+libfdt_clean: STD_CLEANFILES = *.o .*.d
+libfdt_clean: LIBFDT_dir = $(LIBFDT_objdir)
+libfdt_clean: SHAREDLIB_EXT = so
 
 # Build directory target
 .PHONY: directories
@@ -110,15 +112,11 @@ install: standalone
 	mkdir -p $(DESTDIR)
 	install tests/* $(DESTDIR)
 
-clean: arch_clean
+clean: arch_clean libfdt_clean
+	$(RM) $(LIBFDT_archive)
 	$(RM) lib/.*.d $(libcflat) $(cflatobjs)
 
-libfdt_clean:
-	$(RM) $(LIBFDT_archive) \
-	$(addprefix $(LIBFDT_objdir)/,$(LIBFDT_OBJS)) \
-	$(LIBFDT_objdir)/.*.d
-
-distclean: clean libfdt_clean
+distclean: clean
 	$(RM) lib/asm lib/config.h config.mak $(TEST_DIR)-run msr.out cscope.* build-head
 	$(RM) -r tests logs logs.old
 
diff --git a/arm/Makefile.common b/arm/Makefile.common
index a123e85..55478ec 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -78,7 +78,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
 $(libeabi): $(eabiobjs)
 	$(AR) rcs $@ $^
 
-arm_clean: libfdt_clean asm_offsets_clean
+arm_clean: asm_offsets_clean
 	$(RM) $(TEST_DIR)/*.{o,flat,elf} $(libeabi) $(eabiobjs) \
 	      $(TEST_DIR)/.*.d lib/arm/.*.d
 
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index ac3cab6..4c3121a 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -73,7 +73,7 @@ $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
 	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
 	@chmod a-x $@
 
-powerpc_clean: libfdt_clean asm_offsets_clean
+powerpc_clean: asm_offsets_clean
 	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
 	      $(TEST_DIR)/.*.d lib/powerpc/.*.d
 
-- 
2.25.1

[kvm-unit-tests PATCH v2 4/4] devicetree: Parse correctly the stdout-path

[Nikos Nikoleris]

The stdout-path property in the device tree is a string of the form

"UART_NODE:OPTS"

Where UART_NODE is a compatible serial port and OPTS specify
parameters such as the baud. Fix the way we parse the node and, at
least for now, ignore options as we don't act on them.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/string.h     |  1 +
 lib/devicetree.c | 15 +++++++++------
 lib/string.c     |  7 +++++++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/lib/string.h b/lib/string.h
index 8da687e..e1febfe 100644
--- a/lib/string.h
+++ b/lib/string.h
@@ -15,6 +15,7 @@ extern int strcmp(const char *a, const char *b);
 extern int strncmp(const char *a, const char *b, size_t n);
 extern char *strchr(const char *s, int c);
 extern char *strrchr(const char *s, int c);
+extern char *strchrnul(const char *s, int c);
 extern char *strstr(const char *haystack, const char *needle);
 extern void *memset(void *s, int c, size_t n);
 extern void *memcpy(void *dest, const void *src, size_t n);
diff --git a/lib/devicetree.c b/lib/devicetree.c
index 1020324..409d18b 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -265,21 +265,24 @@ int dt_get_bootargs(const char **bootargs)
 
 int dt_get_default_console_node(void)
 {
-	const struct fdt_property *prop;
+	const char *p, *q;
 	int node, len;
 
 	node = fdt_path_offset(fdt, "/chosen");
 	if (node < 0)
 		return node;
 
-	prop = fdt_get_property(fdt, node, "stdout-path", &len);
-	if (!prop) {
-		prop = fdt_get_property(fdt, node, "linux,stdout-path", &len);
-		if (!prop)
+	p = fdt_getprop(fdt, node, "stdout-path", &len);
+	if (!p) {
+		p = fdt_getprop(fdt, node, "linux,stdout-path", &len);
+		if (!p)
 			return len;
 	}
 
-	return fdt_path_offset(fdt, prop->data);
+	q = strchrnul(p, ':');
+	len = q - p;
+
+	return fdt_path_offset_namelen(fdt, p, len);
 }
 
 int dt_get_initrd(const char **initrd, u32 *size)
diff --git a/lib/string.c b/lib/string.c
index f77881f..30592c5 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -75,6 +75,13 @@ char *strrchr(const char *s, int c)
     return (char *)last;
 }
 
+char *strchrnul(const char *s, int c)
+{
+    while (*s && *s != (char)c)
+        s++;
+    return (char *)s;
+}
+
 char *strstr(const char *s1, const char *s2)
 {
     size_t l1, l2;
-- 
2.25.1

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andrew Jones]

On Thu, Mar 18, 2021 at 06:07:24PM +0000, Nikos Nikoleris wrote:
> This change adds two functions from <string.h> and one from <stdlib.h>
> in preparation for an update in libfdt.
> 
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  lib/stdlib.h | 12 +++++++++
>  lib/string.h |  4 ++-
>  lib/string.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 77 insertions(+), 9 deletions(-)
>  create mode 100644 lib/stdlib.h
> 
> diff --git a/lib/stdlib.h b/lib/stdlib.h
> new file mode 100644
> index 0000000..e8abe75
> --- /dev/null
> +++ b/lib/stdlib.h
> @@ -0,0 +1,12 @@
> +/*
> + * Header for libc stdlib functions
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#ifndef _STDLIB_H_
> +#define _STDLIB_H_
> +
> +unsigned long int strtoul(const char *nptr, char **endptr, int base);
> +
> +#endif /* _STDLIB_H_ */
> diff --git a/lib/string.h b/lib/string.h
> index 493d51b..8da687e 100644
> --- a/lib/string.h
> +++ b/lib/string.h
> @@ -7,12 +7,14 @@
>  #ifndef __STRING_H
>  #define __STRING_H
>  
> -extern unsigned long strlen(const char *buf);
> +extern size_t strlen(const char *buf);
> +extern size_t strnlen(const char *buf, size_t maxlen);
>  extern char *strcat(char *dest, const char *src);
>  extern char *strcpy(char *dest, const char *src);
>  extern int strcmp(const char *a, const char *b);
>  extern int strncmp(const char *a, const char *b, size_t n);
>  extern char *strchr(const char *s, int c);
> +extern char *strrchr(const char *s, int c);
>  extern char *strstr(const char *haystack, const char *needle);
>  extern void *memset(void *s, int c, size_t n);
>  extern void *memcpy(void *dest, const void *src, size_t n);
> diff --git a/lib/string.c b/lib/string.c
> index 75257f5..f77881f 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -6,16 +6,26 @@
>   */
>  
>  #include "libcflat.h"
> +#include "stdlib.h"
>  
> -unsigned long strlen(const char *buf)
> +size_t strlen(const char *buf)
>  {
> -    unsigned long len = 0;
> +    size_t len = 0;
>  
>      while (*buf++)
>  	++len;
>      return len;
>  }
>  
> +size_t strnlen(const char *buf, size_t maxlen)
> +{
> +    const char *sc;
> +
> +    for (sc = buf; maxlen-- && *sc != '\0'; ++sc)
> +        /* nothing */;
> +    return sc - buf;
> +}
> +
>  char *strcat(char *dest, const char *src)
>  {
>      char *p = dest;
> @@ -55,6 +65,16 @@ char *strchr(const char *s, int c)
>      return (char *)s;
>  }
>  
> +char *strrchr(const char *s, int c)
> +{
> +    const char *last = NULL;
> +    do {
> +        if (*s == (char)c)
> +            last = s;
> +    } while (*s++);
> +    return (char *)last;
> +}
> +
>  char *strstr(const char *s1, const char *s2)
>  {
>      size_t l1, l2;
> @@ -135,13 +155,21 @@ void *memchr(const void *s, int c, size_t n)
>      return NULL;
>  }
>  
> -long atol(const char *ptr)
> +static int isspace(int c)
> +{
> +    return c == ' ' || c == '\t' || c == '\f' || c == '\n' || c == '\r';

I added \v. We don't need to do it for this patch, but at some point we
should consider adding a ctype.h file and consolidating all these is*
functions. There's a few in lib/argv.c too.

> +}
> +
> +unsigned long int strtoul(const char *nptr, char **endptr, int base)
>  {
>      long acc = 0;
> -    const char *s = ptr;
> +    const char *s = nptr;
>      int neg, c;
>  
> -    while (*s == ' ' || *s == '\t')
> +    if (base < 0 || base == 1 || base > 32)
> +        goto out; // errno = EINVAL

I changed this to

 assert(base == 0 || (base >= 2 && base <= 36));

Any reason why you weren't allowing bases 33 - 36?

> +
> +    while (isspace(*s))
>          s++;
>      if (*s == '-'){
>          neg = 1;
> @@ -152,20 +180,46 @@ long atol(const char *ptr)
>              s++;
>      }
>  
> +    if (base == 0 || base == 16) {
> +        if (*s == '0') {
> +            s++;
> +            if (*s == 'x') {

I changed this to (*s == 'x' || *s == 'X')

> +                 s++;
> +                 base = 16;
> +            } else if (base == 0)
> +                 base = 8;
> +        } else if (base == 0)
> +            base = 10;
> +    }
> +
>      while (*s) {
> -        if (*s < '0' || *s > '9')
> +        if (*s >= '0' && *s < '0' + base && *s <= '9')
> +            c = *s - '0';
> +        else if (*s >= 'a' && *s < 'a' + base - 10)
> +            c = *s - 'a' + 10;
> +        else if (*s >= 'A' && *s < 'A' + base - 10)
> +            c = *s - 'A' + 10;
> +        else
>              break;
> -        c = *s - '0';
> -        acc = acc * 10 + c;
> +        acc = acc * base + c;

I changed this to catch overflow.

>          s++;
>      }
>  
>      if (neg)
>          acc = -acc;
>  
> + out:
> +    if (endptr)
> +        *endptr = (char *)s;
> +
>      return acc;
>  }
>  
> +long atol(const char *ptr)
> +{
> +    return strtoul(ptr, NULL, 10);

Since atol should be strtol, I went ahead and also added strtol.

> +}
> +
>  extern char **environ;
>  
>  char *getenv(const char *name)
> -- 
> 2.25.1
> 

Here's a diff of my changes on top of your patch


diff --git a/lib/string.c b/lib/string.c
index 30592c5603c5..b684271bb18f 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -164,21 +164,22 @@ void *memchr(const void *s, int c, size_t n)
 
 static int isspace(int c)
 {
-    return c == ' ' || c == '\t' || c == '\f' || c == '\n' || c == '\r';
+    return c == ' ' || c == '\t' || c == '\r' || c == '\n' || c == '\v' || c == '\f';
 }
 
-unsigned long int strtoul(const char *nptr, char **endptr, int base)
-{
-    long acc = 0;
+static unsigned long __strtol(const char *nptr, char **endptr,
+                              int base, bool is_signed) {
+    unsigned long acc = 0;
     const char *s = nptr;
+    bool overflow;
     int neg, c;
 
-    if (base < 0 || base == 1 || base > 32)
-        goto out; // errno = EINVAL
+    assert(base == 0 || (base >= 2 && base <= 36));
 
     while (isspace(*s))
         s++;
-    if (*s == '-'){
+
+    if (*s == '-') {
         neg = 1;
         s++;
     } else {
@@ -190,7 +191,7 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
     if (base == 0 || base == 16) {
         if (*s == '0') {
             s++;
-            if (*s == 'x') {
+            if (*s == 'x' || *s == 'X') {
                  s++;
                  base = 16;
             } else if (base == 0)
@@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
             c = *s - 'A' + 10;
         else
             break;
-        acc = acc * base + c;
+
+        if (is_signed) {
+            long __acc = (long)acc;
+            overflow = __builtin_smull_overflow(__acc, base, &__acc);
+            assert(!overflow);
+            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
+            assert(!overflow);
+            acc = (unsigned long)__acc;
+        } else {
+            overflow = __builtin_umull_overflow(acc, base, &acc);
+            assert(!overflow);
+            overflow = __builtin_uaddl_overflow(acc, c, &acc);
+            assert(!overflow);
+        }
+
         s++;
     }
 
     if (neg)
         acc = -acc;
 
- out:
     if (endptr)
         *endptr = (char *)s;
 
     return acc;
 }
 
+long int strtol(const char *nptr, char **endptr, int base)
+{
+    return __strtol(nptr, endptr, base, true);
+}
+
+unsigned long int strtoul(const char *nptr, char **endptr, int base)
+{
+    return __strtol(nptr, endptr, base, false);
+}
+
 long atol(const char *ptr)
 {
-    return strtoul(ptr, NULL, 10);
+    return strtol(ptr, NULL, 10);
 }
 
 extern char **environ;


Thanks,
drew

Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path

[Andrew Jones]

On Thu, Mar 18, 2021 at 06:07:23PM +0000, Nikos Nikoleris wrote:
> This set of patches fixes the way we parse the stdout-path
> property in the DT. The stdout-path property is used to set up
> the console. Prior to this, the code ignored the fact that
> stdout-path is made of the path to the uart node as well as
> parameters. As a result, it would fail to find the relevant DT
> node. In addition to minor fixes in the device tree code, this
> series pulls a new version of libfdt from upstream.
> 
> v1: https://lore.kernel.org/kvm/20210316152405.50363-1-nikos.nikoleris@arm.com/
> 
> Changes in v2:
>   - Added strtoul and minor fix in strrchr
>   - Fixes in libfdt_clean
>   - Minor fix in lib/libfdt/README
> 
> Thanks,
> 
> Nikos
>

Applied to arm/queue

https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue

Thanks,
drew 

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Nikos Nikoleris]

Hi Drew,

On 22/03/2021 08:35, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 06:07:24PM +0000, Nikos Nikoleris wrote:
>> This change adds two functions from <string.h> and one from <stdlib.h>
>> in preparation for an update in libfdt.
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>>   lib/stdlib.h | 12 +++++++++
>>   lib/string.h |  4 ++-
>>   lib/string.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++------
>>   3 files changed, 77 insertions(+), 9 deletions(-)
>>   create mode 100644 lib/stdlib.h
>>
>> diff --git a/lib/stdlib.h b/lib/stdlib.h
>> new file mode 100644
>> index 0000000..e8abe75
>> --- /dev/null
>> +++ b/lib/stdlib.h
>> @@ -0,0 +1,12 @@
>> +/*
>> + * Header for libc stdlib functions
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Library General Public License version 2.
>> + */
>> +#ifndef _STDLIB_H_
>> +#define _STDLIB_H_
>> +
>> +unsigned long int strtoul(const char *nptr, char **endptr, int base);
>> +
>> +#endif /* _STDLIB_H_ */
>> diff --git a/lib/string.h b/lib/string.h
>> index 493d51b..8da687e 100644
>> --- a/lib/string.h
>> +++ b/lib/string.h
>> @@ -7,12 +7,14 @@
>>   #ifndef __STRING_H
>>   #define __STRING_H
>>   
>> -extern unsigned long strlen(const char *buf);
>> +extern size_t strlen(const char *buf);
>> +extern size_t strnlen(const char *buf, size_t maxlen);
>>   extern char *strcat(char *dest, const char *src);
>>   extern char *strcpy(char *dest, const char *src);
>>   extern int strcmp(const char *a, const char *b);
>>   extern int strncmp(const char *a, const char *b, size_t n);
>>   extern char *strchr(const char *s, int c);
>> +extern char *strrchr(const char *s, int c);
>>   extern char *strstr(const char *haystack, const char *needle);
>>   extern void *memset(void *s, int c, size_t n);
>>   extern void *memcpy(void *dest, const void *src, size_t n);
>> diff --git a/lib/string.c b/lib/string.c
>> index 75257f5..f77881f 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -6,16 +6,26 @@
>>    */
>>   
>>   #include "libcflat.h"
>> +#include "stdlib.h"
>>   
>> -unsigned long strlen(const char *buf)
>> +size_t strlen(const char *buf)
>>   {
>> -    unsigned long len = 0;
>> +    size_t len = 0;
>>   
>>       while (*buf++)
>>   	++len;
>>       return len;
>>   }
>>   
>> +size_t strnlen(const char *buf, size_t maxlen)
>> +{
>> +    const char *sc;
>> +
>> +    for (sc = buf; maxlen-- && *sc != '\0'; ++sc)
>> +        /* nothing */;
>> +    return sc - buf;
>> +}
>> +
>>   char *strcat(char *dest, const char *src)
>>   {
>>       char *p = dest;
>> @@ -55,6 +65,16 @@ char *strchr(const char *s, int c)
>>       return (char *)s;
>>   }
>>   
>> +char *strrchr(const char *s, int c)
>> +{
>> +    const char *last = NULL;
>> +    do {
>> +        if (*s == (char)c)
>> +            last = s;
>> +    } while (*s++);
>> +    return (char *)last;
>> +}
>> +
>>   char *strstr(const char *s1, const char *s2)
>>   {
>>       size_t l1, l2;
>> @@ -135,13 +155,21 @@ void *memchr(const void *s, int c, size_t n)
>>       return NULL;
>>   }
>>   
>> -long atol(const char *ptr)
>> +static int isspace(int c)
>> +{
>> +    return c == ' ' || c == '\t' || c == '\f' || c == '\n' || c == '\r';
> 
> I added \v. We don't need to do it for this patch, but at some point we
> should consider adding a ctype.h file and consolidating all these is*
> functions. There's a few in lib/argv.c too.
> 

I agree.

>> +}
>> +
>> +unsigned long int strtoul(const char *nptr, char **endptr, int base)
>>   {
>>       long acc = 0;
>> -    const char *s = ptr;
>> +    const char *s = nptr;
>>       int neg, c;
>>   
>> -    while (*s == ' ' || *s == '\t')
>> +    if (base < 0 || base == 1 || base > 32)
>> +        goto out; // errno = EINVAL
> 
> I changed this to
> 
>   assert(base == 0 || (base >= 2 && base <= 36));
> 
> Any reason why you weren't allowing bases 33 - 36?
> 

I was going through the manpage for strtoul and I got confused. 36 is 
the right value.

I wasn't sure if we should assert, the manpage seems to imply that it 
will return without converting and set the errno and endptr. I guess it 
might be better to assert().

>> +
>> +    while (isspace(*s))
>>           s++;
>>       if (*s == '-'){
>>           neg = 1;
>> @@ -152,20 +180,46 @@ long atol(const char *ptr)
>>               s++;
>>       }
>>   
>> +    if (base == 0 || base == 16) {
>> +        if (*s == '0') {
>> +            s++;
>> +            if (*s == 'x') {
> 
> I changed this to (*s == 'x' || *s == 'X')
> 

Here my intent was to not parse 0X as a valid prefix for base 16, 0X is 
not in the manpage.

>> +                 s++;
>> +                 base = 16;
>> +            } else if (base == 0)
>> +                 base = 8;
>> +        } else if (base == 0)
>> +            base = 10;
>> +    }
>> +
>>       while (*s) {
>> -        if (*s < '0' || *s > '9')
>> +        if (*s >= '0' && *s < '0' + base && *s <= '9')
>> +            c = *s - '0';
>> +        else if (*s >= 'a' && *s < 'a' + base - 10)
>> +            c = *s - 'a' + 10;
>> +        else if (*s >= 'A' && *s < 'A' + base - 10)
>> +            c = *s - 'A' + 10;
>> +        else
>>               break;
>> -        c = *s - '0';
>> -        acc = acc * 10 + c;
>> +        acc = acc * base + c;
> 
> I changed this to catch overflow.
> 

Thanks! Some thoughts on the assertion.

>>           s++;
>>       }
>>   
>>       if (neg)
>>           acc = -acc;
>>   
>> + out:
>> +    if (endptr)
>> +        *endptr = (char *)s;
>> +
>>       return acc;
>>   }
>>   
>> +long atol(const char *ptr)
>> +{
>> +    return strtoul(ptr, NULL, 10);
> 
> Since atol should be strtol, I went ahead and also added strtol.
>

Not very important but we could also add it to stdlib.h?


Thanks for the fixes it looks much better now!

Nikos


>> +}
>> +
>>   extern char **environ;
>>   
>>   char *getenv(const char *name)
>> -- 
>> 2.25.1
>>
> 
> Here's a diff of my changes on top of your patch
> 
> 
> diff --git a/lib/string.c b/lib/string.c
> index 30592c5603c5..b684271bb18f 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -164,21 +164,22 @@ void *memchr(const void *s, int c, size_t n)
>   
>   static int isspace(int c)
>   {
> -    return c == ' ' || c == '\t' || c == '\f' || c == '\n' || c == '\r';
> +    return c == ' ' || c == '\t' || c == '\r' || c == '\n' || c == '\v' || c == '\f';
>   }
>   
> -unsigned long int strtoul(const char *nptr, char **endptr, int base)
> -{
> -    long acc = 0;
> +static unsigned long __strtol(const char *nptr, char **endptr,
> +                              int base, bool is_signed) {
> +    unsigned long acc = 0;
>       const char *s = nptr;
> +    bool overflow;
>       int neg, c;
>   
> -    if (base < 0 || base == 1 || base > 32)
> -        goto out; // errno = EINVAL
> +    assert(base == 0 || (base >= 2 && base <= 36));
>   
>       while (isspace(*s))
>           s++;
> -    if (*s == '-'){
> +
> +    if (*s == '-') {
>           neg = 1;
>           s++;
>       } else {
> @@ -190,7 +191,7 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
>       if (base == 0 || base == 16) {
>           if (*s == '0') {
>               s++;
> -            if (*s == 'x') {
> +            if (*s == 'x' || *s == 'X') {
>                    s++;
>                    base = 16;
>               } else if (base == 0)
> @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
>               c = *s - 'A' + 10;
>           else
>               break;
> -        acc = acc * base + c;
> +
> +        if (is_signed) {
> +            long __acc = (long)acc;
> +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> +            assert(!overflow);
> +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> +            assert(!overflow);
> +            acc = (unsigned long)__acc;
> +        } else {
> +            overflow = __builtin_umull_overflow(acc, base, &acc);
> +            assert(!overflow);
> +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> +            assert(!overflow);
> +        }
> +
>           s++;
>       }
>   
>       if (neg)
>           acc = -acc;
>   
> - out:
>       if (endptr)
>           *endptr = (char *)s;
>   
>       return acc;
>   }
>   
> +long int strtol(const char *nptr, char **endptr, int base)
> +{
> +    return __strtol(nptr, endptr, base, true);
> +}
> +
> +unsigned long int strtoul(const char *nptr, char **endptr, int base)
> +{
> +    return __strtol(nptr, endptr, base, false);
> +}
> +
>   long atol(const char *ptr)
>   {
> -    return strtoul(ptr, NULL, 10);
> +    return strtol(ptr, NULL, 10);
>   }
>   
>   extern char **environ;
> 
> 
> Thanks,
> drew
> 

Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path

[Nikos Nikoleris]

On 22/03/2021 08:53, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 06:07:23PM +0000, Nikos Nikoleris wrote:
>> This set of patches fixes the way we parse the stdout-path
>> property in the DT. The stdout-path property is used to set up
>> the console. Prior to this, the code ignored the fact that
>> stdout-path is made of the path to the uart node as well as
>> parameters. As a result, it would fail to find the relevant DT
>> node. In addition to minor fixes in the device tree code, this
>> series pulls a new version of libfdt from upstream.
>>
>> v1: https://lore.kernel.org/kvm/20210316152405.50363-1-nikos.nikoleris@arm.com/
>>
>> Changes in v2:
>>    - Added strtoul and minor fix in strrchr
>>    - Fixes in libfdt_clean
>>    - Minor fix in lib/libfdt/README
>>
>> Thanks,
>>
>> Nikos
>>
>
> Applied to arm/queue
>
> https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue
>
> Thanks,
> drew
>

Thanks for the reviews!

Nikos
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andrew Jones]

On Mon, Mar 22, 2021 at 09:52:43AM +0000, Nikos Nikoleris wrote:
> > > +unsigned long int strtoul(const char *nptr, char **endptr, int base)
> > >   {
> > >       long acc = 0;
> > > -    const char *s = ptr;
> > > +    const char *s = nptr;
> > >       int neg, c;
> > > -    while (*s == ' ' || *s == '\t')
> > > +    if (base < 0 || base == 1 || base > 32)
> > > +        goto out; // errno = EINVAL
> > 
> > I changed this to
> > 
> >   assert(base == 0 || (base >= 2 && base <= 36));
> > 
> > Any reason why you weren't allowing bases 33 - 36?
> > 
> 
> I was going through the manpage for strtoul and I got confused. 36 is the
> right value.
> 
> I wasn't sure if we should assert, the manpage seems to imply that it will
> return without converting and set the errno and endptr. I guess it might be
> better to assert().

Yeah, I think so for our little test framework. Anything that would result
in EINVAL means fix your test code. assert should help find those things
more quickly.

> 
> > > +
> > > +    while (isspace(*s))
> > >           s++;
> > >       if (*s == '-'){
> > >           neg = 1;
> > > @@ -152,20 +180,46 @@ long atol(const char *ptr)
> > >               s++;
> > >       }
> > > +    if (base == 0 || base == 16) {
> > > +        if (*s == '0') {
> > > +            s++;
> > > +            if (*s == 'x') {
> > 
> > I changed this to (*s == 'x' || *s == 'X')
> > 
> 
> Here my intent was to not parse 0X as a valid prefix for base 16, 0X is not
> in the manpage.

It's a manpage bug. strtol's manpage does specify it and libc's strtoul
allows it.

> > > +long atol(const char *ptr)
> > > +{
> > > +    return strtoul(ptr, NULL, 10);
> > 
> > Since atol should be strtol, I went ahead and also added strtol.
> > 
> 
> Not very important but we could also add it to stdlib.h?

Yeah, atol should go to stdlib, but I think that's another cleanup patch
we can do later. I'd actually like to kill libcflat and start using
standard headers for everything. At least for starters we could create
all the standard headers we need and move all the prototypes out of
libcflat but keep libcflat as a header full of #includes.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path

[Andre Przywara]

On Mon, 22 Mar 2021 09:53:36 +0100
Andrew Jones <drjones@redhat.com> wrote:

> On Thu, Mar 18, 2021 at 06:07:23PM +0000, Nikos Nikoleris wrote:
> > This set of patches fixes the way we parse the stdout-path
> > property in the DT. The stdout-path property is used to set up
> > the console. Prior to this, the code ignored the fact that
> > stdout-path is made of the path to the uart node as well as
> > parameters. As a result, it would fail to find the relevant DT
> > node. In addition to minor fixes in the device tree code, this
> > series pulls a new version of libfdt from upstream.
> > 
> > v1: https://lore.kernel.org/kvm/20210316152405.50363-1-nikos.nikoleris@arm.com/
> > 
> > Changes in v2:
> >   - Added strtoul and minor fix in strrchr
> >   - Fixes in libfdt_clean
> >   - Minor fix in lib/libfdt/README
> > 
> > Thanks,
> > 
> > Nikos
> >  
> 
> Applied to arm/queue

So I understand that this is a bit late now, but is this really the way
forward: to just implement libc functions as we go, from scratch, and
merge them without any real testing?
I understand that hacking up strchr() is fun, but when it comes to
those string parsing functions, it gets a bit hairy, and I feel like we
are dismissing decades of experience here by implementing stuff from
scratch. At the very least we should run some unit tests (!) on newly
introduced C library functions?

Or probably the better alternative: we pick some existing C library,
and start to borrow implementations from there? Is klibc[1] a good
choice, maybe?

Cheers,
Andre

[1] https://git.kernel.org/pub/scm/libs/klibc/klibc.git/


> 
> https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue
> 
> Thanks,
> drew 
> 

Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path

[Andrew Jones]

On Mon, Mar 22, 2021 at 06:04:45PM +0000, Andre Przywara wrote:
> On Mon, 22 Mar 2021 09:53:36 +0100
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Thu, Mar 18, 2021 at 06:07:23PM +0000, Nikos Nikoleris wrote:
> > > This set of patches fixes the way we parse the stdout-path
> > > property in the DT. The stdout-path property is used to set up
> > > the console. Prior to this, the code ignored the fact that
> > > stdout-path is made of the path to the uart node as well as
> > > parameters. As a result, it would fail to find the relevant DT
> > > node. In addition to minor fixes in the device tree code, this
> > > series pulls a new version of libfdt from upstream.
> > > 
> > > v1: https://lore.kernel.org/kvm/20210316152405.50363-1-nikos.nikoleris@arm.com/
> > > 
> > > Changes in v2:
> > >   - Added strtoul and minor fix in strrchr
> > >   - Fixes in libfdt_clean
> > >   - Minor fix in lib/libfdt/README
> > > 
> > > Thanks,
> > > 
> > > Nikos
> > >  
> > 
> > Applied to arm/queue
> 
> So I understand that this is a bit late now, but is this really the way
> forward: to just implement libc functions as we go, from scratch, and
> merge them without any real testing?
> I understand that hacking up strchr() is fun, but when it comes to
> those string parsing functions, it gets a bit hairy, and I feel like we
> are dismissing decades of experience here by implementing stuff from
> scratch. At the very least we should run some unit tests (!) on newly
> introduced C library functions?

Who says I didn't test the new string functions? Did you come up with a
test case that breaks something?

> 
> Or probably the better alternative: we pick some existing C library,
> and start to borrow implementations from there? Is klibc[1] a good
> choice, maybe?

The trivial functions like strchr don't really scare me much. And the
more complicated functions don't always adapt to our framework.
I just looked at klibc's strtol. It doesn't have any error handling;
not the errno type specified in the man page and not the type we do in
kvm-unit-tests (asserts). IOW, our implementation is even more complete.

Anyway, after like 15 years of development, kvm-unit-tests only has
20 string, 3 stdlib, and 4 printf functions. I'm not too worried
about overly reinventing the wheel just yet :-)

Thanks,
drew


> 
> Cheers,
> Andre
> 
> [1] https://git.kernel.org/pub/scm/libs/klibc/klibc.git/
> 
> 
> > 
> > https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue
> > 
> > Thanks,
> > drew 
> > 
> 

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andrew Jones]

On Mon, Mar 22, 2021 at 09:35:23AM +0100, Andrew Jones wrote:
> @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
>              c = *s - 'A' + 10;
>          else
>              break;
> -        acc = acc * base + c;
> +
> +        if (is_signed) {
> +            long __acc = (long)acc;
> +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> +            assert(!overflow);
> +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> +            assert(!overflow);
> +            acc = (unsigned long)__acc;
> +        } else {
> +            overflow = __builtin_umull_overflow(acc, base, &acc);
> +            assert(!overflow);
> +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> +            assert(!overflow);
> +        }
> +

Unfortunately my use of these builtins isn't loved by older compilers,
like the one used by the build-centos7 pipeline in our gitlab CI. I
could wrap them in an #if GCC_VERSION >= 50100 and just have the old
'acc = acc * base + c' as the fallback, but that's not pretty and
would also mean that clang would use the fallback too. Maybe we can
try and make our compiler.h more fancy in order to provide a
COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define like linux does for
both gcc and clang. Or, we could just forgot the overflow checking.

Anybody else have suggestions? Paolo? Thomas?

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andre Przywara]

On Tue, 23 Mar 2021 13:14:15 +0100
Andrew Jones <drjones@redhat.com> wrote:

Hi,

> On Mon, Mar 22, 2021 at 09:35:23AM +0100, Andrew Jones wrote:
> > @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
> >              c = *s - 'A' + 10;
> >          else
> >              break;
> > -        acc = acc * base + c;
> > +
> > +        if (is_signed) {
> > +            long __acc = (long)acc;
> > +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> > +            assert(!overflow);
> > +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> > +            assert(!overflow);
> > +            acc = (unsigned long)__acc;
> > +        } else {
> > +            overflow = __builtin_umull_overflow(acc, base, &acc);
> > +            assert(!overflow);
> > +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> > +            assert(!overflow);
> > +        }
> > +  
> 
> Unfortunately my use of these builtins isn't loved by older compilers,
> like the one used by the build-centos7 pipeline in our gitlab CI. I
> could wrap them in an #if GCC_VERSION >= 50100 and just have the old
> 'acc = acc * base + c' as the fallback, but that's not pretty and
> would also mean that clang would use the fallback too. Maybe we can
> try and make our compiler.h more fancy in order to provide a
> COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define like linux does for
> both gcc and clang. Or, we could just forgot the overflow checking.

In line with my email from yesterday:
Before we go down the path of all evil (premature optimisation!), can't
we just copy
https://git.kernel.org/pub/scm/libs/klibc/klibc.git/tree/usr/klibc/strntoumax.c
and have a tested version that works everywhere? This is BSD/GPL dual
licensed, IIUC.
I don't really see the reason to performance optimise strtol in the
context of kvm-unit-tests.

Cheers,
Andre

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Thomas Huth]

On 23/03/2021 13.14, Andrew Jones wrote:
> On Mon, Mar 22, 2021 at 09:35:23AM +0100, Andrew Jones wrote:
>> @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
>>               c = *s - 'A' + 10;
>>           else
>>               break;
>> -        acc = acc * base + c;
>> +
>> +        if (is_signed) {
>> +            long __acc = (long)acc;
>> +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
>> +            assert(!overflow);
>> +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
>> +            assert(!overflow);
>> +            acc = (unsigned long)__acc;
>> +        } else {
>> +            overflow = __builtin_umull_overflow(acc, base, &acc);
>> +            assert(!overflow);
>> +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
>> +            assert(!overflow);
>> +        }
>> +
> 
> Unfortunately my use of these builtins isn't loved by older compilers,
> like the one used by the build-centos7 pipeline in our gitlab CI. I
> could wrap them in an #if GCC_VERSION >= 50100 and just have the old
> 'acc = acc * base + c' as the fallback, but that's not pretty and
> would also mean that clang would use the fallback too. Maybe we can
> try and make our compiler.h more fancy in order to provide a
> COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define like linux does for
> both gcc and clang. Or, we could just forgot the overflow checking.
> 
> Anybody else have suggestions? Paolo? Thomas?

What does a "normal" libc implementation do (e.g. glibc)? If it is also not 
doing overflow checking, I think we also don't need it in the kvm-unit-tests.

  Thomas

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andrew Jones]

On Tue, Mar 23, 2021 at 02:01:47PM +0100, Thomas Huth wrote:
> On 23/03/2021 13.14, Andrew Jones wrote:
> > On Mon, Mar 22, 2021 at 09:35:23AM +0100, Andrew Jones wrote:
> > > @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
> > >               c = *s - 'A' + 10;
> > >           else
> > >               break;
> > > -        acc = acc * base + c;
> > > +
> > > +        if (is_signed) {
> > > +            long __acc = (long)acc;
> > > +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> > > +            assert(!overflow);
> > > +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> > > +            assert(!overflow);
> > > +            acc = (unsigned long)__acc;
> > > +        } else {
> > > +            overflow = __builtin_umull_overflow(acc, base, &acc);
> > > +            assert(!overflow);
> > > +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> > > +            assert(!overflow);
> > > +        }
> > > +
> > 
> > Unfortunately my use of these builtins isn't loved by older compilers,
> > like the one used by the build-centos7 pipeline in our gitlab CI. I
> > could wrap them in an #if GCC_VERSION >= 50100 and just have the old
> > 'acc = acc * base + c' as the fallback, but that's not pretty and
> > would also mean that clang would use the fallback too. Maybe we can
> > try and make our compiler.h more fancy in order to provide a
> > COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define like linux does for
> > both gcc and clang. Or, we could just forgot the overflow checking.
> > 
> > Anybody else have suggestions? Paolo? Thomas?
> 
> What does a "normal" libc implementation do (e.g. glibc)? If it is also not
> doing overflow checking, I think we also don't need it in the
> kvm-unit-tests.
>

You'll get LONG_MAX for strtol and ULONG_MAX for strtoul and errno will be
set to ERANGE.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andrew Jones]

On Tue, Mar 23, 2021 at 01:00:01PM +0000, Andre Przywara wrote:
> On Tue, 23 Mar 2021 13:14:15 +0100
> Andrew Jones <drjones@redhat.com> wrote:
> 
> Hi,
> 
> > On Mon, Mar 22, 2021 at 09:35:23AM +0100, Andrew Jones wrote:
> > > @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
> > >              c = *s - 'A' + 10;
> > >          else
> > >              break;
> > > -        acc = acc * base + c;
> > > +
> > > +        if (is_signed) {
> > > +            long __acc = (long)acc;
> > > +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> > > +            assert(!overflow);
> > > +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> > > +            assert(!overflow);
> > > +            acc = (unsigned long)__acc;
> > > +        } else {
> > > +            overflow = __builtin_umull_overflow(acc, base, &acc);
> > > +            assert(!overflow);
> > > +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> > > +            assert(!overflow);
> > > +        }
> > > +  
> > 
> > Unfortunately my use of these builtins isn't loved by older compilers,
> > like the one used by the build-centos7 pipeline in our gitlab CI. I
> > could wrap them in an #if GCC_VERSION >= 50100 and just have the old
> > 'acc = acc * base + c' as the fallback, but that's not pretty and
> > would also mean that clang would use the fallback too. Maybe we can
> > try and make our compiler.h more fancy in order to provide a
> > COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define like linux does for
> > both gcc and clang. Or, we could just forgot the overflow checking.
> 
> In line with my email from yesterday:
> Before we go down the path of all evil (premature optimisation!), can't
> we just copy
> https://git.kernel.org/pub/scm/libs/klibc/klibc.git/tree/usr/klibc/strntoumax.c
> and have a tested version that works everywhere? This is BSD/GPL dual
> licensed, IIUC.
> I don't really see the reason to performance optimise strtol in the
> context of kvm-unit-tests.
>

Using the builtin isn't to optimize, it's to simplify. Checking for
overflow on multiplication is ugly business. As I said yesterday,
klibc doesn't do any error checking. We could choose to go that
way too, but I'd prefer we give a best effort to making the test
framework robust.

I quick pulled together the diff below. This gives us the overflow
checking when not using old compilers, but just falls back to the
simple math otherwise. Unless people have strong opinions about
that, then I'm inclined to go with it.

Thanks,
drew


diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h
index 2d72f18c36e5..311da9807932 100644
--- a/lib/linux/compiler.h
+++ b/lib/linux/compiler.h
@@ -8,6 +8,20 @@
 
 #ifndef __ASSEMBLY__
 
+#define GCC_VERSION (__GNUC__ * 10000           \
+                    + __GNUC_MINOR__ * 100     \
+                    + __GNUC_PATCHLEVEL__)
+
+#ifdef __clang__
+#if __has_builtin(__builtin_mul_overflow) && \
+    __has_builtin(__builtin_add_overflow) && \
+    __has_builtin(__builtin_sub_overflow)
+#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
+#endif
+#elif GCC_VERSION >= 50100
+#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
+#endif
+
 #include <stdint.h>
 
 #define barrier()      asm volatile("" : : : "memory")
diff --git a/lib/string.c b/lib/string.c
index b684271bb18f..e323908fe24e 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -7,6 +7,7 @@
 
 #include "libcflat.h"
 #include "stdlib.h"
+#include "linux/compiler.h"
 
 size_t strlen(const char *buf)
 {
@@ -171,7 +172,6 @@ static unsigned long __strtol(const char *nptr, char **endptr,
                               int base, bool is_signed) {
     unsigned long acc = 0;
     const char *s = nptr;
-    bool overflow;
     int neg, c;
 
     assert(base == 0 || (base >= 2 && base <= 36));
@@ -210,19 +210,23 @@ static unsigned long __strtol(const char *nptr, char **endptr,
         else
             break;
 
+#ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
         if (is_signed) {
             long __acc = (long)acc;
-            overflow = __builtin_smull_overflow(__acc, base, &__acc);
+            bool overflow = __builtin_smull_overflow(__acc, base, &__acc);
             assert(!overflow);
             overflow = __builtin_saddl_overflow(__acc, c, &__acc);
             assert(!overflow);
             acc = (unsigned long)__acc;
         } else {
-            overflow = __builtin_umull_overflow(acc, base, &acc);
+            bool overflow = __builtin_umull_overflow(acc, base, &acc);
             assert(!overflow);
             overflow = __builtin_uaddl_overflow(acc, c, &acc);
             assert(!overflow);
         }
+#else
+        acc = acc * base + c;
+#endif
 
         s++;
     }

Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andre Przywara]

On Tue, 23 Mar 2021 14:41:21 +0100
Andrew Jones <drjones@redhat.com> wrote:

Hi,

> On Tue, Mar 23, 2021 at 01:00:01PM +0000, Andre Przywara wrote:
> > On Tue, 23 Mar 2021 13:14:15 +0100
> > Andrew Jones <drjones@redhat.com> wrote:
> > 
> > Hi,
> >   
> > > On Mon, Mar 22, 2021 at 09:35:23AM +0100, Andrew Jones wrote:  
> > > > @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
> > > >              c = *s - 'A' + 10;
> > > >          else
> > > >              break;
> > > > -        acc = acc * base + c;
> > > > +
> > > > +        if (is_signed) {
> > > > +            long __acc = (long)acc;
> > > > +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> > > > +            assert(!overflow);
> > > > +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> > > > +            assert(!overflow);
> > > > +            acc = (unsigned long)__acc;
> > > > +        } else {
> > > > +            overflow = __builtin_umull_overflow(acc, base, &acc);
> > > > +            assert(!overflow);
> > > > +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> > > > +            assert(!overflow);
> > > > +        }
> > > > +    
> > > 
> > > Unfortunately my use of these builtins isn't loved by older compilers,
> > > like the one used by the build-centos7 pipeline in our gitlab CI. I
> > > could wrap them in an #if GCC_VERSION >= 50100 and just have the old
> > > 'acc = acc * base + c' as the fallback, but that's not pretty and
> > > would also mean that clang would use the fallback too. Maybe we can
> > > try and make our compiler.h more fancy in order to provide a
> > > COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define like linux does for
> > > both gcc and clang. Or, we could just forgot the overflow checking.  
> > 
> > In line with my email from yesterday:
> > Before we go down the path of all evil (premature optimisation!), can't
> > we just copy
> > https://git.kernel.org/pub/scm/libs/klibc/klibc.git/tree/usr/klibc/strntoumax.c
> > and have a tested version that works everywhere? This is BSD/GPL dual
> > licensed, IIUC.
> > I don't really see the reason to performance optimise strtol in the
> > context of kvm-unit-tests.
> >  
> 
> Using the builtin isn't to optimize, it's to simplify. Checking for
> overflow on multiplication is ugly business. As I said yesterday,
> klibc doesn't do any error checking.

Argh, sorry, I missed your reply yesterday in a bunch of other emails!

> We could choose to go that
> way too, but I'd prefer we give a best effort to making the test
> framework robust.

I agree, klibc was just some example, I didn't look too closely into
it. If it lacks, we should indeed not use it.

I just felt we are going through all the special cases of those
functions again, when people elsewhere checked all of them already. I
had some unpleasant experience with implementing a seemingly simple
memcpy() last year, with some surprising corner cases, so grew a bit
wary about re-implementing standard stuff and hoping it's all good.

Cheers,
Andre

> I quick pulled together the diff below. This gives us the overflow
> checking when not using old compilers, but just falls back to the
> simple math otherwise. Unless people have strong opinions about
> that, then I'm inclined to go with it.
> 
> Thanks,
> drew
> 
> 
> diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h
> index 2d72f18c36e5..311da9807932 100644
> --- a/lib/linux/compiler.h
> +++ b/lib/linux/compiler.h
> @@ -8,6 +8,20 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#define GCC_VERSION (__GNUC__ * 10000           \
> +                    + __GNUC_MINOR__ * 100     \
> +                    + __GNUC_PATCHLEVEL__)
> +
> +#ifdef __clang__
> +#if __has_builtin(__builtin_mul_overflow) && \
> +    __has_builtin(__builtin_add_overflow) && \
> +    __has_builtin(__builtin_sub_overflow)
> +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> +#endif
> +#elif GCC_VERSION >= 50100
> +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> +#endif
> +
>  #include <stdint.h>
>  
>  #define barrier()      asm volatile("" : : : "memory")
> diff --git a/lib/string.c b/lib/string.c
> index b684271bb18f..e323908fe24e 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -7,6 +7,7 @@
>  
>  #include "libcflat.h"
>  #include "stdlib.h"
> +#include "linux/compiler.h"
>  
>  size_t strlen(const char *buf)
>  {
> @@ -171,7 +172,6 @@ static unsigned long __strtol(const char *nptr, char **endptr,
>                                int base, bool is_signed) {
>      unsigned long acc = 0;
>      const char *s = nptr;
> -    bool overflow;
>      int neg, c;
>  
>      assert(base == 0 || (base >= 2 && base <= 36));
> @@ -210,19 +210,23 @@ static unsigned long __strtol(const char *nptr, char **endptr,
>          else
>              break;
>  
> +#ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
>          if (is_signed) {
>              long __acc = (long)acc;
> -            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> +            bool overflow = __builtin_smull_overflow(__acc, base, &__acc);
>              assert(!overflow);
>              overflow = __builtin_saddl_overflow(__acc, c, &__acc);
>              assert(!overflow);
>              acc = (unsigned long)__acc;
>          } else {
> -            overflow = __builtin_umull_overflow(acc, base, &acc);
> +            bool overflow = __builtin_umull_overflow(acc, base, &acc);
>              assert(!overflow);
>              overflow = __builtin_uaddl_overflow(acc, c, &acc);
>              assert(!overflow);
>          }
> +#else
> +        acc = acc * base + c;
> +#endif
>  
>          s++;
>      }
>