Change in ...coreboot[master]: libpayload/libc: Use size_t for lengths and indices

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33794

Change subject: libpayload/libc: Use size_t for lengths and indices
......................................................................

libpayload/libc: Use size_t for lengths and indices

size_t is the natural integer type for strlen() and array indices, and
this fixes several integer conversion and sign comparison warnings.

Change-Id: I5658b19f990de4596a602b36d9533b1ca96ad947
Signed-off-by: Jacob Garber 
---
M payloads/libpayload/libc/string.c
1 file changed, 38 insertions(+), 40 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33794/1

diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c
index fd88a88..0e34a03 100644
--- a/payloads/libpayload/libc/string.c
+++ b/payloads/libpayload/libc/string.c
@@ -91,9 +91,9 @@
  */
 int strcasecmp(const char *s1, const char *s2)
 {
-	int i, res;
+	int res;

-	for (i = 0; 1; i++) {
+	for (size_t i = 0; 1; i++) {
    	res = tolower(s1[i]) - tolower(s2[i]);
    	if (res || (s1[i] == '\0'))
    		break;
@@ -112,10 +112,9 @@
  */
 int strncasecmp(const char *s1, const char *s2, size_t maxlen)
 {
-	int i, res;
+	int res = 0;

-	res = 0;
-	for (i = 0; i < maxlen; i++) {
+	for (size_t i = 0; i < maxlen; i++) {
    	res = tolower(s1[i]) - tolower(s2[i]);
    	if (res || (s1[i] == '\0'))
    		break;
@@ -135,9 +134,9 @@
  */
 int strcmp(const char *s1, const char *s2)
 {
-	int i, res;
+	int res;

-	for (i = 0; 1; i++) {
+	for (size_t i = 0; 1; i++) {
    	res = s1[i] - s2[i];
    	if (res || (s1[i] == '\0'))
    		break;
@@ -156,10 +155,9 @@
  */
 int strncmp(const char *s1, const char *s2, size_t maxlen)
 {
-	int i, res;
+	int res = 0;

-	res = 0;
-	for (i = 0; i < maxlen; i++) {
+	for (size_t i = 0; i < maxlen; i++) {
    	res = s1[i] - s2[i];
    	if (res || (s1[i] == '\0'))
    		break;
@@ -179,10 +177,9 @@
 char *strncpy(char *d, const char *s, size_t n)
 {
    /* Use +1 to get the NUL terminator. */
-	int max = n > strlen(s) + 1 ? strlen(s) + 1 : n;
-	int i;
+	size_t max = n > strlen(s) + 1 ? strlen(s) + 1 : n;

-	for (i = 0; i < max; i++)
+	for (size_t i = 0; i < max; i++)
    	d[i] = (char)s[i];

return d;
@@ -210,13 +207,12 @@
 char *strcat(char *d, const char *s)
 {
    char *p = d + strlen(d);
-	int sl = strlen(s);
-	int i;
+	size_t sl = strlen(s);

-	for (i = 0; i < sl; i++)
+	for (size_t i = 0; i < sl; i++)
    	p[i] = s[i];

-	p[i] = '\0';
+	p[sl] = '\0';
    return d;
 }

@@ -231,15 +227,13 @@
 char *strncat(char *d, const char *s, size_t n)
 {
    char *p = d + strlen(d);
-	int sl = strlen(s);
-	int max = n > sl ? sl : n;
-	// int max = n > strlen(s) ? strlen(s) : n;
-	int i;
+	size_t sl = strlen(s);
+	size_t max = n > sl ? sl : n;

-	for (i = 0; i < max; i++)
+	for (size_t i = 0; i < max; i++)
    	p[i] = s[i];

-	p[i] = '\0';
+	p[max] = '\0';
    return d;
 }

@@ -253,17 +247,19 @@
  */
 size_t strlcat(char *d, const char *s, size_t n)
 {
-	int sl = strlen(s);
-	int dl = strlen(d);
+	size_t sl = strlen(s);
+	size_t dl = strlen(d);
+
+	if (n <= dl + 1)
+		return sl + dl;

char *p = d + dl;
-	int max = n > (sl + dl) ? sl : (n - dl - 1);
-	int i;
+	size_t max = n > (sl + dl) ? sl : (n - dl - 1);

-	for (i = 0; i < max; i++)
+	for (size_t i = 0; i < max; i++)
    	p[i] = s[i];

-	p[i] = '\0';
+	p[max] = '\0';
    return sl + dl;
 }

@@ -316,7 +312,7 @@
  */
 char *strdup(const char *s)
 {
-	int n = strlen(s);
+	size_t n = strlen(s);
    char *p = malloc(n + 1);

if (p != NULL) {
@@ -336,11 +332,13 @@
  */
 char *strstr(const char *h, const char *n)
 {
-	int hn = strlen(h);
-	int nn = strlen(n);
-	int i;
+	size_t hn = strlen(h);
+	size_t nn = strlen(n);

-	for (i = 0; i <= hn - nn; i++)
+	if (hn < nn)
+		return NULL;
+
+	for (size_t i = 0; i <= hn - nn; i++)
    	if (!memcmp(&h[i], n, nn))
    		return (char *)&h[i];

@@ -532,11 +530,11 @@
  */
 size_t strspn(const char *s, const char *a)
 {
-	int i, j;
-	int al = strlen(a);
+	size_t i;
+	size_t al = strlen(a);
    for (i = 0; s[i] != 0; i++) {
    	int found = 0;
-		for (j = 0; j < al; j++) {
+		for (size_t j = 0; j < al; j++) {
    		if (s[i] == a[j]) {
    			found = 1;
    			break;
@@ -556,11 +554,11 @@
  */
 size_t strcspn(const char *s, const char *a)
 {
-	int i, j;
-	int al = strlen(a);
+	size_t i;
+	size_t al = strlen(a);
    for (i = 0; s[i] != 0; i++) {
    	int found = 0;
-		for (j = 0; j < al; j++) {
+		for (size_t j = 0; j < al; j++) {
    		if (s[i] == a[j]) {
    			found = 1;
    			break;

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33794 )

Change subject: libpayload/libc: Use size_t for lengths and indices
......................................................................

Patch Set 1: Code-Review+2

Change in coreboot[master]: libpayload/libc: Use size_t for lengths and indices

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33794 )

Change subject: libpayload/libc: Use size_t for lengths and indices
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG@10 
PS2, Line 10: this fixes several integer conversion and sign comparison warnings.
Do you know, if there is a size penalty for these changes?

Change in coreboot[master]: libpayload/libc: Use size_t for lengths and indices

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33794 )

Change subject: libpayload/libc: Use size_t for lengths and indices
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG@10 
PS2, Line 10: this fixes several integer conversion and sign comparison warnings.
> Do you know, if there is a size penalty for these changes?

Generally no - I tested compiling some of these functions in various architectures on godbolt.org, and the generated code is either identical or slightly smaller.

Change in coreboot[master]: libpayload/libc: Use size_t for lengths and indices

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33794 )

Change subject: libpayload/libc: Use size_t for lengths and indices
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG@10 
PS2, Line 10: this fixes several integer conversion and sign comparison warnings.
> Generally no - I tested compiling some of these functions in various architectures on godbolt. […]

Done

Change in coreboot[master]: libpayload/libc: Use size_t for lengths and indices

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33794 )

Change subject: libpayload/libc: Use size_t for lengths and indices
......................................................................

libpayload/libc: Use size_t for lengths and indices

size_t is the natural integer type for strlen() and array indices, and
this fixes several integer conversion and sign comparison warnings.

Change-Id: I5658b19f990de4596a602b36d9533b1ca96ad947
Signed-off-by: Jacob Garber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33794
Reviewed-by: Paul Menzel 
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M payloads/libpayload/libc/string.c
1 file changed, 38 insertions(+), 40 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Julius Werner: Looks good to me, approved

diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c
index fd88a88..0e34a03 100644
--- a/payloads/libpayload/libc/string.c
+++ b/payloads/libpayload/libc/string.c
@@ -91,9 +91,9 @@
  */
 int strcasecmp(const char *s1, const char *s2)
 {
-	int i, res;
+	int res;

-	for (i = 0; 1; i++) {
+	for (size_t i = 0; 1; i++) {
    	res = tolower(s1[i]) - tolower(s2[i]);
    	if (res || (s1[i] == '\0'))
    		break;
@@ -112,10 +112,9 @@
  */
 int strncasecmp(const char *s1, const char *s2, size_t maxlen)
 {
-	int i, res;
+	int res = 0;

-	res = 0;
-	for (i = 0; i < maxlen; i++) {
+	for (size_t i = 0; i < maxlen; i++) {
    	res = tolower(s1[i]) - tolower(s2[i]);
    	if (res || (s1[i] == '\0'))
    		break;
@@ -135,9 +134,9 @@
  */
 int strcmp(const char *s1, const char *s2)
 {
-	int i, res;
+	int res;

-	for (i = 0; 1; i++) {
+	for (size_t i = 0; 1; i++) {
    	res = s1[i] - s2[i];
    	if (res || (s1[i] == '\0'))
    		break;
@@ -156,10 +155,9 @@
  */
 int strncmp(const char *s1, const char *s2, size_t maxlen)
 {
-	int i, res;
+	int res = 0;

-	res = 0;
-	for (i = 0; i < maxlen; i++) {
+	for (size_t i = 0; i < maxlen; i++) {
    	res = s1[i] - s2[i];
    	if (res || (s1[i] == '\0'))
    		break;
@@ -179,10 +177,9 @@
 char *strncpy(char *d, const char *s, size_t n)
 {
    /* Use +1 to get the NUL terminator. */
-	int max = n > strlen(s) + 1 ? strlen(s) + 1 : n;
-	int i;
+	size_t max = n > strlen(s) + 1 ? strlen(s) + 1 : n;

-	for (i = 0; i < max; i++)
+	for (size_t i = 0; i < max; i++)
    	d[i] = (char)s[i];

return d;
@@ -210,13 +207,12 @@
 char *strcat(char *d, const char *s)
 {
    char *p = d + strlen(d);
-	int sl = strlen(s);
-	int i;
+	size_t sl = strlen(s);

-	for (i = 0; i < sl; i++)
+	for (size_t i = 0; i < sl; i++)
    	p[i] = s[i];

-	p[i] = '\0';
+	p[sl] = '\0';
    return d;
 }

@@ -231,15 +227,13 @@
 char *strncat(char *d, const char *s, size_t n)
 {
    char *p = d + strlen(d);
-	int sl = strlen(s);
-	int max = n > sl ? sl : n;
-	// int max = n > strlen(s) ? strlen(s) : n;
-	int i;
+	size_t sl = strlen(s);
+	size_t max = n > sl ? sl : n;

-	for (i = 0; i < max; i++)
+	for (size_t i = 0; i < max; i++)
    	p[i] = s[i];

-	p[i] = '\0';
+	p[max] = '\0';
    return d;
 }

@@ -253,17 +247,19 @@
  */
 size_t strlcat(char *d, const char *s, size_t n)
 {
-	int sl = strlen(s);
-	int dl = strlen(d);
+	size_t sl = strlen(s);
+	size_t dl = strlen(d);
+
+	if (n <= dl + 1)
+		return sl + dl;

char *p = d + dl;
-	int max = n > (sl + dl) ? sl : (n - dl - 1);
-	int i;
+	size_t max = n > (sl + dl) ? sl : (n - dl - 1);

-	for (i = 0; i < max; i++)
+	for (size_t i = 0; i < max; i++)
    	p[i] = s[i];

-	p[i] = '\0';
+	p[max] = '\0';
    return sl + dl;
 }

@@ -316,7 +312,7 @@
  */
 char *strdup(const char *s)
 {
-	int n = strlen(s);
+	size_t n = strlen(s);
    char *p = malloc(n + 1);

if (p != NULL) {
@@ -336,11 +332,13 @@
  */
 char *strstr(const char *h, const char *n)
 {
-	int hn = strlen(h);
-	int nn = strlen(n);
-	int i;
+	size_t hn = strlen(h);
+	size_t nn = strlen(n);

-	for (i = 0; i <= hn - nn; i++)
+	if (hn < nn)
+		return NULL;
+
+	for (size_t i = 0; i <= hn - nn; i++)
    	if (!memcmp(&h[i], n, nn))
    		return (char *)&h[i];

@@ -532,11 +530,11 @@
  */
 size_t strspn(const char *s, const char *a)
 {
-	int i, j;
-	int al = strlen(a);
+	size_t i;
+	size_t al = strlen(a);
    for (i = 0; s[i] != 0; i++) {
    	int found = 0;
-		for (j = 0; j < al; j++) {
+		for (size_t j = 0; j < al; j++) {
    		if (s[i] == a[j]) {
    			found = 1;
    			break;
@@ -556,11 +554,11 @@
  */
 size_t strcspn(const char *s, const char *a)
 {
-	int i, j;
-	int al = strlen(a);
+	size_t i;
+	size_t al = strlen(a);
    for (i = 0; s[i] != 0; i++) {
    	int found = 0;
-		for (j = 0; j < al; j++) {
+		for (size_t j = 0; j < al; j++) {
    		if (s[i] == a[j]) {
    			found = 1;
    			break;

Change in coreboot[master]: libpayload/libc: Use size_t for lengths and indices

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33794 )

Change subject: libpayload/libc: Use size_t for lengths and indices
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33794/2//COMMIT_MSG@10 
PS2, Line 10: this fixes several integer conversion and sign comparison warnings.
> Done

Nice, I would not have expected that.