[PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

[PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

[Andre Przywara]

Those are the six remaining patches of the initial post to fix the
C comparison warnings.
I reworked the fixes according to David's comments, and took quite a
different approach for some of them.
Changelog below.

The series is against https://github.com/dgibson/dtc/commits/main
------------------------------------

When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
some warnings about the signedness of the operands not matching:
=================
libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
   if ((absoffset < offset)
.....
=================

This does not occur under normal conditions in the dtc repo, but might
show up when libfdt is embedded in another project. There have been reports
from U-Boot and Trusted-Firmware-A.

The underlying issue is mostly due to C's promotion behaviour (ANSI C
section 6.1.3.8) when dealing with operands of different signedness
(but same size): Signed values get implictly casted to unsigned, which
is not typically what we want if they could have been negative.

The Internet(TM) suggests that blindly applying casts is probably doing
more harm than it helps, so this series tries to fix the underlying
issues properly.
In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
some signed types are due to them being returned along wih error values in
other functions (node offsets).
So these fixes here have been based on the following assumptions:
- We cannot change the prototype of exported functions.
- It's better to change types (for local variables) than to cast.
- If we have established that a signed value is not negative, we can safely
  cast it to an unsigned type.

I split up the fixes in small chunks, to make them easier to review.

This is only covering libfdt for now (which is what those other projects
care about). There are more issues with dtc, but they can be addressed
later.

Please have a look, happy to discuss the invididual cases.

Cheers,
Andre

Changelog v1 .. v2:
- Omit the 8 patches that have already been applied.
- fdt_add_string_ (was 04/14): make every type unsigned; use subtractions
- fdt_move (was 05/14): add can_assume()
- fdt_create_with_flags (was 08/14): just make hdrsize signed
- libfdt_wip (was 10/14): drop redundant check for proplen being non-negative
- fdt_get_string (was 12/14): introduce unsigned variable (instead of casting)
- fdt_strerror (was 14/14): move int cast into FDT_ERRTABSIZE definition

Andre Przywara (6):
  libfdt: fdt_add_string_(): Fix comparison warning
  libfdt: fdt_move(): Fix comparison warnings
  libfdt: fdt_create_with_flags(): Fix comparison warning
  libfdt: libfdt_wip: Fix comparison warning
  libfdt: fdt_get_string(): Fix sequential write comparison warnings
  libfdt: fdt_strerror(): Fix comparison warning

 libfdt/fdt.c          |  5 ++++-
 libfdt/fdt_ro.c       | 10 ++++++----
 libfdt/fdt_strerror.c |  4 ++--
 libfdt/fdt_sw.c       | 18 +++++++++---------
 libfdt/fdt_wip.c      |  2 +-
 5 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.17.5

[PATCH v2 1/6] libfdt: fdt_add_string_(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_add_string_().

Make all variables unsigned, and express the negative offset trick via
subtractions in the code.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_sw.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 8de18fd..354f466 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -250,18 +250,18 @@ int fdt_end_node(void *fdt)
 static int fdt_add_string_(void *fdt, const char *s)
 {
 	char *strtab = (char *)fdt + fdt_totalsize(fdt);
-	int strtabsize = fdt_size_dt_strings(fdt);
-	int len = strlen(s) + 1;
-	int struct_top, offset;
+	unsigned int strtabsize = fdt_size_dt_strings(fdt);
+	unsigned int len = strlen(s) + 1;
+	unsigned int struct_top, offset;
 
-	offset = -strtabsize - len;
+	offset = strtabsize + len;
 	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
-	if (fdt_totalsize(fdt) + offset < struct_top)
+	if (fdt_totalsize(fdt) - offset < struct_top)
 		return 0; /* no more room :( */
 
-	memcpy(strtab + offset, s, len);
+	memcpy(strtab - offset, s, len);
 	fdt_set_size_dt_strings(fdt, strtabsize + len);
-	return offset;
+	return -offset;
 }
 
 /* Must only be used to roll back in case of error */
-- 
2.17.5

[PATCH v2 2/6] libfdt: fdt_move(): Fix comparison warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in comparisons in fdt_move().

This stems from "bufsize" being passed in as a signed integer, even
though we would expect a buffer size to be positive.

Short of changing the prototype, check that bufsize is not negative, and
cast it to an unsigned type in the comparison.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 04e1e06..6cf2fa0 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -314,9 +314,12 @@ const char *fdt_find_string_(const char *strtab, int tabsize, const char *s)
 
 int fdt_move(const void *fdt, void *buf, int bufsize)
 {
+	if (!can_assume(VALID_INPUT) && bufsize < 0)
+		return -FDT_ERR_NOSPACE;
+
 	FDT_RO_PROBE(fdt);
 
-	if (fdt_totalsize(fdt) > bufsize)
+	if (fdt_totalsize(fdt) > (unsigned int)bufsize)
 		return -FDT_ERR_NOSPACE;
 
 	memmove(buf, fdt, fdt_totalsize(fdt));
-- 
2.17.5

[PATCH v2 3/6] libfdt: fdt_create_with_flags(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_create_with_flags().

By making hdrsize a signed integer (we are sure it's a very small
number), we avoid all the casts and have matching types.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_sw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 354f466..68b543c 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -108,8 +108,8 @@ static void *fdt_grab_space_(void *fdt, size_t len)
 
 int fdt_create_with_flags(void *buf, int bufsize, uint32_t flags)
 {
-	const size_t hdrsize = FDT_ALIGN(sizeof(struct fdt_header),
-					 sizeof(struct fdt_reserve_entry));
+	const int hdrsize = FDT_ALIGN(sizeof(struct fdt_header),
+				      sizeof(struct fdt_reserve_entry));
 	void *fdt = buf;
 
 	if (bufsize < hdrsize)
-- 
2.17.5

[PATCH v2 4/6] libfdt: libfdt_wip: Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_setprop_inplace_namelen_partial().

fdt_getprop_namelen() will only return negative error values in "proplen"
if the return value is NULL. So we can rely on "proplen" being positive
in our case and can safely cast it to an unsigned type.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_wip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
index f64139e..c2d7566 100644
--- a/libfdt/fdt_wip.c
+++ b/libfdt/fdt_wip.c
@@ -23,7 +23,7 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
 	if (!propval)
 		return proplen;
 
-	if (proplen < (len + idx))
+	if ((unsigned)proplen < (len + idx))
 		return -FDT_ERR_NOSPACE;
 
 	memcpy((char *)propval + idx, val, len);
-- 
2.17.5

[PATCH v2 5/6] libfdt: fdt_get_string(): Fix sequential write comparison warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
comparisons in fdt_get_string().

Introduce a new usigned variable, which holds the actual (negated)
stroffset value, so we avoid negating all the other variables and have
proper types everywhere.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_ro.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index e192184..91cc6fe 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -67,11 +67,13 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 				len = fdt_size_dt_strings(fdt) - stroffset;
 		}
 	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
-		if ((stroffset >= 0)
-		    || (stroffset < -fdt_size_dt_strings(fdt)))
+		unsigned int sw_stroffset = -stroffset;
+
+		if ((stroffset >= 0) ||
+		    (sw_stroffset > fdt_size_dt_strings(fdt)))
 			goto fail;
-		if ((-stroffset) < len)
-			len = -stroffset;
+		if (sw_stroffset < len)
+			len = sw_stroffset;
 	} else {
 		err = -FDT_ERR_INTERNAL;
 		goto fail;
-- 
2.17.5

[PATCH v2 6/6] libfdt: fdt_strerror(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_strerror().

Force FDT_ERRTABSIZE to be signed (it's surely small enough to fit), so
that the types match. Also move the minus sign to errval, as this is
actually what we use in the next line.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_strerror.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
index 768db66..b435693 100644
--- a/libfdt/fdt_strerror.c
+++ b/libfdt/fdt_strerror.c
@@ -40,7 +40,7 @@ static struct fdt_errtabent fdt_errtable[] = {
 	FDT_ERRTABENT(FDT_ERR_NOPHANDLES),
 	FDT_ERRTABENT(FDT_ERR_BADFLAGS),
 };
-#define FDT_ERRTABSIZE	(sizeof(fdt_errtable) / sizeof(fdt_errtable[0]))
+#define FDT_ERRTABSIZE	((int)(sizeof(fdt_errtable) / sizeof(fdt_errtable[0])))
 
 const char *fdt_strerror(int errval)
 {
@@ -48,7 +48,7 @@ const char *fdt_strerror(int errval)
 		return "<valid offset/length>";
 	else if (errval == 0)
 		return "<no error>";
-	else if (errval > -FDT_ERRTABSIZE) {
+	else if (-errval < FDT_ERRTABSIZE) {
 		const char *s = fdt_errtable[-errval].str;
 
 		if (s)
-- 
2.17.5

Re: [PATCH v2 1/6] libfdt: fdt_add_string_(): Fix comparison warning

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

On Thu, Oct 01, 2020 at 05:46:25PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_add_string_().
> 
> Make all variables unsigned, and express the negative offset trick via
> subtractions in the code.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied.  I think there are some followup improvements we could make
here, though.

> ---
>  libfdt/fdt_sw.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 8de18fd..354f466 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -250,18 +250,18 @@ int fdt_end_node(void *fdt)
>  static int fdt_add_string_(void *fdt, const char *s)
>  {
>  	char *strtab = (char *)fdt + fdt_totalsize(fdt);
> -	int strtabsize = fdt_size_dt_strings(fdt);
> -	int len = strlen(s) + 1;
> -	int struct_top, offset;
> +	unsigned int strtabsize = fdt_size_dt_strings(fdt);
> +	unsigned int len = strlen(s) + 1;

In both the old and new versions, there's an implicit cast from size_t
here, which I think could theoretically overflow (with a colossal
string, 32-bit ints and 64-bit pointers/size_t).  So we probably
should actually check that this is <= INT_MAX.

> +	unsigned int struct_top, offset;
>  
> -	offset = -strtabsize - len;
> +	offset = strtabsize + len;
>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
> -	if (fdt_totalsize(fdt) + offset < struct_top)
> +	if (fdt_totalsize(fdt) - offset < struct_top)

Likewise we should check that totalisize - offset doesn't overflow
(underflow?).

>  		return 0; /* no more room :( */
>  
> -	memcpy(strtab + offset, s, len);
> +	memcpy(strtab - offset, s, len);
>  	fdt_set_size_dt_strings(fdt, strtabsize + len);
> -	return offset;
> +	return -offset;
>  }
>  
>  /* Must only be used to roll back in case of error */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/6] libfdt: fdt_move(): Fix comparison warnings

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

On Thu, Oct 01, 2020 at 05:46:26PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in comparisons in fdt_move().
> 
> This stems from "bufsize" being passed in as a signed integer, even
> though we would expect a buffer size to be positive.
> 
> Short of changing the prototype, check that bufsize is not negative, and
> cast it to an unsigned type in the comparison.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 04e1e06..6cf2fa0 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -314,9 +314,12 @@ const char *fdt_find_string_(const char *strtab, int tabsize, const char *s)
>  
>  int fdt_move(const void *fdt, void *buf, int bufsize)
>  {
> +	if (!can_assume(VALID_INPUT) && bufsize < 0)
> +		return -FDT_ERR_NOSPACE;
> +
>  	FDT_RO_PROBE(fdt);
>  
> -	if (fdt_totalsize(fdt) > bufsize)
> +	if (fdt_totalsize(fdt) > (unsigned int)bufsize)
>  		return -FDT_ERR_NOSPACE;
>  
>  	memmove(buf, fdt, fdt_totalsize(fdt));

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/6] libfdt: fdt_create_with_flags(): Fix comparison warning

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

On Thu, Oct 01, 2020 at 05:46:27PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_create_with_flags().
> 
> By making hdrsize a signed integer (we are sure it's a very small
> number), we avoid all the casts and have matching types.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_sw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 354f466..68b543c 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -108,8 +108,8 @@ static void *fdt_grab_space_(void *fdt, size_t len)
>  
>  int fdt_create_with_flags(void *buf, int bufsize, uint32_t flags)
>  {
> -	const size_t hdrsize = FDT_ALIGN(sizeof(struct fdt_header),
> -					 sizeof(struct fdt_reserve_entry));
> +	const int hdrsize = FDT_ALIGN(sizeof(struct fdt_header),
> +				      sizeof(struct fdt_reserve_entry));
>  	void *fdt = buf;
>  
>  	if (bufsize < hdrsize)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/6] libfdt: libfdt_wip: Fix comparison warning

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

On Thu, Oct 01, 2020 at 05:46:28PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_setprop_inplace_namelen_partial().
> 
> fdt_getprop_namelen() will only return negative error values in "proplen"
> if the return value is NULL. So we can rely on "proplen" being positive
> in our case and can safely cast it to an unsigned type.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_wip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
> index f64139e..c2d7566 100644
> --- a/libfdt/fdt_wip.c
> +++ b/libfdt/fdt_wip.c
> @@ -23,7 +23,7 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
>  	if (!propval)
>  		return proplen;
>  
> -	if (proplen < (len + idx))
> +	if ((unsigned)proplen < (len + idx))
>  		return -FDT_ERR_NOSPACE;
>  
>  	memcpy((char *)propval + idx, val, len);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/6] libfdt: fdt_get_string(): Fix sequential write comparison warnings

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

On Thu, Oct 01, 2020 at 05:46:29PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in fdt_get_string().
> 
> Introduce a new usigned variable, which holds the actual (negated)
> stroffset value, so we avoid negating all the other variables and have
> proper types everywhere.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_ro.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index e192184..91cc6fe 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -67,11 +67,13 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  				len = fdt_size_dt_strings(fdt) - stroffset;
>  		}
>  	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
> -		if ((stroffset >= 0)
> -		    || (stroffset < -fdt_size_dt_strings(fdt)))
> +		unsigned int sw_stroffset = -stroffset;
> +
> +		if ((stroffset >= 0) ||
> +		    (sw_stroffset > fdt_size_dt_strings(fdt)))
>  			goto fail;
> -		if ((-stroffset) < len)
> -			len = -stroffset;
> +		if (sw_stroffset < len)
> +			len = sw_stroffset;
>  	} else {
>  		err = -FDT_ERR_INTERNAL;
>  		goto fail;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/6] libfdt: fdt_strerror(): Fix comparison warning

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

On Thu, Oct 01, 2020 at 05:46:30PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_strerror().
> 
> Force FDT_ERRTABSIZE to be signed (it's surely small enough to fit), so
> that the types match. Also move the minus sign to errval, as this is
> actually what we use in the next line.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_strerror.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
> index 768db66..b435693 100644
> --- a/libfdt/fdt_strerror.c
> +++ b/libfdt/fdt_strerror.c
> @@ -40,7 +40,7 @@ static struct fdt_errtabent fdt_errtable[] = {
>  	FDT_ERRTABENT(FDT_ERR_NOPHANDLES),
>  	FDT_ERRTABENT(FDT_ERR_BADFLAGS),
>  };
> -#define FDT_ERRTABSIZE	(sizeof(fdt_errtable) / sizeof(fdt_errtable[0]))
> +#define FDT_ERRTABSIZE	((int)(sizeof(fdt_errtable) / sizeof(fdt_errtable[0])))
>  
>  const char *fdt_strerror(int errval)
>  {
> @@ -48,7 +48,7 @@ const char *fdt_strerror(int errval)
>  		return "<valid offset/length>";
>  	else if (errval == 0)
>  		return "<no error>";
> -	else if (errval > -FDT_ERRTABSIZE) {
> +	else if (-errval < FDT_ERRTABSIZE) {
>  		const char *s = fdt_errtable[-errval].str;
>  
>  		if (s)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

On Thu, Oct 01, 2020 at 05:46:24PM +0100, Andre Przywara wrote:
> Those are the six remaining patches of the initial post to fix the
> C comparison warnings.
> I reworked the fixes according to David's comments, and took quite a
> different approach for some of them.
> Changelog below.
> 
> The series is against https://github.com/dgibson/dtc/commits/main
> ------------------------------------
> 
> When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
> some warnings about the signedness of the operands not matching:
> =================
> libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>    if ((absoffset < offset)
> .....
> =================
> 
> This does not occur under normal conditions in the dtc repo, but might
> show up when libfdt is embedded in another project. There have been reports
> from U-Boot and Trusted-Firmware-A.
> 
> The underlying issue is mostly due to C's promotion behaviour (ANSI C
> section 6.1.3.8) when dealing with operands of different signedness
> (but same size): Signed values get implictly casted to unsigned, which
> is not typically what we want if they could have been negative.
> 
> The Internet(TM) suggests that blindly applying casts is probably doing
> more harm than it helps, so this series tries to fix the underlying
> issues properly.
> In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
> some signed types are due to them being returned along wih error values in
> other functions (node offsets).
> So these fixes here have been based on the following assumptions:
> - We cannot change the prototype of exported functions.
> - It's better to change types (for local variables) than to cast.
> - If we have established that a signed value is not negative, we can safely
>   cast it to an unsigned type.
> 
> I split up the fixes in small chunks, to make them easier to review.
> 
> This is only covering libfdt for now (which is what those other projects
> care about). There are more issues with dtc, but they can be addressed
> later.
> 
> Please have a look, happy to discuss the invididual cases.

Thanks again for this work.  I've applied all the remaining patches,
although I have some comments for some followups I think would be
good.

At this point can we turn on -Wsign-compare by default?  That sounds
like a good idea to stop these problems creeping back in.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

[André Przywara]

On 02/10/2020 02:03, David Gibson wrote:

Hi David,

> On Thu, Oct 01, 2020 at 05:46:24PM +0100, Andre Przywara wrote:
>> Those are the six remaining patches of the initial post to fix the
>> C comparison warnings.
>> I reworked the fixes according to David's comments, and took quite a
>> different approach for some of them.
>> Changelog below.
>>
>> The series is against https://github.com/dgibson/dtc/commits/main
>> ------------------------------------
>>
>> When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
>> some warnings about the signedness of the operands not matching:
>> =================
>> libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>    if ((absoffset < offset)
>> .....
>> =================
>>
>> This does not occur under normal conditions in the dtc repo, but might
>> show up when libfdt is embedded in another project. There have been reports
>> from U-Boot and Trusted-Firmware-A.
>>
>> The underlying issue is mostly due to C's promotion behaviour (ANSI C
>> section 6.1.3.8) when dealing with operands of different signedness
>> (but same size): Signed values get implictly casted to unsigned, which
>> is not typically what we want if they could have been negative.
>>
>> The Internet(TM) suggests that blindly applying casts is probably doing
>> more harm than it helps, so this series tries to fix the underlying
>> issues properly.
>> In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
>> some signed types are due to them being returned along wih error values in
>> other functions (node offsets).
>> So these fixes here have been based on the following assumptions:
>> - We cannot change the prototype of exported functions.
>> - It's better to change types (for local variables) than to cast.
>> - If we have established that a signed value is not negative, we can safely
>>   cast it to an unsigned type.
>>
>> I split up the fixes in small chunks, to make them easier to review.
>>
>> This is only covering libfdt for now (which is what those other projects
>> care about). There are more issues with dtc, but they can be addressed
>> later.
>>
>> Please have a look, happy to discuss the invididual cases.
> 
> Thanks again for this work.  I've applied all the remaining patches,
> although I have some comments for some followups I think would be
> good.

Thanks, that's much appreciated! Happy to discuss any further comments.

> At this point can we turn on -Wsign-compare by default?  That sounds
> like a good idea to stop these problems creeping back in.

Not quite yet: the patches were only covering "make libfdt". I see more
reports for dtc and the tests. I should probably address those as long
as this is still in my "L1 cache" ...
Unless you want to turn on -Wsign-compare just for libfdt.

Cheers,
Andre

Re: [PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3524 bytes --]

On Fri, Oct 02, 2020 at 10:25:09AM +0100, André Przywara wrote:
> On 02/10/2020 02:03, David Gibson wrote:
> 
> Hi David,
> 
> > On Thu, Oct 01, 2020 at 05:46:24PM +0100, Andre Przywara wrote:
> >> Those are the six remaining patches of the initial post to fix the
> >> C comparison warnings.
> >> I reworked the fixes according to David's comments, and took quite a
> >> different approach for some of them.
> >> Changelog below.
> >>
> >> The series is against https://github.com/dgibson/dtc/commits/main
> >> ------------------------------------
> >>
> >> When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
> >> some warnings about the signedness of the operands not matching:
> >> =================
> >> libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
> >>    if ((absoffset < offset)
> >> .....
> >> =================
> >>
> >> This does not occur under normal conditions in the dtc repo, but might
> >> show up when libfdt is embedded in another project. There have been reports
> >> from U-Boot and Trusted-Firmware-A.
> >>
> >> The underlying issue is mostly due to C's promotion behaviour (ANSI C
> >> section 6.1.3.8) when dealing with operands of different signedness
> >> (but same size): Signed values get implictly casted to unsigned, which
> >> is not typically what we want if they could have been negative.
> >>
> >> The Internet(TM) suggests that blindly applying casts is probably doing
> >> more harm than it helps, so this series tries to fix the underlying
> >> issues properly.
> >> In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
> >> some signed types are due to them being returned along wih error values in
> >> other functions (node offsets).
> >> So these fixes here have been based on the following assumptions:
> >> - We cannot change the prototype of exported functions.
> >> - It's better to change types (for local variables) than to cast.
> >> - If we have established that a signed value is not negative, we can safely
> >>   cast it to an unsigned type.
> >>
> >> I split up the fixes in small chunks, to make them easier to review.
> >>
> >> This is only covering libfdt for now (which is what those other projects
> >> care about). There are more issues with dtc, but they can be addressed
> >> later.
> >>
> >> Please have a look, happy to discuss the invididual cases.
> > 
> > Thanks again for this work.  I've applied all the remaining patches,
> > although I have some comments for some followups I think would be
> > good.
> 
> Thanks, that's much appreciated! Happy to discuss any further comments.
> 
> > At this point can we turn on -Wsign-compare by default?  That sounds
> > like a good idea to stop these problems creeping back in.
> 
> Not quite yet: the patches were only covering "make libfdt". I see more
> reports for dtc and the tests. I should probably address those as long
> as this is still in my "L1 cache" ...
> Unless you want to turn on -Wsign-compare just for libfdt.

If you think you can get us there for the whole project reasonably
soon, then wait for that.  If it looks like you might need to shelve
this for a while, then it would be good to get it enabled for just
libfdt.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

[André Przywara]

On 02/10/2020 13:32, David Gibson wrote:
> On Fri, Oct 02, 2020 at 10:25:09AM +0100, André Przywara wrote:
>> On 02/10/2020 02:03, David Gibson wrote:
>>
>> Hi David,
>>
>>> On Thu, Oct 01, 2020 at 05:46:24PM +0100, Andre Przywara wrote:
>>>> Those are the six remaining patches of the initial post to fix the
>>>> C comparison warnings.
>>>> I reworked the fixes according to David's comments, and took quite a
>>>> different approach for some of them.
>>>> Changelog below.
>>>>
>>>> The series is against https://github.com/dgibson/dtc/commits/main
>>>> ------------------------------------
>>>>
>>>> When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
>>>> some warnings about the signedness of the operands not matching:
>>>> =================
>>>> libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>>>    if ((absoffset < offset)
>>>> .....
>>>> =================
>>>>
>>>> This does not occur under normal conditions in the dtc repo, but might
>>>> show up when libfdt is embedded in another project. There have been reports
>>>> from U-Boot and Trusted-Firmware-A.
>>>>
>>>> The underlying issue is mostly due to C's promotion behaviour (ANSI C
>>>> section 6.1.3.8) when dealing with operands of different signedness
>>>> (but same size): Signed values get implictly casted to unsigned, which
>>>> is not typically what we want if they could have been negative.
>>>>
>>>> The Internet(TM) suggests that blindly applying casts is probably doing
>>>> more harm than it helps, so this series tries to fix the underlying
>>>> issues properly.
>>>> In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
>>>> some signed types are due to them being returned along wih error values in
>>>> other functions (node offsets).
>>>> So these fixes here have been based on the following assumptions:
>>>> - We cannot change the prototype of exported functions.
>>>> - It's better to change types (for local variables) than to cast.
>>>> - If we have established that a signed value is not negative, we can safely
>>>>   cast it to an unsigned type.
>>>>
>>>> I split up the fixes in small chunks, to make them easier to review.
>>>>
>>>> This is only covering libfdt for now (which is what those other projects
>>>> care about). There are more issues with dtc, but they can be addressed
>>>> later.
>>>>
>>>> Please have a look, happy to discuss the invididual cases.
>>>
>>> Thanks again for this work.  I've applied all the remaining patches,
>>> although I have some comments for some followups I think would be
>>> good.
>>
>> Thanks, that's much appreciated! Happy to discuss any further comments.
>>
>>> At this point can we turn on -Wsign-compare by default?  That sounds
>>> like a good idea to stop these problems creeping back in.
>>
>> Not quite yet: the patches were only covering "make libfdt". I see more
>> reports for dtc and the tests. I should probably address those as long
>> as this is still in my "L1 cache" ...
>> Unless you want to turn on -Wsign-compare just for libfdt.
> 
> If you think you can get us there for the whole project reasonably
> soon, then wait for that.  If it looks like you might need to shelve
> this for a while, then it would be good to get it enabled for just
> libfdt.

So I have "make tests" covered now, that looks mostly reasonable.
The rest is more nasty, I needed to paper over a few of them to make
some progress. But "make all check" comes out clean now.

I will try to make reasonable patches out this mess next week.

Cheers,
Andre