* [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path @ 2021-03-18 18:07 Nikos Nikoleris 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul Nikos Nikoleris ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Nikos Nikoleris @ 2021-03-18 18:07 UTC (permalink / raw) To: kvm; +Cc: pbonzini, drjones, alexandru.elisei, andre.przywara, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-18 18:07 [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Nikos Nikoleris @ 2021-03-18 18:07 ` Nikos Nikoleris 2021-03-22 8:35 ` Andrew Jones 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 3/4] Makefile: Remove overriding recipe for libfdt_clean Nikos Nikoleris ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Nikos Nikoleris @ 2021-03-18 18:07 UTC (permalink / raw) To: kvm; +Cc: pbonzini, drjones, alexandru.elisei, andre.przywara, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul Nikos Nikoleris @ 2021-03-22 8:35 ` Andrew Jones 2021-03-22 9:52 ` Nikos Nikoleris 2021-03-23 12:14 ` Andrew Jones 0 siblings, 2 replies; 17+ messages in thread From: Andrew Jones @ 2021-03-22 8:35 UTC (permalink / raw) To: Nikos Nikoleris; +Cc: kvm, pbonzini, alexandru.elisei, andre.przywara 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-22 8:35 ` Andrew Jones @ 2021-03-22 9:52 ` Nikos Nikoleris 2021-03-22 10:09 ` Andrew Jones 2021-03-23 12:14 ` Andrew Jones 1 sibling, 1 reply; 17+ messages in thread From: Nikos Nikoleris @ 2021-03-22 9:52 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, pbonzini, alexandru.elisei, andre.przywara 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-22 9:52 ` Nikos Nikoleris @ 2021-03-22 10:09 ` Andrew Jones 0 siblings, 0 replies; 17+ messages in thread From: Andrew Jones @ 2021-03-22 10:09 UTC (permalink / raw) To: Nikos Nikoleris; +Cc: kvm, pbonzini, alexandru.elisei, andre.przywara 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-22 8:35 ` Andrew Jones 2021-03-22 9:52 ` Nikos Nikoleris @ 2021-03-23 12:14 ` Andrew Jones 2021-03-23 13:00 ` Andre Przywara 2021-03-23 13:01 ` Thomas Huth 1 sibling, 2 replies; 17+ messages in thread From: Andrew Jones @ 2021-03-23 12:14 UTC (permalink / raw) To: Nikos Nikoleris; +Cc: kvm, pbonzini, alexandru.elisei, andre.przywara, thuth 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-23 12:14 ` Andrew Jones @ 2021-03-23 13:00 ` Andre Przywara 2021-03-23 13:41 ` Andrew Jones 2021-03-23 13:01 ` Thomas Huth 1 sibling, 1 reply; 17+ messages in thread From: Andre Przywara @ 2021-03-23 13:00 UTC (permalink / raw) To: Andrew Jones; +Cc: Nikos Nikoleris, kvm, pbonzini, alexandru.elisei, thuth 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-23 13:00 ` Andre Przywara @ 2021-03-23 13:41 ` Andrew Jones 2021-03-23 16:11 ` Andre Przywara 0 siblings, 1 reply; 17+ messages in thread From: Andrew Jones @ 2021-03-23 13:41 UTC (permalink / raw) To: Andre Przywara; +Cc: Nikos Nikoleris, kvm, pbonzini, alexandru.elisei, thuth 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++; } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-23 13:41 ` Andrew Jones @ 2021-03-23 16:11 ` Andre Przywara 0 siblings, 0 replies; 17+ messages in thread From: Andre Przywara @ 2021-03-23 16:11 UTC (permalink / raw) To: Andrew Jones; +Cc: Nikos Nikoleris, kvm, pbonzini, alexandru.elisei, thuth 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++; > } > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-23 12:14 ` Andrew Jones 2021-03-23 13:00 ` Andre Przywara @ 2021-03-23 13:01 ` Thomas Huth 2021-03-23 13:31 ` Andrew Jones 1 sibling, 1 reply; 17+ messages in thread From: Thomas Huth @ 2021-03-23 13:01 UTC (permalink / raw) To: Andrew Jones, Nikos Nikoleris Cc: kvm, pbonzini, alexandru.elisei, andre.przywara 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul 2021-03-23 13:01 ` Thomas Huth @ 2021-03-23 13:31 ` Andrew Jones 0 siblings, 0 replies; 17+ messages in thread From: Andrew Jones @ 2021-03-23 13:31 UTC (permalink / raw) To: Thomas Huth Cc: Nikos Nikoleris, kvm, pbonzini, alexandru.elisei, andre.przywara 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [kvm-unit-tests PATCH v2 3/4] Makefile: Remove overriding recipe for libfdt_clean 2021-03-18 18:07 [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Nikos Nikoleris 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul Nikos Nikoleris @ 2021-03-18 18:07 ` Nikos Nikoleris 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 4/4] devicetree: Parse correctly the stdout-path Nikos Nikoleris 2021-03-22 8:53 ` [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Andrew Jones 3 siblings, 0 replies; 17+ messages in thread From: Nikos Nikoleris @ 2021-03-18 18:07 UTC (permalink / raw) To: kvm; +Cc: pbonzini, drjones, alexandru.elisei, andre.przywara, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [kvm-unit-tests PATCH v2 4/4] devicetree: Parse correctly the stdout-path 2021-03-18 18:07 [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Nikos Nikoleris 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul Nikos Nikoleris 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 3/4] Makefile: Remove overriding recipe for libfdt_clean Nikos Nikoleris @ 2021-03-18 18:07 ` Nikos Nikoleris 2021-03-22 8:53 ` [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Andrew Jones 3 siblings, 0 replies; 17+ messages in thread From: Nikos Nikoleris @ 2021-03-18 18:07 UTC (permalink / raw) To: kvm; +Cc: pbonzini, drjones, alexandru.elisei, andre.przywara, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path 2021-03-18 18:07 [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Nikos Nikoleris ` (2 preceding siblings ...) 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 4/4] devicetree: Parse correctly the stdout-path Nikos Nikoleris @ 2021-03-22 8:53 ` Andrew Jones 2021-03-22 9:55 ` Nikos Nikoleris 2021-03-22 18:04 ` Andre Przywara 3 siblings, 2 replies; 17+ messages in thread From: Andrew Jones @ 2021-03-22 8:53 UTC (permalink / raw) To: Nikos Nikoleris; +Cc: kvm, pbonzini, alexandru.elisei, andre.przywara 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path 2021-03-22 8:53 ` [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Andrew Jones @ 2021-03-22 9:55 ` Nikos Nikoleris 2021-03-22 18:04 ` Andre Przywara 1 sibling, 0 replies; 17+ messages in thread From: Nikos Nikoleris @ 2021-03-22 9:55 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, pbonzini, alexandru.elisei, andre.przywara 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path 2021-03-22 8:53 ` [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Andrew Jones 2021-03-22 9:55 ` Nikos Nikoleris @ 2021-03-22 18:04 ` Andre Przywara 2021-03-22 18:56 ` Andrew Jones 1 sibling, 1 reply; 17+ messages in thread From: Andre Przywara @ 2021-03-22 18:04 UTC (permalink / raw) To: Andrew Jones; +Cc: Nikos Nikoleris, kvm, pbonzini, alexandru.elisei 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path 2021-03-22 18:04 ` Andre Przywara @ 2021-03-22 18:56 ` Andrew Jones 0 siblings, 0 replies; 17+ messages in thread From: Andrew Jones @ 2021-03-22 18:56 UTC (permalink / raw) To: Andre Przywara; +Cc: Nikos Nikoleris, kvm, pbonzini, alexandru.elisei 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 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-03-23 16:12 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-18 18:07 [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Nikos Nikoleris 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul Nikos Nikoleris 2021-03-22 8:35 ` Andrew Jones 2021-03-22 9:52 ` Nikos Nikoleris 2021-03-22 10:09 ` Andrew Jones 2021-03-23 12:14 ` Andrew Jones 2021-03-23 13:00 ` Andre Przywara 2021-03-23 13:41 ` Andrew Jones 2021-03-23 16:11 ` Andre Przywara 2021-03-23 13:01 ` Thomas Huth 2021-03-23 13:31 ` Andrew Jones 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 3/4] Makefile: Remove overriding recipe for libfdt_clean Nikos Nikoleris 2021-03-18 18:07 ` [kvm-unit-tests PATCH v2 4/4] devicetree: Parse correctly the stdout-path Nikos Nikoleris 2021-03-22 8:53 ` [kvm-unit-tests PATCH v2 0/4] Fix the devicetree parser for stdout-path Andrew Jones 2021-03-22 9:55 ` Nikos Nikoleris 2021-03-22 18:04 ` Andre Przywara 2021-03-22 18:56 ` Andrew Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.