Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32901
Change subject: string.h: Move common string functions into .c file ......................................................................
string.h: Move common string functions into .c file
There's no clear reason why most of coreboot's basic string functions are static inline. These functions don't particularly benefit from inlining (at least not notably more than other functions). This patch moves them to string.c to be more consistent with our usual coding practices.
Leaving the ctype functions as static inline because they actually seem small and collapsable enough that inlining seems reasonable.
Also clarified the situation of strdup() and strconcat() a bit more and optimized strrchr() to be single-pass.
Change-Id: I88166ba9876e94dfa3cfc06969c78a9e1bc6fc36 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/include/string.h M src/lib/Makefile.inc M src/lib/string.c 3 files changed, 127 insertions(+), 104 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/32901/1
diff --git a/src/include/string.h b/src/include/string.h index 65ce561..278d7c6 100644 --- a/src/include/string.h +++ b/src/include/string.h @@ -25,33 +25,13 @@ #endif char *strdup(const char *s); char *strconcat(const char *s1, const char *s2); - -// simple string functions - -static inline size_t strnlen(const char *src, size_t max) -{ - size_t i = 0; - while ((*src++) && (i < max)) - i++; - return i; -} - -static inline size_t strlen(const char *src) -{ - size_t i = 0; - while (*src++) - i++; - return i; -} - -static inline char *strchr(const char *s, int c) -{ - for (; *s; s++) { - if (*s == c) - return (char *) s; - } - return 0; -} +size_t strnlen(const char *src, size_t max); +size_t strlen(const char *src); +char *strchr(const char *s, int c); +char *strncpy(char *to, const char *from, int count); +char *strcpy(char *dst, const char *src); +int strcmp(const char *s1, const char *s2); +int strncmp(const char *s1, const char *s2, int maxlen);
/** * Find a character in a string. @@ -61,71 +41,12 @@ * @return A pointer to the last occurrence of the character in the * string, or NULL if the character was not encountered within the string. */ -static inline char *strrchr(const char *s, int c) -{ - char *p = (char *)s + strlen(s); +char *strrchr(const char *s, int c);
- for (; p >= s; p--) { - if (*p == c) - return p; - } - - return NULL; -} - -static inline char *strncpy(char *to, const char *from, int count) -{ - register char *ret = to; - register char data; - - while (count > 0) { - count--; - data = *from++; - *to++ = data; - if (data == '\0') - break; - } - - while (count > 0) { - count--; - *to++ = '\0'; - } - return ret; -} - -static inline char *strcpy(char *dst, const char *src) -{ - char *ptr = dst; - - while (*src) - *dst++ = *src++; - *dst = '\0'; - - return ptr; -} - -static inline int strcmp(const char *s1, const char *s2) -{ - int r; - - while ((r = (*s1 - *s2)) == 0 && *s1) { - s1++; - s2++; - } - return r; -} - -static inline int strncmp(const char *s1, const char *s2, int maxlen) -{ - int i; - - for (i = 0; i < maxlen; i++) { - if ((s1[i] != s2[i]) || (s1[i] == '\0')) - return s1[i] - s2[i]; - } - - return 0; -} +/* Parses an unsigned integer and moves the input pointer forward to the first + character that's not a valid digit. s and *s must not be NULL. Result + undefined if it overruns the return type size. */ +unsigned int skip_atoi(char **s);
static inline int isspace(int c) { @@ -179,16 +100,4 @@ return c; }
-/* Parses an unsigned integer and moves the input pointer forward to the first - character that's not a valid digit. s and *s must not be NULL. Result - undefined if it overruns the return type size. */ -static inline unsigned int skip_atoi(char **s) -{ - unsigned int i = 0; - - while (isdigit(**s)) - i = i*10 + *((*s)++) - '0'; - return i; -} - #endif /* STRING_H */ diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 1350152..913675b 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -118,7 +118,6 @@ ramstage-y += fmap.c ramstage-y += memchr.c ramstage-y += memcmp.c -ramstage-y += string.c ramstage-y += malloc.c smm-$(CONFIG_SMM_TSEG) += malloc.c ramstage-y += dimm_info_util.c @@ -244,6 +243,14 @@ ramstage-y += reset.c smm-y += reset.c
+decompressor-y += string.c +bootblock-y += string.c +verstage-y += string.c +romstage-y += string.c +postcar-y += string.c +ramstage-y += string.c +smm-y += string.c + postcar-y += bootmode.c postcar-y += boot_device.c postcar-y += cbfs.c diff --git a/src/lib/string.c b/src/lib/string.c index 2e71489f..f8ec9de 100644 --- a/src/lib/string.c +++ b/src/lib/string.c @@ -1,9 +1,14 @@ +#include <assert.h> +#include <rules.h> #include <string.h> #include <stddef.h> #include <stdlib.h>
char *strdup(const char *s) { + if (!ENV_RAMSTAGE) + dead_code(); /* This can't be used without malloc(). */ + size_t sz = strlen(s) + 1; char *d = malloc(sz); if (d) @@ -13,6 +18,9 @@
char *strconcat(const char *s1, const char *s2) { + if (!ENV_RAMSTAGE) + dead_code(); /* This can't be used without malloc(). */ + size_t sz_1 = strlen(s1); size_t sz_2 = strlen(s2); char *d = malloc(sz_1 + sz_2 + 1); @@ -22,3 +30,102 @@ } return d; } + +size_t strnlen(const char *src, size_t max) +{ + size_t i = 0; + while ((*src++) && (i < max)) + i++; + return i; +} + +size_t strlen(const char *src) +{ + size_t i = 0; + while (*src++) + i++; + return i; +} + +char *strchr(const char *s, int c) +{ + for (; *s; s++) { + if (*s == c) + return (char *)s; + } + return NULL; +} + +char *strrchr(const char *s, int c) +{ + char *p = NULL; + + for (; *s; s++) + if (*s == c) + p = (char *)s; + + return p; +} + +char *strncpy(char *to, const char *from, int count) +{ + register char *ret = to; + register char data; + + while (count > 0) { + count--; + data = *from++; + *to++ = data; + if (data == '\0') + break; + } + + while (count > 0) { + count--; + *to++ = '\0'; + } + return ret; +} + +char *strcpy(char *dst, const char *src) +{ + char *ptr = dst; + + while (*src) + *dst++ = *src++; + *dst = '\0'; + + return ptr; +} + +int strcmp(const char *s1, const char *s2) +{ + int r; + + while ((r = (*s1 - *s2)) == 0 && *s1) { + s1++; + s2++; + } + return r; +} + +int strncmp(const char *s1, const char *s2, int maxlen) +{ + int i; + + for (i = 0; i < maxlen; i++) { + if ((s1[i] != s2[i]) || (s1[i] == '\0')) + return s1[i] - s2[i]; + } + + return 0; +} + +unsigned int skip_atoi(char **s) +{ + unsigned int i = 0; + + while (isdigit(**s)) + i = i*10 + *((*s)++) - '0'; + return i; +}
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32901 )
Change subject: string.h: Move common string functions into .c file ......................................................................
Patch Set 1: Code-Review+1
Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32901 )
Change subject: string.h: Move common string functions into .c file ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG@16 PS1, Line 16: collapsable collapsible
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG@18 PS1, Line 18: Also clarified Also, clarified
https://writing.wisc.edu/handbook/grammarpunct/conjadv
https://review.coreboot.org/#/c/32901/1/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/#/c/32901/1/src/lib/string.c@50 PS1, Line 50: char *strchr(const char *s, int c) : { : for (; *s; s++) { : if (*s == c) : return (char *)s; : } : return NULL; : } : : char *strrchr(const char *s, int c) : { : char *p = NULL; : : for (; *s; s++) : if (*s == c) : p = (char *)s; : : return p; : } These two seem wrong. If `c` is 0, then pointer to the terminating zero must be returned, instead NULL would be returned.
https://review.coreboot.org/#/c/32901/1/src/lib/string.c@72 PS1, Line 72: register char *ret = to; : register char data; `register` is an anachronism, better just drop it
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32901 )
Change subject: string.h: Move common string functions into .c file ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG@16 PS1, Line 16: collapsable
collapsible
Done
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG@18 PS1, Line 18: Also clarified
Also, clarified […]
I disagree. The "also" is not conjunctive here. Instead, this sentence is a shortened form of "I also clarified ...", so there should be no comma.
https://review.coreboot.org/#/c/32901/1/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/#/c/32901/1/src/lib/string.c@50 PS1, Line 50: char *strchr(const char *s, int c) : { : for (; *s; s++) { : if (*s == c) : return (char *)s; : } : return NULL; : } : : char *strrchr(const char *s, int c) : { : char *p = NULL; : : for (; *s; s++) : if (*s == c) : p = (char *)s; : : return p; : }
These two seem wrong. […]
True, that's a bug in the old implementations already. Fixed.
https://review.coreboot.org/#/c/32901/1/src/lib/string.c@72 PS1, Line 72: register char *ret = to; : register char data;
`register` is an anachronism, better just drop it
Sure... I mean, I just moved the existing implementations around here. But I can take it out if you want.
Hello Alex Thiessen, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32901
to look at the new patch set (#2).
Change subject: string.h: Move common string functions into .c file ......................................................................
string.h: Move common string functions into .c file
There's no clear reason why most of coreboot's basic string functions are static inline. These functions don't particularly benefit from inlining (at least not notably more than other functions). This patch moves them to string.c to be more consistent with our usual coding practices.
Leaving the ctype functions as static inline because they actually seem small and collapsible enough that inlining seems reasonable.
Also clarified the situation of strdup() and strconcat() a bit more, optimized strrchr() to be single-pass, fixed a bug with using strchr() to find '\0' and got rid of unnecessary register keywords.
Change-Id: I88166ba9876e94dfa3cfc06969c78a9e1bc6fc36 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/include/string.h M src/lib/Makefile.inc M src/lib/string.c 3 files changed, 131 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/32901/2
Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32901 )
Change subject: string.h: Move common string functions into .c file ......................................................................
Patch Set 2: Code-Review+2
(4 comments)
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG@16 PS1, Line 16: collapsable
Done
Done
https://review.coreboot.org/#/c/32901/1//COMMIT_MSG@18 PS1, Line 18: Also clarified
I disagree. The "also" is not conjunctive here. […]
Done
https://review.coreboot.org/#/c/32901/1/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/#/c/32901/1/src/lib/string.c@50 PS1, Line 50: char *strchr(const char *s, int c) : { : for (; *s; s++) { : if (*s == c) : return (char *)s; : } : return NULL; : } : : char *strrchr(const char *s, int c) : { : char *p = NULL; : : for (; *s; s++) : if (*s == c) : p = (char *)s; : : return p; : }
True, that's a bug in the old implementations already. Fixed.
Done
https://review.coreboot.org/#/c/32901/1/src/lib/string.c@72 PS1, Line 72: register char *ret = to; : register char data;
Sure... I mean, I just moved the existing implementations around here. […]
Done
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32901 )
Change subject: string.h: Move common string functions into .c file ......................................................................
string.h: Move common string functions into .c file
There's no clear reason why most of coreboot's basic string functions are static inline. These functions don't particularly benefit from inlining (at least not notably more than other functions). This patch moves them to string.c to be more consistent with our usual coding practices.
Leaving the ctype functions as static inline because they actually seem small and collapsible enough that inlining seems reasonable.
Also clarified the situation of strdup() and strconcat() a bit more, optimized strrchr() to be single-pass, fixed a bug with using strchr() to find '\0' and got rid of unnecessary register keywords.
Change-Id: I88166ba9876e94dfa3cfc06969c78a9e1bc6fc36 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32901 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Alex Thiessen alex.thiessen.de+coreboot@gmail.com --- M src/include/string.h M src/lib/Makefile.inc M src/lib/string.c 3 files changed, 131 insertions(+), 106 deletions(-)
Approvals: build bot (Jenkins): Verified Alex Thiessen: Looks good to me, approved
diff --git a/src/include/string.h b/src/include/string.h index c56a760..d164f32 100644 --- a/src/include/string.h +++ b/src/include/string.h @@ -25,33 +25,13 @@ #endif char *strdup(const char *s); char *strconcat(const char *s1, const char *s2); - -// simple string functions - -static inline size_t strnlen(const char *src, size_t max) -{ - size_t i = 0; - while ((*src++) && (i < max)) - i++; - return i; -} - -static inline size_t strlen(const char *src) -{ - size_t i = 0; - while (*src++) - i++; - return i; -} - -static inline char *strchr(const char *s, int c) -{ - for (; *s; s++) { - if (*s == c) - return (char *) s; - } - return 0; -} +size_t strnlen(const char *src, size_t max); +size_t strlen(const char *src); +char *strchr(const char *s, int c); +char *strncpy(char *to, const char *from, int count); +char *strcpy(char *dst, const char *src); +int strcmp(const char *s1, const char *s2); +int strncmp(const char *s1, const char *s2, int maxlen);
/** * Find a character in a string. @@ -61,71 +41,14 @@ * @return A pointer to the last occurrence of the character in the * string, or NULL if the character was not encountered within the string. */ -static inline char *strrchr(const char *s, int c) -{ - char *p = (char *)s + strlen(s); +char *strrchr(const char *s, int c);
- for (; p >= s; p--) { - if (*p == c) - return p; - } - - return NULL; -} - -static inline char *strncpy(char *to, const char *from, int count) -{ - register char *ret = to; - register char data; - - while (count > 0) { - count--; - data = *from++; - *to++ = data; - if (data == '\0') - break; - } - - while (count > 0) { - count--; - *to++ = '\0'; - } - return ret; -} - -static inline char *strcpy(char *dst, const char *src) -{ - char *ptr = dst; - - while (*src) - *dst++ = *src++; - *dst = '\0'; - - return ptr; -} - -static inline int strcmp(const char *s1, const char *s2) -{ - int r; - - while ((r = (*s1 - *s2)) == 0 && *s1) { - s1++; - s2++; - } - return r; -} - -static inline int strncmp(const char *s1, const char *s2, int maxlen) -{ - int i; - - for (i = 0; i < maxlen; i++) { - if ((s1[i] != s2[i]) || (s1[i] == '\0')) - return s1[i] - s2[i]; - } - - return 0; -} +/* + * Parses an unsigned integer and moves the input pointer forward to the first + * character that's not a valid digit. s and *s must not be NULL. Result + * undefined if it overruns the return type size. + */ +unsigned int skip_atoi(char **s);
static inline int isspace(int c) { @@ -179,18 +102,4 @@ return c; }
-/* - * Parses an unsigned integer and moves the input pointer forward to the first - * character that's not a valid digit. s and *s must not be NULL. Result - * undefined if it overruns the return type size. - */ -static inline unsigned int skip_atoi(char **s) -{ - unsigned int i = 0; - - while (isdigit(**s)) - i = i*10 + *((*s)++) - '0'; - return i; -} - #endif /* STRING_H */ diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 1350152..913675b 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -118,7 +118,6 @@ ramstage-y += fmap.c ramstage-y += memchr.c ramstage-y += memcmp.c -ramstage-y += string.c ramstage-y += malloc.c smm-$(CONFIG_SMM_TSEG) += malloc.c ramstage-y += dimm_info_util.c @@ -244,6 +243,14 @@ ramstage-y += reset.c smm-y += reset.c
+decompressor-y += string.c +bootblock-y += string.c +verstage-y += string.c +romstage-y += string.c +postcar-y += string.c +ramstage-y += string.c +smm-y += string.c + postcar-y += bootmode.c postcar-y += boot_device.c postcar-y += cbfs.c diff --git a/src/lib/string.c b/src/lib/string.c index 2e71489f..a19f017 100644 --- a/src/lib/string.c +++ b/src/lib/string.c @@ -1,9 +1,14 @@ +#include <assert.h> +#include <rules.h> #include <string.h> #include <stddef.h> #include <stdlib.h>
char *strdup(const char *s) { + if (!ENV_RAMSTAGE) + dead_code(); /* This can't be used without malloc(). */ + size_t sz = strlen(s) + 1; char *d = malloc(sz); if (d) @@ -13,6 +18,9 @@
char *strconcat(const char *s1, const char *s2) { + if (!ENV_RAMSTAGE) + dead_code(); /* This can't be used without malloc(). */ + size_t sz_1 = strlen(s1); size_t sz_2 = strlen(s2); char *d = malloc(sz_1 + sz_2 + 1); @@ -22,3 +30,104 @@ } return d; } + +size_t strnlen(const char *src, size_t max) +{ + size_t i = 0; + while ((*src++) && (i < max)) + i++; + return i; +} + +size_t strlen(const char *src) +{ + size_t i = 0; + while (*src++) + i++; + return i; +} + +char *strchr(const char *s, int c) +{ + do { + if (*s == c) + return (char *)s; + } while (*s++); + + return NULL; +} + +char *strrchr(const char *s, int c) +{ + char *p = NULL; + + do { + if (*s == c) + p = (char *)s; + } while (*s++); + + return p; +} + +char *strncpy(char *to, const char *from, int count) +{ + char *ret = to; + char data; + + while (count > 0) { + count--; + data = *from++; + *to++ = data; + if (data == '\0') + break; + } + + while (count > 0) { + count--; + *to++ = '\0'; + } + return ret; +} + +char *strcpy(char *dst, const char *src) +{ + char *ptr = dst; + + while (*src) + *dst++ = *src++; + *dst = '\0'; + + return ptr; +} + +int strcmp(const char *s1, const char *s2) +{ + int r; + + while ((r = (*s1 - *s2)) == 0 && *s1) { + s1++; + s2++; + } + return r; +} + +int strncmp(const char *s1, const char *s2, int maxlen) +{ + int i; + + for (i = 0; i < maxlen; i++) { + if ((s1[i] != s2[i]) || (s1[i] == '\0')) + return s1[i] - s2[i]; + } + + return 0; +} + +unsigned int skip_atoi(char **s) +{ + unsigned int i = 0; + + while (isdigit(**s)) + i = i*10 + *((*s)++) - '0'; + return i; +}
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32901 )
Change subject: string.h: Move common string functions into .c file ......................................................................
Patch Set 3:
The generated line below has an invalid email address (domain part).
Reviewed-by: Alex Thiessen <alex.thiessen.de+coreboot@gmail.com+coreboot@gmail.com>
Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32901 )
Change subject: string.h: Move common string functions into .c file ......................................................................
Patch Set 3:
Patch Set 3:
The generated line below has an invalid email address (domain part).
Reviewed-by: Alex Thiessen <alex.thiessen.de+coreboot@gmail.com+coreboot@gmail.com>
That's some stupid email address parsing bug on Gerrit side. Don't know if it's coreboot-specific or a genuine Gerrit issue. A quick search in https://bugs.chromium.org/p/gerrit/issues did not return any existing ticket that covers this.
-
Alex Thiessen (Code Review) -
Julius Werner (Code Review)
-
Paul Menzel (Code Review)