[PATCH v2 0/8] dtc: Fix signed/unsigned comparison warnings

[PATCH v2 0/8] dtc: Fix signed/unsigned comparison warnings

[Andre Przywara]

Hi,

sorry for dropping the ball on this, but here in an update for the half
finished series from last year to fix signedness comparison warnings in
the tools (dtc/tests) - the libfdt fixes were already merged.
I addressed all (but one) of David's comments back from October, hopefully
this is good to go now.

Changelog v1 .. v2:
- dropped 2/11, 6/11 and 8/11, which are already merged
- tests: add explicit tests for negative len values
- use size_t instead of "unsigned int", where appropriate
- fdtget: expand hard-to-read conditional operator into switch/case
- dtc: use static inline fdt_is_valid_phandle() instead of macro
- use strtoul() to parse unsigned value

-----------------------

When dtc and the other tools in the tree are compiled with -Wsign-compare
or -Wextra, GCC emits quite some warnings about the signedness of the
operands not matching:
=================
In file included from dtc.h:15:0,
                 from checks.c:6:
checks.c: In function ‘fixup_phandle_references’:
checks.c:591:38: error: comparison between signed and unsigned integer
expressions [-Werror=sign-compare]
    assert(m->offset + sizeof(cell_t) <= prop->val.len);
.....
=================

libfdt has just been fixed in this regard recently; this adds the missing
bits to cover the rest of the source tree, namely dtc, the various smaller
tools, checks, and tests.

Those warnings do not show under normal conditions in the dtc repo at the
moment, because we avoid to enable the warning option.

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.
Many types in dtc are somewhat suboptimal, they hold a size (which should
be non-negative), but are "int" anyway. Loop counters just declared as
"int i;" are another frequent issue.

The main strategy to fix those issues is to make the types right, which
mostly means to make variables "unsigned".
If that is not easily possible, we cast the signed expression to
"unsigned", provided this is safe, because it cannot be negative.

This series gathers multiple similar fixes in one patch, splitting them
mostly by the tool or source file. This is just to simplify review, the
patches could also be merged later on.

I tried to put some rationale in most of the patches, but explaining
every single instance becomes really tedious (so some patches paper over
that).

The final patch eventually enables the warning option in question, that
should avoid those kind of errors creeping back in again.

Compile tested with "make clean && make all tests check", after every
patch, on x86-64 and arm64.

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

Cheers,
Andre

Andre Przywara (8):
  tests: Fix signedness comparisons warnings
  fdtdump: Fix signedness comparisons warnings
  fdtget: Fix signedness comparisons warnings
  dtc: Wrap phandle validity check
  dtc: Fix signedness comparisons warnings: reservednum
  dtc: Fix signedness comparisons warnings: pointer diff
  checks: Fix signedness comparisons warnings
  Makefile: add -Wsign-compare to warning options

 Makefile                    |  2 +-
 checks.c                    | 31 ++++++++++++++++---------------
 dtc.c                       |  4 ++--
 dtc.h                       |  7 ++++++-
 fdtdump.c                   |  6 +++---
 fdtget.c                    | 10 ++++++++--
 flattree.c                  |  2 +-
 livetree.c                  |  6 +++---
 tests/dumptrees.c           |  2 +-
 tests/fs_tree1.c            |  2 +-
 tests/get_name.c            |  4 +++-
 tests/integer-expressions.c |  2 +-
 tests/nopulate.c            |  3 ++-
 tests/overlay.c             |  4 +++-
 tests/phandle_format.c      |  2 +-
 tests/property_iterate.c    |  2 +-
 tests/references.c          |  2 +-
 tests/set_name.c            |  6 ++++--
 tests/subnode_iterate.c     |  2 +-
 tests/tests.h               |  2 +-
 tests/testutils.c           | 12 +++++++++---
 21 files changed, 69 insertions(+), 44 deletions(-)

-- 
2.17.5

[PATCH v2 1/8] tests: Fix signedness comparisons warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
comparisons in various files in the tests/ directory.

For about half of the cases we can simply change the signed variable to
be of an unsigned type, because they will never need to store negative
values (which is the best fix of the problem).

In the remaining cases we can cast the signed variable to an unsigned
type, provided we know for sure it is not negative.
We see two different scenarios here:
- We either just explicitly checked for this variable to be positive
  (if (rc < 0) FAIL();), or
- We rely on a function returning only positive values in the "length"
  pointer if the function returned successfully: which we just checked.

At two occassions we compare with a constant "-1" (even though the
variable is unsigned), so we just change this to ~0U to create an
unsigned comparison value.

Since this is about the tests, let's also add explicit tests for those
values really not being negative.

This fixes "make tests" (but not "make check" yet), when compiled
with -Wsign-compare.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 tests/dumptrees.c           |  2 +-
 tests/fs_tree1.c            |  2 +-
 tests/get_name.c            |  4 +++-
 tests/integer-expressions.c |  2 +-
 tests/nopulate.c            |  3 ++-
 tests/overlay.c             |  4 +++-
 tests/phandle_format.c      |  2 +-
 tests/property_iterate.c    |  2 +-
 tests/references.c          |  2 +-
 tests/set_name.c            |  6 ++++--
 tests/subnode_iterate.c     |  2 +-
 tests/tests.h               |  2 +-
 tests/testutils.c           | 12 +++++++++---
 13 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/tests/dumptrees.c b/tests/dumptrees.c
index f1e0ea9..08967b3 100644
--- a/tests/dumptrees.c
+++ b/tests/dumptrees.c
@@ -32,7 +32,7 @@ static struct {
 
 int main(int argc, char *argv[])
 {
-	int i;
+	unsigned int i;
 
 	if (argc != 2) {
 	    fprintf(stderr, "Missing output directory argument\n");
diff --git a/tests/fs_tree1.c b/tests/fs_tree1.c
index dff3880..978f6a3 100644
--- a/tests/fs_tree1.c
+++ b/tests/fs_tree1.c
@@ -54,7 +54,7 @@ static void mkfile(const char *name, void *data, size_t len)
 	rc = write(fd, data, len);
 	if (rc < 0)
 		FAIL("write(\"%s\"): %s", name, strerror(errno));
-	if (rc != len)
+	if ((unsigned)rc != len)
 		FAIL("write(\"%s\"): short write", name);
 	
 	rc = close(fd);
diff --git a/tests/get_name.c b/tests/get_name.c
index 5a35103..d20bf30 100644
--- a/tests/get_name.c
+++ b/tests/get_name.c
@@ -34,12 +34,14 @@ static void check_name(void *fdt, const char *path)
 		       offset, getname, len);
 	if (!getname)
 		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
+	if (len < 0)
+		FAIL("negative name length (%d) for returned node name\n", len);
 
 	if (strcmp(getname, checkname) != 0)
 		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
 		     path, getname, checkname);
 
-	if (len != strlen(getname))
+	if ((unsigned)len != strlen(getname))
 		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
 		     path, len, strlen(getname));
 
diff --git a/tests/integer-expressions.c b/tests/integer-expressions.c
index 6f33d81..2f164d9 100644
--- a/tests/integer-expressions.c
+++ b/tests/integer-expressions.c
@@ -59,7 +59,7 @@ int main(int argc, char *argv[])
 	void *fdt;
 	const fdt32_t *res;
 	int reslen;
-	int i;
+	unsigned int i;
 
 	test_init(argc, argv);
 
diff --git a/tests/nopulate.c b/tests/nopulate.c
index 2ae1753..e06a0b3 100644
--- a/tests/nopulate.c
+++ b/tests/nopulate.c
@@ -43,7 +43,8 @@ static int nopulate_struct(char *buf, const char *fdt)
 int main(int argc, char *argv[])
 {
 	char *fdt, *fdt2, *buf;
-	int newsize, struct_start, struct_end_old, struct_end_new, delta;
+	int newsize, struct_end_old, struct_end_new, delta;
+	unsigned int struct_start;
 	const char *inname;
 	char outname[PATH_MAX];
 
diff --git a/tests/overlay.c b/tests/overlay.c
index 91afa72..b21b28e 100644
--- a/tests/overlay.c
+++ b/tests/overlay.c
@@ -35,7 +35,9 @@ static int fdt_getprop_u32_by_poffset(void *fdt, const char *path,
 		return node_off;
 
 	val = fdt_getprop(fdt, node_off, name, &len);
-	if (!val || (len < (sizeof(uint32_t) * (poffset + 1))))
+	if (val && len < 0)
+		return -FDT_ERR_BADVALUE;
+	if (!val || ((unsigned)len < (sizeof(uint32_t) * (poffset + 1))))
 		return -FDT_ERR_NOTFOUND;
 
 	*out = fdt32_to_cpu(*(val + poffset));
diff --git a/tests/phandle_format.c b/tests/phandle_format.c
index d00618f..0febb32 100644
--- a/tests/phandle_format.c
+++ b/tests/phandle_format.c
@@ -45,7 +45,7 @@ int main(int argc, char *argv[])
 		FAIL("fdt_path_offset(/node4): %s", fdt_strerror(n4));
 
 	h4 = fdt_get_phandle(fdt, n4);
-	if ((h4 == 0) || (h4 == -1))
+	if ((h4 == 0) || (h4 == ~0U))
 		FAIL("/node4 has bad phandle 0x%x\n", h4);
 
 	if (phandle_format & PHANDLE_LEGACY)
diff --git a/tests/property_iterate.c b/tests/property_iterate.c
index 9a67f49..0b6af9b 100644
--- a/tests/property_iterate.c
+++ b/tests/property_iterate.c
@@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
 	uint32_t properties;
 	const fdt32_t *prop;
 	int offset, property;
-	int count;
+	unsigned int count;
 	int len;
 
 	/*
diff --git a/tests/references.c b/tests/references.c
index d18e722..cb1daaa 100644
--- a/tests/references.c
+++ b/tests/references.c
@@ -106,7 +106,7 @@ int main(int argc, char *argv[])
 	if ((h4 == 0x2000) || (h4 == 0x1) || (h4 == 0))
 		FAIL("/node4 has bad phandle, 0x%x", h4);
 
-	if ((h5 == 0) || (h5 == -1))
+	if ((h5 == 0) || (h5 == ~0U))
 		FAIL("/node5 has bad phandle, 0x%x", h5);
 	if ((h5 == h4) || (h5 == h2) || (h5 == h1))
 		FAIL("/node5 has duplicate phandle, 0x%x", h5);
diff --git a/tests/set_name.c b/tests/set_name.c
index a62cb58..5eeb7b9 100644
--- a/tests/set_name.c
+++ b/tests/set_name.c
@@ -39,7 +39,7 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
 		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
 		     path, getname, oldname);
 
-	if (len != strlen(getname))
+	if ((unsigned)len != strlen(getname))
 		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
 		     path, len, strlen(getname));
 
@@ -51,12 +51,14 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
 	getname = fdt_get_name(fdt, offset, &len);
 	if (!getname)
 		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
+	if (len < 0)
+		FAIL("negative name length (%d) for returned node name\n", len);
 
 	if (strcmp(getname, newname) != 0)
 		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
 		     path, getname, newname);
 
-	if (len != strlen(getname))
+	if ((unsigned)len != strlen(getname))
 		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
 		     path, len, strlen(getname));
 }
diff --git a/tests/subnode_iterate.c b/tests/subnode_iterate.c
index 2dc9b2d..2553a51 100644
--- a/tests/subnode_iterate.c
+++ b/tests/subnode_iterate.c
@@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
 	uint32_t subnodes;
 	const fdt32_t *prop;
 	int offset;
-	int count;
+	unsigned int count;
 	int len;
 
 	/* This property indicates the number of subnodes to expect */
diff --git a/tests/tests.h b/tests/tests.h
index 1017366..bf8f23c 100644
--- a/tests/tests.h
+++ b/tests/tests.h
@@ -83,7 +83,7 @@ void cleanup(void);
 void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size);
 
 void check_property(void *fdt, int nodeoffset, const char *name,
-		    int len, const void *val);
+		    unsigned int len, const void *val);
 #define check_property_cell(fdt, nodeoffset, name, val) \
 	({ \
 		fdt32_t x = cpu_to_fdt32(val);			      \
diff --git a/tests/testutils.c b/tests/testutils.c
index 5e494c5..10129c0 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -88,7 +88,7 @@ void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size)
 }
 
 void check_property(void *fdt, int nodeoffset, const char *name,
-		    int len, const void *val)
+		    unsigned int len, const void *val)
 {
 	const struct fdt_property *prop;
 	int retlen, namelen;
@@ -101,6 +101,9 @@ void check_property(void *fdt, int nodeoffset, const char *name,
 	if (! prop)
 		FAIL("Error retrieving \"%s\" pointer: %s", name,
 		     fdt_strerror(retlen));
+	if (retlen < 0)
+		FAIL("negative name length (%d) for returned property\n",
+		     retlen);
 
 	tag = fdt32_to_cpu(prop->tag);
 	nameoff = fdt32_to_cpu(prop->nameoff);
@@ -112,13 +115,16 @@ void check_property(void *fdt, int nodeoffset, const char *name,
 	propname = fdt_get_string(fdt, nameoff, &namelen);
 	if (!propname)
 		FAIL("Couldn't get property name: %s", fdt_strerror(namelen));
-	if (namelen != strlen(propname))
+	if (namelen < 0)
+		FAIL("negative name length (%d) for returned string\n",
+		     namelen);
+	if ((unsigned)namelen != strlen(propname))
 		FAIL("Incorrect prop name length: %d instead of %zd",
 		     namelen, strlen(propname));
 	if (!streq(propname, name))
 		FAIL("Property name mismatch \"%s\" instead of \"%s\"",
 		     propname, name);
-	if (proplen != retlen)
+	if (proplen != (unsigned)retlen)
 		FAIL("Length retrieved for \"%s\" by fdt_get_property()"
 		     " differs from stored length (%d != %d)",
 		     name, retlen, proplen);
-- 
2.17.5

Re: [PATCH v2 1/8] tests: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:33PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in various files in the tests/ directory.
> 
> For about half of the cases we can simply change the signed variable to
> be of an unsigned type, because they will never need to store negative
> values (which is the best fix of the problem).
> 
> In the remaining cases we can cast the signed variable to an unsigned
> type, provided we know for sure it is not negative.
> We see two different scenarios here:
> - We either just explicitly checked for this variable to be positive
>   (if (rc < 0) FAIL();), or
> - We rely on a function returning only positive values in the "length"
>   pointer if the function returned successfully: which we just checked.
> 
> At two occassions we compare with a constant "-1" (even though the
> variable is unsigned), so we just change this to ~0U to create an
> unsigned comparison value.
> 
> Since this is about the tests, let's also add explicit tests for those
> values really not being negative.
> 
> This fixes "make tests" (but not "make check" yet), when compiled
> with -Wsign-compare.

Thanks for doing this.

> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  tests/dumptrees.c           |  2 +-
>  tests/fs_tree1.c            |  2 +-
>  tests/get_name.c            |  4 +++-
>  tests/integer-expressions.c |  2 +-
>  tests/nopulate.c            |  3 ++-
>  tests/overlay.c             |  4 +++-
>  tests/phandle_format.c      |  2 +-
>  tests/property_iterate.c    |  2 +-
>  tests/references.c          |  2 +-
>  tests/set_name.c            |  6 ++++--
>  tests/subnode_iterate.c     |  2 +-
>  tests/tests.h               |  2 +-
>  tests/testutils.c           | 12 +++++++++---
>  13 files changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/dumptrees.c b/tests/dumptrees.c
> index f1e0ea9..08967b3 100644
> --- a/tests/dumptrees.c
> +++ b/tests/dumptrees.c
> @@ -32,7 +32,7 @@ static struct {
>  
>  int main(int argc, char *argv[])
>  {
> -	int i;
> +	unsigned int i;
>  
>  	if (argc != 2) {
>  	    fprintf(stderr, "Missing output directory argument\n");
> diff --git a/tests/fs_tree1.c b/tests/fs_tree1.c
> index dff3880..978f6a3 100644
> --- a/tests/fs_tree1.c
> +++ b/tests/fs_tree1.c
> @@ -54,7 +54,7 @@ static void mkfile(const char *name, void *data, size_t len)
>  	rc = write(fd, data, len);
>  	if (rc < 0)
>  		FAIL("write(\"%s\"): %s", name, strerror(errno));
> -	if (rc != len)
> +	if ((unsigned)rc != len)
>  		FAIL("write(\"%s\"): short write", name);
>  	
>  	rc = close(fd);
> diff --git a/tests/get_name.c b/tests/get_name.c
> index 5a35103..d20bf30 100644
> --- a/tests/get_name.c
> +++ b/tests/get_name.c
> @@ -34,12 +34,14 @@ static void check_name(void *fdt, const char *path)
>  		       offset, getname, len);
>  	if (!getname)
>  		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
> +	if (len < 0)
> +		FAIL("negative name length (%d) for returned node name\n", len);
>  
>  	if (strcmp(getname, checkname) != 0)
>  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
>  		     path, getname, checkname);
>  
> -	if (len != strlen(getname))
> +	if ((unsigned)len != strlen(getname))
>  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
>  		     path, len, strlen(getname));
>  
> diff --git a/tests/integer-expressions.c b/tests/integer-expressions.c
> index 6f33d81..2f164d9 100644
> --- a/tests/integer-expressions.c
> +++ b/tests/integer-expressions.c
> @@ -59,7 +59,7 @@ int main(int argc, char *argv[])
>  	void *fdt;
>  	const fdt32_t *res;
>  	int reslen;
> -	int i;
> +	unsigned int i;
>  
>  	test_init(argc, argv);
>  
> diff --git a/tests/nopulate.c b/tests/nopulate.c
> index 2ae1753..e06a0b3 100644
> --- a/tests/nopulate.c
> +++ b/tests/nopulate.c
> @@ -43,7 +43,8 @@ static int nopulate_struct(char *buf, const char *fdt)
>  int main(int argc, char *argv[])
>  {
>  	char *fdt, *fdt2, *buf;
> -	int newsize, struct_start, struct_end_old, struct_end_new, delta;
> +	int newsize, struct_end_old, struct_end_new, delta;
> +	unsigned int struct_start;

Making just one of these variables unsigned looks pretty weird, but I
guess it works.  The alternative would be to much more strictly check
the various offsets here - which would also mean that adding the nops
does take the device tree beyond the allowed size.

>  	const char *inname;
>  	char outname[PATH_MAX];
>  
> diff --git a/tests/overlay.c b/tests/overlay.c
> index 91afa72..b21b28e 100644
> --- a/tests/overlay.c
> +++ b/tests/overlay.c
> @@ -35,7 +35,9 @@ static int fdt_getprop_u32_by_poffset(void *fdt, const char *path,
>  		return node_off;
>  
>  	val = fdt_getprop(fdt, node_off, name, &len);
> -	if (!val || (len < (sizeof(uint32_t) * (poffset + 1))))
> +	if (val && len < 0)
> +		return -FDT_ERR_BADVALUE;

This indicates an internal error in libfdt that's more or less
independent of what this test is really looking for, better to just
FAIL() here rather than bubble this error up through the caller to
report.

> +	if (!val || ((unsigned)len < (sizeof(uint32_t) * (poffset + 1))))
>  		return -FDT_ERR_NOTFOUND;

NOTFOUND seems kind of dangeous here, because this test catches both
true NOTFOUND cases (val==NULL && len == NOTFOUND), and broken cases
(val=NULL && len!=NOTFOUND), best to check the broken case explicitly
and FAIL().

>  
>  	*out = fdt32_to_cpu(*(val + poffset));
> diff --git a/tests/phandle_format.c b/tests/phandle_format.c
> index d00618f..0febb32 100644
> --- a/tests/phandle_format.c
> +++ b/tests/phandle_format.c
> @@ -45,7 +45,7 @@ int main(int argc, char *argv[])
>  		FAIL("fdt_path_offset(/node4): %s", fdt_strerror(n4));
>  
>  	h4 = fdt_get_phandle(fdt, n4);
> -	if ((h4 == 0) || (h4 == -1))
> +	if ((h4 == 0) || (h4 == ~0U))
>  		FAIL("/node4 has bad phandle 0x%x\n", h4);
>  
>  	if (phandle_format & PHANDLE_LEGACY)
> diff --git a/tests/property_iterate.c b/tests/property_iterate.c
> index 9a67f49..0b6af9b 100644
> --- a/tests/property_iterate.c
> +++ b/tests/property_iterate.c
> @@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
>  	uint32_t properties;
>  	const fdt32_t *prop;
>  	int offset, property;
> -	int count;
> +	unsigned int count;
>  	int len;
>  
>  	/*
> diff --git a/tests/references.c b/tests/references.c
> index d18e722..cb1daaa 100644
> --- a/tests/references.c
> +++ b/tests/references.c
> @@ -106,7 +106,7 @@ int main(int argc, char *argv[])
>  	if ((h4 == 0x2000) || (h4 == 0x1) || (h4 == 0))
>  		FAIL("/node4 has bad phandle, 0x%x", h4);
>  
> -	if ((h5 == 0) || (h5 == -1))
> +	if ((h5 == 0) || (h5 == ~0U))
>  		FAIL("/node5 has bad phandle, 0x%x", h5);
>  	if ((h5 == h4) || (h5 == h2) || (h5 == h1))
>  		FAIL("/node5 has duplicate phandle, 0x%x", h5);
> diff --git a/tests/set_name.c b/tests/set_name.c
> index a62cb58..5eeb7b9 100644
> --- a/tests/set_name.c
> +++ b/tests/set_name.c
> @@ -39,7 +39,7 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
>  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
>  		     path, getname, oldname);
>

> -	if (len != strlen(getname))
> +	if ((unsigned)len != strlen(getname))

AFAICT you haven't actually checked for len < 0 before this, so this
isn't quite right


>  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
>  		     path, len, strlen(getname));
>  
> @@ -51,12 +51,14 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
>  	getname = fdt_get_name(fdt, offset, &len);
>  	if (!getname)
>  		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
> +	if (len < 0)
> +		FAIL("negative name length (%d) for returned node name\n", len);
>  
>  	if (strcmp(getname, newname) != 0)
>  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
>  		     path, getname, newname);
>  
> -	if (len != strlen(getname))
> +	if ((unsigned)len != strlen(getname))
>  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
>  		     path, len, strlen(getname));
>  }
> diff --git a/tests/subnode_iterate.c b/tests/subnode_iterate.c
> index 2dc9b2d..2553a51 100644
> --- a/tests/subnode_iterate.c
> +++ b/tests/subnode_iterate.c
> @@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
>  	uint32_t subnodes;
>  	const fdt32_t *prop;
>  	int offset;
> -	int count;
> +	unsigned int count;
>  	int len;
>  
>  	/* This property indicates the number of subnodes to expect */
> diff --git a/tests/tests.h b/tests/tests.h
> index 1017366..bf8f23c 100644
> --- a/tests/tests.h
> +++ b/tests/tests.h
> @@ -83,7 +83,7 @@ void cleanup(void);
>  void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size);
>  
>  void check_property(void *fdt, int nodeoffset, const char *name,
> -		    int len, const void *val);
> +		    unsigned int len, const void *val);
>  #define check_property_cell(fdt, nodeoffset, name, val) \
>  	({ \
>  		fdt32_t x = cpu_to_fdt32(val);			      \
> diff --git a/tests/testutils.c b/tests/testutils.c
> index 5e494c5..10129c0 100644
> --- a/tests/testutils.c
> +++ b/tests/testutils.c
> @@ -88,7 +88,7 @@ void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size)
>  }
>  
>  void check_property(void *fdt, int nodeoffset, const char *name,
> -		    int len, const void *val)
> +		    unsigned int len, const void *val)
>  {
>  	const struct fdt_property *prop;
>  	int retlen, namelen;
> @@ -101,6 +101,9 @@ void check_property(void *fdt, int nodeoffset, const char *name,
>  	if (! prop)
>  		FAIL("Error retrieving \"%s\" pointer: %s", name,
>  		     fdt_strerror(retlen));
> +	if (retlen < 0)
> +		FAIL("negative name length (%d) for returned property\n",
> +		     retlen);
>  
>  	tag = fdt32_to_cpu(prop->tag);
>  	nameoff = fdt32_to_cpu(prop->nameoff);
> @@ -112,13 +115,16 @@ void check_property(void *fdt, int nodeoffset, const char *name,
>  	propname = fdt_get_string(fdt, nameoff, &namelen);
>  	if (!propname)
>  		FAIL("Couldn't get property name: %s", fdt_strerror(namelen));
> -	if (namelen != strlen(propname))
> +	if (namelen < 0)
> +		FAIL("negative name length (%d) for returned string\n",
> +		     namelen);
> +	if ((unsigned)namelen != strlen(propname))
>  		FAIL("Incorrect prop name length: %d instead of %zd",
>  		     namelen, strlen(propname));
>  	if (!streq(propname, name))
>  		FAIL("Property name mismatch \"%s\" instead of \"%s\"",
>  		     propname, name);

Don't you need to check for retlen < 0 as well?

> -	if (proplen != retlen)
> +	if (proplen != (unsigned)retlen)
>  		FAIL("Length retrieved for \"%s\" by fdt_get_property()"
>  		     " differs from stored length (%d != %d)",
>  		     name, retlen, proplen);

-- 
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 1/8] tests: Fix signedness comparisons warnings

[Andre Przywara]

On Tue, 15 Jun 2021 12:35:00 +1000
David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:

Hi David,

> On Fri, Jun 11, 2021 at 06:10:33PM +0100, Andre Przywara wrote:
> > With -Wsign-compare, compilers warn about a mismatching signedness in
> > comparisons in various files in the tests/ directory.
> > 
> > For about half of the cases we can simply change the signed variable to
> > be of an unsigned type, because they will never need to store negative
> > values (which is the best fix of the problem).
> > 
> > In the remaining cases we can cast the signed variable to an unsigned
> > type, provided we know for sure it is not negative.
> > We see two different scenarios here:
> > - We either just explicitly checked for this variable to be positive
> >   (if (rc < 0) FAIL();), or
> > - We rely on a function returning only positive values in the "length"
> >   pointer if the function returned successfully: which we just checked.
> > 
> > At two occassions we compare with a constant "-1" (even though the
> > variable is unsigned), so we just change this to ~0U to create an
> > unsigned comparison value.
> > 
> > Since this is about the tests, let's also add explicit tests for those
> > values really not being negative.
> > 
> > This fixes "make tests" (but not "make check" yet), when compiled
> > with -Wsign-compare.  
> 
> Thanks for doing this.
> 
> > 
> > Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  tests/dumptrees.c           |  2 +-
> >  tests/fs_tree1.c            |  2 +-
> >  tests/get_name.c            |  4 +++-
> >  tests/integer-expressions.c |  2 +-
> >  tests/nopulate.c            |  3 ++-
> >  tests/overlay.c             |  4 +++-
> >  tests/phandle_format.c      |  2 +-
> >  tests/property_iterate.c    |  2 +-
> >  tests/references.c          |  2 +-
> >  tests/set_name.c            |  6 ++++--
> >  tests/subnode_iterate.c     |  2 +-
> >  tests/tests.h               |  2 +-
> >  tests/testutils.c           | 12 +++++++++---
> >  13 files changed, 29 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tests/dumptrees.c b/tests/dumptrees.c
> > index f1e0ea9..08967b3 100644
> > --- a/tests/dumptrees.c
> > +++ b/tests/dumptrees.c
> > @@ -32,7 +32,7 @@ static struct {
> >  
> >  int main(int argc, char *argv[])
> >  {
> > -	int i;
> > +	unsigned int i;
> >  
> >  	if (argc != 2) {
> >  	    fprintf(stderr, "Missing output directory argument\n");
> > diff --git a/tests/fs_tree1.c b/tests/fs_tree1.c
> > index dff3880..978f6a3 100644
> > --- a/tests/fs_tree1.c
> > +++ b/tests/fs_tree1.c
> > @@ -54,7 +54,7 @@ static void mkfile(const char *name, void *data, size_t len)
> >  	rc = write(fd, data, len);
> >  	if (rc < 0)
> >  		FAIL("write(\"%s\"): %s", name, strerror(errno));
> > -	if (rc != len)
> > +	if ((unsigned)rc != len)
> >  		FAIL("write(\"%s\"): short write", name);
> >  	
> >  	rc = close(fd);
> > diff --git a/tests/get_name.c b/tests/get_name.c
> > index 5a35103..d20bf30 100644
> > --- a/tests/get_name.c
> > +++ b/tests/get_name.c
> > @@ -34,12 +34,14 @@ static void check_name(void *fdt, const char *path)
> >  		       offset, getname, len);
> >  	if (!getname)
> >  		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
> > +	if (len < 0)
> > +		FAIL("negative name length (%d) for returned node name\n", len);
> >  
> >  	if (strcmp(getname, checkname) != 0)
> >  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
> >  		     path, getname, checkname);
> >  
> > -	if (len != strlen(getname))
> > +	if ((unsigned)len != strlen(getname))
> >  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
> >  		     path, len, strlen(getname));
> >  
> > diff --git a/tests/integer-expressions.c b/tests/integer-expressions.c
> > index 6f33d81..2f164d9 100644
> > --- a/tests/integer-expressions.c
> > +++ b/tests/integer-expressions.c
> > @@ -59,7 +59,7 @@ int main(int argc, char *argv[])
> >  	void *fdt;
> >  	const fdt32_t *res;
> >  	int reslen;
> > -	int i;
> > +	unsigned int i;
> >  
> >  	test_init(argc, argv);
> >  
> > diff --git a/tests/nopulate.c b/tests/nopulate.c
> > index 2ae1753..e06a0b3 100644
> > --- a/tests/nopulate.c
> > +++ b/tests/nopulate.c
> > @@ -43,7 +43,8 @@ static int nopulate_struct(char *buf, const char *fdt)
> >  int main(int argc, char *argv[])
> >  {
> >  	char *fdt, *fdt2, *buf;
> > -	int newsize, struct_start, struct_end_old, struct_end_new, delta;
> > +	int newsize, struct_end_old, struct_end_new, delta;
> > +	unsigned int struct_start;  
> 
> Making just one of these variables unsigned looks pretty weird, but I
> guess it works.  The alternative would be to much more strictly check
> the various offsets here - which would also mean that adding the nops
> does take the device tree beyond the allowed size.

TBH this was just the bare minimum mechanical fix, I haven't much looked
into what this test really does.
And there seem to be more issues, for instance we seem to assume
that newsize is non-negative, but nopulate_struct() returns a signed
type. And looking deeper, fdt_next_tag() can return a negative error
value into nextoffset, at which point everything falls apart.

So I guess I will leave this for another rainy afternoon, to not block
this particular patch.

> 
> >  	const char *inname;
> >  	char outname[PATH_MAX];
> >  
> > diff --git a/tests/overlay.c b/tests/overlay.c
> > index 91afa72..b21b28e 100644
> > --- a/tests/overlay.c
> > +++ b/tests/overlay.c
> > @@ -35,7 +35,9 @@ static int fdt_getprop_u32_by_poffset(void *fdt, const char *path,
> >  		return node_off;
> >  
> >  	val = fdt_getprop(fdt, node_off, name, &len);
> > -	if (!val || (len < (sizeof(uint32_t) * (poffset + 1))))
> > +	if (val && len < 0)
> > +		return -FDT_ERR_BADVALUE;  
> 
> This indicates an internal error in libfdt that's more or less
> independent of what this test is really looking for, better to just
> FAIL() here rather than bubble this error up through the caller to
> report.

Done.

> 
> > +	if (!val || ((unsigned)len < (sizeof(uint32_t) * (poffset + 1))))
> >  		return -FDT_ERR_NOTFOUND;  
> 
> NOTFOUND seems kind of dangeous here, because this test catches both
> true NOTFOUND cases (val==NULL && len == NOTFOUND), and broken cases
> (val=NULL && len!=NOTFOUND), best to check the broken case explicitly
> and FAIL().

Right, I fixed that, so that any case of len < 0 is handled before we
come to the comparison.

> >  
> >  	*out = fdt32_to_cpu(*(val + poffset));
> > diff --git a/tests/phandle_format.c b/tests/phandle_format.c
> > index d00618f..0febb32 100644
> > --- a/tests/phandle_format.c
> > +++ b/tests/phandle_format.c
> > @@ -45,7 +45,7 @@ int main(int argc, char *argv[])
> >  		FAIL("fdt_path_offset(/node4): %s", fdt_strerror(n4));
> >  
> >  	h4 = fdt_get_phandle(fdt, n4);
> > -	if ((h4 == 0) || (h4 == -1))
> > +	if ((h4 == 0) || (h4 == ~0U))
> >  		FAIL("/node4 has bad phandle 0x%x\n", h4);
> >  
> >  	if (phandle_format & PHANDLE_LEGACY)
> > diff --git a/tests/property_iterate.c b/tests/property_iterate.c
> > index 9a67f49..0b6af9b 100644
> > --- a/tests/property_iterate.c
> > +++ b/tests/property_iterate.c
> > @@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
> >  	uint32_t properties;
> >  	const fdt32_t *prop;
> >  	int offset, property;
> > -	int count;
> > +	unsigned int count;
> >  	int len;
> >  
> >  	/*
> > diff --git a/tests/references.c b/tests/references.c
> > index d18e722..cb1daaa 100644
> > --- a/tests/references.c
> > +++ b/tests/references.c
> > @@ -106,7 +106,7 @@ int main(int argc, char *argv[])
> >  	if ((h4 == 0x2000) || (h4 == 0x1) || (h4 == 0))
> >  		FAIL("/node4 has bad phandle, 0x%x", h4);
> >  
> > -	if ((h5 == 0) || (h5 == -1))
> > +	if ((h5 == 0) || (h5 == ~0U))
> >  		FAIL("/node5 has bad phandle, 0x%x", h5);
> >  	if ((h5 == h4) || (h5 == h2) || (h5 == h1))
> >  		FAIL("/node5 has duplicate phandle, 0x%x", h5);
> > diff --git a/tests/set_name.c b/tests/set_name.c
> > index a62cb58..5eeb7b9 100644
> > --- a/tests/set_name.c
> > +++ b/tests/set_name.c
> > @@ -39,7 +39,7 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
> >  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
> >  		     path, getname, oldname);
> >  
> 
> > -	if (len != strlen(getname))
> > +	if ((unsigned)len != strlen(getname))  
> 
> AFAICT you haven't actually checked for len < 0 before this, so this
> isn't quite right

True, fixed.

> 
> 
> >  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
> >  		     path, len, strlen(getname));
> >  
> > @@ -51,12 +51,14 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
> >  	getname = fdt_get_name(fdt, offset, &len);
> >  	if (!getname)
> >  		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
> > +	if (len < 0)
> > +		FAIL("negative name length (%d) for returned node name\n", len);
> >  
> >  	if (strcmp(getname, newname) != 0)
> >  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
> >  		     path, getname, newname);
> >  
> > -	if (len != strlen(getname))
> > +	if ((unsigned)len != strlen(getname))
> >  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
> >  		     path, len, strlen(getname));
> >  }
> > diff --git a/tests/subnode_iterate.c b/tests/subnode_iterate.c
> > index 2dc9b2d..2553a51 100644
> > --- a/tests/subnode_iterate.c
> > +++ b/tests/subnode_iterate.c
> > @@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
> >  	uint32_t subnodes;
> >  	const fdt32_t *prop;
> >  	int offset;
> > -	int count;
> > +	unsigned int count;
> >  	int len;
> >  
> >  	/* This property indicates the number of subnodes to expect */
> > diff --git a/tests/tests.h b/tests/tests.h
> > index 1017366..bf8f23c 100644
> > --- a/tests/tests.h
> > +++ b/tests/tests.h
> > @@ -83,7 +83,7 @@ void cleanup(void);
> >  void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size);
> >  
> >  void check_property(void *fdt, int nodeoffset, const char *name,
> > -		    int len, const void *val);
> > +		    unsigned int len, const void *val);
> >  #define check_property_cell(fdt, nodeoffset, name, val) \
> >  	({ \
> >  		fdt32_t x = cpu_to_fdt32(val);			      \
> > diff --git a/tests/testutils.c b/tests/testutils.c
> > index 5e494c5..10129c0 100644
> > --- a/tests/testutils.c
> > +++ b/tests/testutils.c
> > @@ -88,7 +88,7 @@ void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size)
> >  }
> >  
> >  void check_property(void *fdt, int nodeoffset, const char *name,
> > -		    int len, const void *val)
> > +		    unsigned int len, const void *val)
> >  {
> >  	const struct fdt_property *prop;
> >  	int retlen, namelen;
> > @@ -101,6 +101,9 @@ void check_property(void *fdt, int nodeoffset, const char *name,
> >  	if (! prop)
> >  		FAIL("Error retrieving \"%s\" pointer: %s", name,
> >  		     fdt_strerror(retlen));
> > +	if (retlen < 0)
> > +		FAIL("negative name length (%d) for returned property\n",
> > +		     retlen);
> >  
> >  	tag = fdt32_to_cpu(prop->tag);
> >  	nameoff = fdt32_to_cpu(prop->nameoff);
> > @@ -112,13 +115,16 @@ void check_property(void *fdt, int nodeoffset, const char *name,
> >  	propname = fdt_get_string(fdt, nameoff, &namelen);
> >  	if (!propname)
> >  		FAIL("Couldn't get property name: %s", fdt_strerror(namelen));
> > -	if (namelen != strlen(propname))
> > +	if (namelen < 0)
> > +		FAIL("negative name length (%d) for returned string\n",
> > +		     namelen);
> > +	if ((unsigned)namelen != strlen(propname))
> >  		FAIL("Incorrect prop name length: %d instead of %zd",
> >  		     namelen, strlen(propname));
> >  	if (!streq(propname, name))
> >  		FAIL("Property name mismatch \"%s\" instead of \"%s\"",
> >  		     propname, name);  
> 
> Don't you need to check for retlen < 0 as well?

But this is done above, in the previous hunk?

Cheers,
Andre

> 
> > -	if (proplen != retlen)
> > +	if (proplen != (unsigned)retlen)
> >  		FAIL("Length retrieved for \"%s\" by fdt_get_property()"
> >  		     " differs from stored length (%d != %d)",
> >  		     name, retlen, proplen);  
> 

Re: [PATCH v2 1/8] tests: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 18, 2021 at 06:00:36PM +0100, Andre Przywara wrote:
> On Tue, 15 Jun 2021 12:35:00 +1000
> David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> Hi David,
> 
> > On Fri, Jun 11, 2021 at 06:10:33PM +0100, Andre Przywara wrote:
> > > With -Wsign-compare, compilers warn about a mismatching signedness in
> > > comparisons in various files in the tests/ directory.
> > > 
> > > For about half of the cases we can simply change the signed variable to
> > > be of an unsigned type, because they will never need to store negative
> > > values (which is the best fix of the problem).
> > > 
> > > In the remaining cases we can cast the signed variable to an unsigned
> > > type, provided we know for sure it is not negative.
> > > We see two different scenarios here:
> > > - We either just explicitly checked for this variable to be positive
> > >   (if (rc < 0) FAIL();), or
> > > - We rely on a function returning only positive values in the "length"
> > >   pointer if the function returned successfully: which we just checked.
> > > 
> > > At two occassions we compare with a constant "-1" (even though the
> > > variable is unsigned), so we just change this to ~0U to create an
> > > unsigned comparison value.
> > > 
> > > Since this is about the tests, let's also add explicit tests for those
> > > values really not being negative.
> > > 
> > > This fixes "make tests" (but not "make check" yet), when compiled
> > > with -Wsign-compare.  
> > 
> > Thanks for doing this.
> > 
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> > > ---
> > >  tests/dumptrees.c           |  2 +-
> > >  tests/fs_tree1.c            |  2 +-
> > >  tests/get_name.c            |  4 +++-
> > >  tests/integer-expressions.c |  2 +-
> > >  tests/nopulate.c            |  3 ++-
> > >  tests/overlay.c             |  4 +++-
> > >  tests/phandle_format.c      |  2 +-
> > >  tests/property_iterate.c    |  2 +-
> > >  tests/references.c          |  2 +-
> > >  tests/set_name.c            |  6 ++++--
> > >  tests/subnode_iterate.c     |  2 +-
> > >  tests/tests.h               |  2 +-
> > >  tests/testutils.c           | 12 +++++++++---
> > >  13 files changed, 29 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/tests/dumptrees.c b/tests/dumptrees.c
> > > index f1e0ea9..08967b3 100644
> > > --- a/tests/dumptrees.c
> > > +++ b/tests/dumptrees.c
> > > @@ -32,7 +32,7 @@ static struct {
> > >  
> > >  int main(int argc, char *argv[])
> > >  {
> > > -	int i;
> > > +	unsigned int i;
> > >  
> > >  	if (argc != 2) {
> > >  	    fprintf(stderr, "Missing output directory argument\n");
> > > diff --git a/tests/fs_tree1.c b/tests/fs_tree1.c
> > > index dff3880..978f6a3 100644
> > > --- a/tests/fs_tree1.c
> > > +++ b/tests/fs_tree1.c
> > > @@ -54,7 +54,7 @@ static void mkfile(const char *name, void *data, size_t len)
> > >  	rc = write(fd, data, len);
> > >  	if (rc < 0)
> > >  		FAIL("write(\"%s\"): %s", name, strerror(errno));
> > > -	if (rc != len)
> > > +	if ((unsigned)rc != len)
> > >  		FAIL("write(\"%s\"): short write", name);
> > >  	
> > >  	rc = close(fd);
> > > diff --git a/tests/get_name.c b/tests/get_name.c
> > > index 5a35103..d20bf30 100644
> > > --- a/tests/get_name.c
> > > +++ b/tests/get_name.c
> > > @@ -34,12 +34,14 @@ static void check_name(void *fdt, const char *path)
> > >  		       offset, getname, len);
> > >  	if (!getname)
> > >  		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
> > > +	if (len < 0)
> > > +		FAIL("negative name length (%d) for returned node name\n", len);
> > >  
> > >  	if (strcmp(getname, checkname) != 0)
> > >  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
> > >  		     path, getname, checkname);
> > >  
> > > -	if (len != strlen(getname))
> > > +	if ((unsigned)len != strlen(getname))
> > >  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
> > >  		     path, len, strlen(getname));
> > >  
> > > diff --git a/tests/integer-expressions.c b/tests/integer-expressions.c
> > > index 6f33d81..2f164d9 100644
> > > --- a/tests/integer-expressions.c
> > > +++ b/tests/integer-expressions.c
> > > @@ -59,7 +59,7 @@ int main(int argc, char *argv[])
> > >  	void *fdt;
> > >  	const fdt32_t *res;
> > >  	int reslen;
> > > -	int i;
> > > +	unsigned int i;
> > >  
> > >  	test_init(argc, argv);
> > >  
> > > diff --git a/tests/nopulate.c b/tests/nopulate.c
> > > index 2ae1753..e06a0b3 100644
> > > --- a/tests/nopulate.c
> > > +++ b/tests/nopulate.c
> > > @@ -43,7 +43,8 @@ static int nopulate_struct(char *buf, const char *fdt)
> > >  int main(int argc, char *argv[])
> > >  {
> > >  	char *fdt, *fdt2, *buf;
> > > -	int newsize, struct_start, struct_end_old, struct_end_new, delta;
> > > +	int newsize, struct_end_old, struct_end_new, delta;
> > > +	unsigned int struct_start;  
> > 
> > Making just one of these variables unsigned looks pretty weird, but I
> > guess it works.  The alternative would be to much more strictly check
> > the various offsets here - which would also mean that adding the nops
> > does take the device tree beyond the allowed size.
> 
> TBH this was just the bare minimum mechanical fix, I haven't much looked
> into what this test really does.

Fair enough.  Fwiw this test inserts extra "nop" tags into the flat
tree, between every existing tag - this is done in order to then test
that other hings process the nops correctly.

> And there seem to be more issues, for instance we seem to assume
> that newsize is non-negative, but nopulate_struct() returns a signed
> type. And looking deeper, fdt_next_tag() can return a negative error
> value into nextoffset, at which point everything falls apart.
> 
> So I guess I will leave this for another rainy afternoon, to not block
> this particular patch.

Ok.

> > >  	const char *inname;
> > >  	char outname[PATH_MAX];
> > >  
> > > diff --git a/tests/overlay.c b/tests/overlay.c
> > > index 91afa72..b21b28e 100644
> > > --- a/tests/overlay.c
> > > +++ b/tests/overlay.c
> > > @@ -35,7 +35,9 @@ static int fdt_getprop_u32_by_poffset(void *fdt, const char *path,
> > >  		return node_off;
> > >  
> > >  	val = fdt_getprop(fdt, node_off, name, &len);
> > > -	if (!val || (len < (sizeof(uint32_t) * (poffset + 1))))
> > > +	if (val && len < 0)
> > > +		return -FDT_ERR_BADVALUE;  
> > 
> > This indicates an internal error in libfdt that's more or less
> > independent of what this test is really looking for, better to just
> > FAIL() here rather than bubble this error up through the caller to
> > report.
> 
> Done.
> 
> > 
> > > +	if (!val || ((unsigned)len < (sizeof(uint32_t) * (poffset + 1))))
> > >  		return -FDT_ERR_NOTFOUND;  
> > 
> > NOTFOUND seems kind of dangeous here, because this test catches both
> > true NOTFOUND cases (val==NULL && len == NOTFOUND), and broken cases
> > (val=NULL && len!=NOTFOUND), best to check the broken case explicitly
> > and FAIL().
> 
> Right, I fixed that, so that any case of len < 0 is handled before we
> come to the comparison.
> 
> > >  
> > >  	*out = fdt32_to_cpu(*(val + poffset));
> > > diff --git a/tests/phandle_format.c b/tests/phandle_format.c
> > > index d00618f..0febb32 100644
> > > --- a/tests/phandle_format.c
> > > +++ b/tests/phandle_format.c
> > > @@ -45,7 +45,7 @@ int main(int argc, char *argv[])
> > >  		FAIL("fdt_path_offset(/node4): %s", fdt_strerror(n4));
> > >  
> > >  	h4 = fdt_get_phandle(fdt, n4);
> > > -	if ((h4 == 0) || (h4 == -1))
> > > +	if ((h4 == 0) || (h4 == ~0U))
> > >  		FAIL("/node4 has bad phandle 0x%x\n", h4);
> > >  
> > >  	if (phandle_format & PHANDLE_LEGACY)
> > > diff --git a/tests/property_iterate.c b/tests/property_iterate.c
> > > index 9a67f49..0b6af9b 100644
> > > --- a/tests/property_iterate.c
> > > +++ b/tests/property_iterate.c
> > > @@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
> > >  	uint32_t properties;
> > >  	const fdt32_t *prop;
> > >  	int offset, property;
> > > -	int count;
> > > +	unsigned int count;
> > >  	int len;
> > >  
> > >  	/*
> > > diff --git a/tests/references.c b/tests/references.c
> > > index d18e722..cb1daaa 100644
> > > --- a/tests/references.c
> > > +++ b/tests/references.c
> > > @@ -106,7 +106,7 @@ int main(int argc, char *argv[])
> > >  	if ((h4 == 0x2000) || (h4 == 0x1) || (h4 == 0))
> > >  		FAIL("/node4 has bad phandle, 0x%x", h4);
> > >  
> > > -	if ((h5 == 0) || (h5 == -1))
> > > +	if ((h5 == 0) || (h5 == ~0U))
> > >  		FAIL("/node5 has bad phandle, 0x%x", h5);
> > >  	if ((h5 == h4) || (h5 == h2) || (h5 == h1))
> > >  		FAIL("/node5 has duplicate phandle, 0x%x", h5);
> > > diff --git a/tests/set_name.c b/tests/set_name.c
> > > index a62cb58..5eeb7b9 100644
> > > --- a/tests/set_name.c
> > > +++ b/tests/set_name.c
> > > @@ -39,7 +39,7 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
> > >  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
> > >  		     path, getname, oldname);
> > >  
> > 
> > > -	if (len != strlen(getname))
> > > +	if ((unsigned)len != strlen(getname))  
> > 
> > AFAICT you haven't actually checked for len < 0 before this, so this
> > isn't quite right
> 
> True, fixed.
> 
> > 
> > 
> > >  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
> > >  		     path, len, strlen(getname));
> > >  
> > > @@ -51,12 +51,14 @@ static void check_set_name(void *fdt, const char *path, const char *newname)
> > >  	getname = fdt_get_name(fdt, offset, &len);
> > >  	if (!getname)
> > >  		FAIL("fdt_get_name(%d): %s", offset, fdt_strerror(len));
> > > +	if (len < 0)
> > > +		FAIL("negative name length (%d) for returned node name\n", len);
> > >  
> > >  	if (strcmp(getname, newname) != 0)
> > >  		FAIL("fdt_get_name(%s) returned \"%s\" instead of \"%s\"",
> > >  		     path, getname, newname);
> > >  
> > > -	if (len != strlen(getname))
> > > +	if ((unsigned)len != strlen(getname))
> > >  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
> > >  		     path, len, strlen(getname));
> > >  }
> > > diff --git a/tests/subnode_iterate.c b/tests/subnode_iterate.c
> > > index 2dc9b2d..2553a51 100644
> > > --- a/tests/subnode_iterate.c
> > > +++ b/tests/subnode_iterate.c
> > > @@ -23,7 +23,7 @@ static void test_node(void *fdt, int parent_offset)
> > >  	uint32_t subnodes;
> > >  	const fdt32_t *prop;
> > >  	int offset;
> > > -	int count;
> > > +	unsigned int count;
> > >  	int len;
> > >  
> > >  	/* This property indicates the number of subnodes to expect */
> > > diff --git a/tests/tests.h b/tests/tests.h
> > > index 1017366..bf8f23c 100644
> > > --- a/tests/tests.h
> > > +++ b/tests/tests.h
> > > @@ -83,7 +83,7 @@ void cleanup(void);
> > >  void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size);
> > >  
> > >  void check_property(void *fdt, int nodeoffset, const char *name,
> > > -		    int len, const void *val);
> > > +		    unsigned int len, const void *val);
> > >  #define check_property_cell(fdt, nodeoffset, name, val) \
> > >  	({ \
> > >  		fdt32_t x = cpu_to_fdt32(val);			      \
> > > diff --git a/tests/testutils.c b/tests/testutils.c
> > > index 5e494c5..10129c0 100644
> > > --- a/tests/testutils.c
> > > +++ b/tests/testutils.c
> > > @@ -88,7 +88,7 @@ void check_mem_rsv(void *fdt, int n, uint64_t addr, uint64_t size)
> > >  }
> > >  
> > >  void check_property(void *fdt, int nodeoffset, const char *name,
> > > -		    int len, const void *val)
> > > +		    unsigned int len, const void *val)
> > >  {
> > >  	const struct fdt_property *prop;
> > >  	int retlen, namelen;
> > > @@ -101,6 +101,9 @@ void check_property(void *fdt, int nodeoffset, const char *name,
> > >  	if (! prop)
> > >  		FAIL("Error retrieving \"%s\" pointer: %s", name,
> > >  		     fdt_strerror(retlen));
> > > +	if (retlen < 0)
> > > +		FAIL("negative name length (%d) for returned property\n",
> > > +		     retlen);
> > >  
> > >  	tag = fdt32_to_cpu(prop->tag);
> > >  	nameoff = fdt32_to_cpu(prop->nameoff);
> > > @@ -112,13 +115,16 @@ void check_property(void *fdt, int nodeoffset, const char *name,
> > >  	propname = fdt_get_string(fdt, nameoff, &namelen);
> > >  	if (!propname)
> > >  		FAIL("Couldn't get property name: %s", fdt_strerror(namelen));
> > > -	if (namelen != strlen(propname))
> > > +	if (namelen < 0)
> > > +		FAIL("negative name length (%d) for returned string\n",
> > > +		     namelen);
> > > +	if ((unsigned)namelen != strlen(propname))
> > >  		FAIL("Incorrect prop name length: %d instead of %zd",
> > >  		     namelen, strlen(propname));
> > >  	if (!streq(propname, name))
> > >  		FAIL("Property name mismatch \"%s\" instead of \"%s\"",
> > >  		     propname, name);  
> > 
> > Don't you need to check for retlen < 0 as well?
> 
> But this is done above, in the previous hunk?

So it is, sorry.

-- 
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 --]

[PATCH v2 2/8] fdtdump: Fix signedness comparisons warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
comparisons in fdtdump.c.

The "len" parameter to valid_header() refers to a memory size, not a
file offset, so the (unsigned) size_t is better fit, and fixes the
warning nicely.

In the main function we compare the difference between two pointers,
which produces a signed ptrdiff_t type. However the while loop above the
comparison makes sure that "p" always points before "endp" (by virtue of
the limit in the memchr() call). This means "endp - p" is never
negative, so we can safely cast this expression to an unsigned type.

This fixes "make fdtdump", when compiled with -Wsign-compare.

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

diff --git a/fdtdump.c b/fdtdump.c
index d9fb374..bdc0f94 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -18,7 +18,7 @@
 #include "util.h"
 
 #define FDT_MAGIC_SIZE	4
-#define MAX_VERSION 17
+#define MAX_VERSION 17U
 
 #define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
 #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
@@ -163,7 +163,7 @@ static const char * const usage_opts_help[] = {
 	USAGE_COMMON_OPTS_HELP
 };
 
-static bool valid_header(char *p, off_t len)
+static bool valid_header(char *p, size_t len)
 {
 	if (len < sizeof(struct fdt_header) ||
 	    fdt_magic(p) != FDT_MAGIC ||
@@ -235,7 +235,7 @@ int main(int argc, char *argv[])
 			}
 			++p;
 		}
-		if (!p || endp - p < sizeof(struct fdt_header))
+		if (!p || (size_t)(endp - p) < sizeof(struct fdt_header))
 			die("%s: could not locate fdt magic\n", file);
 		printf("%s: found fdt at offset %#tx\n", file, p - buf);
 		buf = p;
-- 
2.17.5

Re: [PATCH v2 2/8] fdtdump: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:34PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in fdtdump.c.
> 
> The "len" parameter to valid_header() refers to a memory size, not a
> file offset, so the (unsigned) size_t is better fit, and fixes the
> warning nicely.
> 
> In the main function we compare the difference between two pointers,
> which produces a signed ptrdiff_t type. However the while loop above the
> comparison makes sure that "p" always points before "endp" (by virtue of
> the limit in the memchr() call). This means "endp - p" is never
> negative, so we can safely cast this expression to an unsigned type.
> 
> This fixes "make fdtdump", when compiled with -Wsign-compare.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  fdtdump.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fdtdump.c b/fdtdump.c
> index d9fb374..bdc0f94 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -18,7 +18,7 @@
>  #include "util.h"
>  
>  #define FDT_MAGIC_SIZE	4
> -#define MAX_VERSION 17
> +#define MAX_VERSION 17U
>  
>  #define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
>  #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
> @@ -163,7 +163,7 @@ static const char * const usage_opts_help[] = {
>  	USAGE_COMMON_OPTS_HELP
>  };
>  
> -static bool valid_header(char *p, off_t len)
> +static bool valid_header(char *p, size_t len)
>  {
>  	if (len < sizeof(struct fdt_header) ||
>  	    fdt_magic(p) != FDT_MAGIC ||
> @@ -235,7 +235,7 @@ int main(int argc, char *argv[])
>  			}
>  			++p;
>  		}
> -		if (!p || endp - p < sizeof(struct fdt_header))
> +		if (!p || (size_t)(endp - p) < sizeof(struct fdt_header))
>  			die("%s: could not locate fdt magic\n", file);
>  		printf("%s: found fdt at offset %#tx\n", file, p - buf);
>  		buf = p;

-- 
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 --]

[PATCH v2 3/8] fdtget: Fix signedness comparisons warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
the different legs of the conditional operator, in fdtget.c.

In the questionable expression, we are constructing a 16-bit value out of
two unsigned 8-bit values, however are relying on the compiler's
automatic expansion of the uint8_t to a larger type, to survive the left
shift. This larger type happens to be an "int", so this part of the
expression becomes signed.

Fix this by explicitly blowing up the uint8_t to a larger *unsigned* type,
before doing the left shift. And while we are at it, convert the hardly
readable conditional operator usage into a sane switch/case expression.

This fixes "make fdtget", when compiled with -Wsign-compare.

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

diff --git a/fdtget.c b/fdtget.c
index 777582e..1f6eb73 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -62,8 +62,14 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
 	for (i = 0; i < len; i += size, p += size) {
 		if (i)
 			printf(" ");
-		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
-			size == 2 ? (*p << 8) | p[1] : *p;
+		switch (size) {
+		case 4: value = fdt32_ld((const fdt32_t *)p); break;
+		case 2: value = ((unsigned)(*p) << 8) | p[1]; break;
+		case 1:
+		default:
+			value = *p;
+			break;
+		}
 		printf(fmt, value);
 	}
 
-- 
2.17.5

Re: [PATCH v2 3/8] fdtget: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:35PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> the different legs of the conditional operator, in fdtget.c.
> 
> In the questionable expression, we are constructing a 16-bit value out of
> two unsigned 8-bit values, however are relying on the compiler's
> automatic expansion of the uint8_t to a larger type, to survive the left
> shift. This larger type happens to be an "int", so this part of the
> expression becomes signed.
> 
> Fix this by explicitly blowing up the uint8_t to a larger *unsigned* type,
> before doing the left shift. And while we are at it, convert the hardly
> readable conditional operator usage into a sane switch/case expression.
> 
> This fixes "make fdtget", when compiled with -Wsign-compare.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Hmm... this code is pretty confusing.  I can't actually see any path
where size will be set to something other than 4, 1 or -1.

The change looks correct, despite that.  Though I wonder if we should
add an fdt16_ld() helper for this case.

> ---
>  fdtget.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fdtget.c b/fdtget.c
> index 777582e..1f6eb73 100644
> --- a/fdtget.c
> +++ b/fdtget.c
> @@ -62,8 +62,14 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
>  	for (i = 0; i < len; i += size, p += size) {
>  		if (i)
>  			printf(" ");
> -		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> -			size == 2 ? (*p << 8) | p[1] : *p;
> +		switch (size) {
> +		case 4: value = fdt32_ld((const fdt32_t *)p); break;
> +		case 2: value = ((unsigned)(*p) << 8) | p[1]; break;
> +		case 1:
> +		default:
> +			value = *p;
> +			break;
> +		}
>  		printf(fmt, value);
>  	}
>  

-- 
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/8] fdtget: Fix signedness comparisons warnings

[Andre Przywara]

On Tue, 15 Jun 2021 12:44:58 +1000
David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:

Hi,

> On Fri, Jun 11, 2021 at 06:10:35PM +0100, Andre Przywara wrote:
> > With -Wsign-compare, compilers warn about a mismatching signedness in
> > the different legs of the conditional operator, in fdtget.c.
> > 
> > In the questionable expression, we are constructing a 16-bit value out of
> > two unsigned 8-bit values, however are relying on the compiler's
> > automatic expansion of the uint8_t to a larger type, to survive the left
> > shift. This larger type happens to be an "int", so this part of the
> > expression becomes signed.
> > 
> > Fix this by explicitly blowing up the uint8_t to a larger *unsigned* type,
> > before doing the left shift. And while we are at it, convert the hardly
> > readable conditional operator usage into a sane switch/case expression.
> > 
> > This fixes "make fdtget", when compiled with -Wsign-compare.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>  
> 
> Hmm... this code is pretty confusing.  I can't actually see any path
> where size will be set to something other than 4, 1 or -1.

If I read correctly, then size==2 could come from the format modifier
"h", as parsed by utilfdt_decode_type().
 
> The change looks correct, despite that.  Though I wonder if we should
> add an fdt16_ld() helper for this case.

Done.

Cheers,
Andre

> 
> > ---
> >  fdtget.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fdtget.c b/fdtget.c
> > index 777582e..1f6eb73 100644
> > --- a/fdtget.c
> > +++ b/fdtget.c
> > @@ -62,8 +62,14 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
> >  	for (i = 0; i < len; i += size, p += size) {
> >  		if (i)
> >  			printf(" ");
> > -		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> > -			size == 2 ? (*p << 8) | p[1] : *p;
> > +		switch (size) {
> > +		case 4: value = fdt32_ld((const fdt32_t *)p); break;
> > +		case 2: value = ((unsigned)(*p) << 8) | p[1]; break;
> > +		case 1:
> > +		default:
> > +			value = *p;
> > +			break;
> > +		}
> >  		printf(fmt, value);
> >  	}
> >    
> 

Re: [PATCH v2 3/8] fdtget: Fix signedness comparisons warnings

[David Gibson]

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

On Tue, Jun 15, 2021 at 10:51:33PM +0100, Andre Przywara wrote:
> On Tue, 15 Jun 2021 12:44:58 +1000
> David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> Hi,
> 
> > On Fri, Jun 11, 2021 at 06:10:35PM +0100, Andre Przywara wrote:
> > > With -Wsign-compare, compilers warn about a mismatching signedness in
> > > the different legs of the conditional operator, in fdtget.c.
> > > 
> > > In the questionable expression, we are constructing a 16-bit value out of
> > > two unsigned 8-bit values, however are relying on the compiler's
> > > automatic expansion of the uint8_t to a larger type, to survive the left
> > > shift. This larger type happens to be an "int", so this part of the
> > > expression becomes signed.
> > > 
> > > Fix this by explicitly blowing up the uint8_t to a larger *unsigned* type,
> > > before doing the left shift. And while we are at it, convert the hardly
> > > readable conditional operator usage into a sane switch/case expression.
> > > 
> > > This fixes "make fdtget", when compiled with -Wsign-compare.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>  
> > 
> > Hmm... this code is pretty confusing.  I can't actually see any path
> > where size will be set to something other than 4, 1 or -1.
> 
> If I read correctly, then size==2 could come from the format modifier
> "h", as parsed by utilfdt_decode_type().

Ah, yes, missed that one.

>  
> > The change looks correct, despite that.  Though I wonder if we should
> > add an fdt16_ld() helper for this case.
> 
> Done.
> 
> Cheers,
> Andre
> 
> > 
> > > ---
> > >  fdtget.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fdtget.c b/fdtget.c
> > > index 777582e..1f6eb73 100644
> > > --- a/fdtget.c
> > > +++ b/fdtget.c
> > > @@ -62,8 +62,14 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
> > >  	for (i = 0; i < len; i += size, p += size) {
> > >  		if (i)
> > >  			printf(" ");
> > > -		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> > > -			size == 2 ? (*p << 8) | p[1] : *p;
> > > +		switch (size) {
> > > +		case 4: value = fdt32_ld((const fdt32_t *)p); break;
> > > +		case 2: value = ((unsigned)(*p) << 8) | p[1]; break;
> > > +		case 1:
> > > +		default:
> > > +			value = *p;
> > > +			break;
> > > +		}
> > >  		printf(fmt, value);
> > >  	}
> > >    
> > 
> 

-- 
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 --]

[PATCH v2 4/8] dtc: Wrap phandle validity check

[Andre Przywara]

In several places we check for a returned phandle value to be valid,
for that it must not be 0 or "-1".

Wrap this check in a static inline function in dtc.h, and use ~0U instead
of -1 on the way, to keep everything in the unsigned realm.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 checks.c   | 10 +++++-----
 dtc.h      |  5 +++++
 livetree.c |  4 ++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/checks.c b/checks.c
index e6c7c3e..67647fa 100644
--- a/checks.c
+++ b/checks.c
@@ -520,7 +520,7 @@ static cell_t check_phandle_prop(struct check *c, struct dt_info *dti,
 
 	phandle = propval_cell(prop);
 
-	if ((phandle == 0) || (phandle == -1)) {
+	if (!fdt_is_valid_phandle(phandle)) {
 		FAIL_PROP(c, dti, node, prop, "bad value (0x%x) in %s property",
 		     phandle, prop->name);
 		return 0;
@@ -1400,14 +1400,14 @@ static void check_property_phandle_args(struct check *c,
 	for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
 		struct node *provider_node;
 		struct property *cellprop;
-		int phandle;
+		cell_t phandle;
 
 		phandle = propval_cell_n(prop, cell);
 		/*
 		 * Some bindings use a cell value 0 or -1 to skip over optional
 		 * entries when each index position has a specific definition.
 		 */
-		if (phandle == 0 || phandle == -1) {
+		if (!fdt_is_valid_phandle(phandle)) {
 			/* Give up if this is an overlay with external references */
 			if (dti->dtsflags & DTSF_PLUGIN)
 				break;
@@ -1615,7 +1615,7 @@ static void check_interrupts_property(struct check *c,
 		prop = get_property(parent, "interrupt-parent");
 		if (prop) {
 			phandle = propval_cell(prop);
-			if ((phandle == 0) || (phandle == -1)) {
+			if (!fdt_is_valid_phandle(phandle)) {
 				/* Give up if this is an overlay with
 				 * external references */
 				if (dti->dtsflags & DTSF_PLUGIN)
@@ -1772,7 +1772,7 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
 
 	phandle = propval_cell(prop);
 	/* Give up if this is an overlay with external references */
-	if (phandle == 0 || phandle == -1)
+	if (!fdt_is_valid_phandle(phandle))
 		return NULL;
 
 	node = get_node_by_phandle(dti->dt, phandle);
diff --git a/dtc.h b/dtc.h
index 6296361..3357300 100644
--- a/dtc.h
+++ b/dtc.h
@@ -51,6 +51,11 @@ extern int annotate;		/* annotate .dts with input source location */
 
 typedef uint32_t cell_t;
 
+static inline bool fdt_is_valid_phandle(cell_t phandle)
+{
+	return phandle != 0 && phandle != ~0U;
+}
+
 static inline uint16_t dtb_ld16(const void *p)
 {
 	const uint8_t *bp = (const uint8_t *)p;
diff --git a/livetree.c b/livetree.c
index 7eacd02..cf2f63d 100644
--- a/livetree.c
+++ b/livetree.c
@@ -559,7 +559,7 @@ struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
 {
 	struct node *child, *node;
 
-	if ((phandle == 0) || (phandle == -1)) {
+	if (!fdt_is_valid_phandle(phandle)) {
 		assert(generate_fixups);
 		return NULL;
 	}
@@ -594,7 +594,7 @@ cell_t get_node_phandle(struct node *root, struct node *node)
 	static cell_t phandle = 1; /* FIXME: ick, static local */
 	struct data d = empty_data;
 
-	if ((node->phandle != 0) && (node->phandle != -1))
+	if (fdt_is_valid_phandle(node->phandle))
 		return node->phandle;
 
 	while (get_node_by_phandle(root, phandle))
-- 
2.17.5

Re: [PATCH v2 4/8] dtc: Wrap phandle validity check

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:36PM +0100, Andre Przywara wrote:
> In several places we check for a returned phandle value to be valid,
> for that it must not be 0 or "-1".
> 
> Wrap this check in a static inline function in dtc.h, and use ~0U instead
> of -1 on the way, to keep everything in the unsigned realm.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

This is fine, except for naming.  Because this is a dtc internal
function - and it's not even operating on *flat* tree at this point,
the fdt prefix is unhelpeful.  Just call it phandle_is_valid().

> ---
>  checks.c   | 10 +++++-----
>  dtc.h      |  5 +++++
>  livetree.c |  4 ++--
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index e6c7c3e..67647fa 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -520,7 +520,7 @@ static cell_t check_phandle_prop(struct check *c, struct dt_info *dti,
>  
>  	phandle = propval_cell(prop);
>  
> -	if ((phandle == 0) || (phandle == -1)) {
> +	if (!fdt_is_valid_phandle(phandle)) {
>  		FAIL_PROP(c, dti, node, prop, "bad value (0x%x) in %s property",
>  		     phandle, prop->name);
>  		return 0;
> @@ -1400,14 +1400,14 @@ static void check_property_phandle_args(struct check *c,
>  	for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
>  		struct node *provider_node;
>  		struct property *cellprop;
> -		int phandle;
> +		cell_t phandle;
>  
>  		phandle = propval_cell_n(prop, cell);
>  		/*
>  		 * Some bindings use a cell value 0 or -1 to skip over optional
>  		 * entries when each index position has a specific definition.
>  		 */
> -		if (phandle == 0 || phandle == -1) {
> +		if (!fdt_is_valid_phandle(phandle)) {
>  			/* Give up if this is an overlay with external references */
>  			if (dti->dtsflags & DTSF_PLUGIN)
>  				break;
> @@ -1615,7 +1615,7 @@ static void check_interrupts_property(struct check *c,
>  		prop = get_property(parent, "interrupt-parent");
>  		if (prop) {
>  			phandle = propval_cell(prop);
> -			if ((phandle == 0) || (phandle == -1)) {
> +			if (!fdt_is_valid_phandle(phandle)) {
>  				/* Give up if this is an overlay with
>  				 * external references */
>  				if (dti->dtsflags & DTSF_PLUGIN)
> @@ -1772,7 +1772,7 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
>  
>  	phandle = propval_cell(prop);
>  	/* Give up if this is an overlay with external references */
> -	if (phandle == 0 || phandle == -1)
> +	if (!fdt_is_valid_phandle(phandle))
>  		return NULL;
>  
>  	node = get_node_by_phandle(dti->dt, phandle);
> diff --git a/dtc.h b/dtc.h
> index 6296361..3357300 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -51,6 +51,11 @@ extern int annotate;		/* annotate .dts with input source location */
>  
>  typedef uint32_t cell_t;
>  
> +static inline bool fdt_is_valid_phandle(cell_t phandle)
> +{
> +	return phandle != 0 && phandle != ~0U;
> +}
> +
>  static inline uint16_t dtb_ld16(const void *p)
>  {
>  	const uint8_t *bp = (const uint8_t *)p;
> diff --git a/livetree.c b/livetree.c
> index 7eacd02..cf2f63d 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -559,7 +559,7 @@ struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
>  {
>  	struct node *child, *node;
>  
> -	if ((phandle == 0) || (phandle == -1)) {
> +	if (!fdt_is_valid_phandle(phandle)) {
>  		assert(generate_fixups);
>  		return NULL;
>  	}
> @@ -594,7 +594,7 @@ cell_t get_node_phandle(struct node *root, struct node *node)
>  	static cell_t phandle = 1; /* FIXME: ick, static local */
>  	struct data d = empty_data;
>  
> -	if ((node->phandle != 0) && (node->phandle != -1))
> +	if (fdt_is_valid_phandle(node->phandle))
>  		return node->phandle;
>  
>  	while (get_node_by_phandle(root, phandle))

-- 
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 --]

[PATCH v2 5/8] dtc: Fix signedness comparisons warnings: reservednum

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in comparisons in code using the "reservednum" variable.

There is obviously little sense in having a negative number of reserved
memory entries, so let's make this variable and all its users unsigned.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 dtc.c      | 4 ++--
 dtc.h      | 2 +-
 flattree.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/dtc.c b/dtc.c
index 3962d3f..bc786c5 100644
--- a/dtc.c
+++ b/dtc.c
@@ -12,7 +12,7 @@
  * Command line options
  */
 int quiet;		/* Level of quietness */
-int reservenum;		/* Number of memory reservation slots */
+unsigned int reservenum;/* Number of memory reservation slots */
 int minsize;		/* Minimum blob size */
 int padsize;		/* Additional padding to blob */
 int alignsize;		/* Additional padding to blob accroding to the alignsize */
@@ -197,7 +197,7 @@ int main(int argc, char *argv[])
 			depname = optarg;
 			break;
 		case 'R':
-			reservenum = strtol(optarg, NULL, 0);
+			reservenum = strtoul(optarg, NULL, 0);
 			break;
 		case 'S':
 			minsize = strtol(optarg, NULL, 0);
diff --git a/dtc.h b/dtc.h
index 3357300..0ceeeba 100644
--- a/dtc.h
+++ b/dtc.h
@@ -35,7 +35,7 @@
  * Command line options
  */
 extern int quiet;		/* Level of quietness */
-extern int reservenum;		/* Number of memory reservation slots */
+extern unsigned int reservenum;	/* Number of memory reservation slots */
 extern int minsize;		/* Minimum blob size */
 extern int padsize;		/* Additional padding to blob */
 extern int alignsize;		/* Additional padding to blob accroding to the alignsize */
diff --git a/flattree.c b/flattree.c
index 4659afb..3d0204f 100644
--- a/flattree.c
+++ b/flattree.c
@@ -295,7 +295,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
 {
 	struct reserve_info *re;
 	struct data d = empty_data;
-	int    j;
+	unsigned int j;
 
 	for (re = reservelist; re; re = re->next) {
 		d = data_append_re(d, re->address, re->size);
-- 
2.17.5

Re: [PATCH v2 5/8] dtc: Fix signedness comparisons warnings: reservednum

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:37PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in comparisons in code using the "reservednum" variable.
> 
> There is obviously little sense in having a negative number of reserved
> memory entries, so let's make this variable and all its users unsigned.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, since it definitely improves things..

> ---
>  dtc.c      | 4 ++--
>  dtc.h      | 2 +-
>  flattree.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index 3962d3f..bc786c5 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -12,7 +12,7 @@
>   * Command line options
>   */
>  int quiet;		/* Level of quietness */
> -int reservenum;		/* Number of memory reservation slots */
> +unsigned int reservenum;/* Number of memory reservation slots */
>  int minsize;		/* Minimum blob size */
>  int padsize;		/* Additional padding to blob */
>  int alignsize;		/* Additional padding to blob accroding to the alignsize */
> @@ -197,7 +197,7 @@ int main(int argc, char *argv[])
>  			depname = optarg;
>  			break;
>  		case 'R':
> -			reservenum = strtol(optarg, NULL, 0);
> +			reservenum = strtoul(optarg, NULL, 0);

,, we really should be checking for parse failures from strtoul,
though :/.

>  			break;
>  		case 'S':
>  			minsize = strtol(optarg, NULL, 0);
> diff --git a/dtc.h b/dtc.h
> index 3357300..0ceeeba 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -35,7 +35,7 @@
>   * Command line options
>   */
>  extern int quiet;		/* Level of quietness */
> -extern int reservenum;		/* Number of memory reservation slots */
> +extern unsigned int reservenum;	/* Number of memory reservation slots */
>  extern int minsize;		/* Minimum blob size */
>  extern int padsize;		/* Additional padding to blob */
>  extern int alignsize;		/* Additional padding to blob accroding to the alignsize */
> diff --git a/flattree.c b/flattree.c
> index 4659afb..3d0204f 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -295,7 +295,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
>  {
>  	struct reserve_info *re;
>  	struct data d = empty_data;
> -	int    j;
> +	unsigned int j;
>  
>  	for (re = reservelist; re; re = re->next) {
>  		d = data_append_re(d, re->address, re->size);

-- 
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 --]

[PATCH v2 6/8] dtc: Fix signedness comparisons warnings: pointer diff

[Andre Przywara]

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

Taking the difference between two pointers results in a signed ptrdiff_t
type, which mismatches the unsigned type returned by strlen().
Since "p" has been returned by a call to strchr() with "path" as its
argument, we know for sure that it's bigger than "path", so the
difference must be positive. So a cast to an unsigned type is valid.

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

diff --git a/livetree.c b/livetree.c
index cf2f63d..ef141a0 100644
--- a/livetree.c
+++ b/livetree.c
@@ -526,7 +526,7 @@ struct node *get_node_by_path(struct node *tree, const char *path)
 	p = strchr(path, '/');
 
 	for_each_child(tree, child) {
-		if (p && strprefixeq(path, p - path, child->name))
+		if (p && strprefixeq(path, (size_t)(p - path), child->name))
 			return get_node_by_path(child, p+1);
 		else if (!p && streq(path, child->name))
 			return child;
-- 
2.17.5

Re: [PATCH v2 6/8] dtc: Fix signedness comparisons warnings: pointer diff

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:38PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in comparisons in the function get_node_by_path().
> 
> Taking the difference between two pointers results in a signed ptrdiff_t
> type, which mismatches the unsigned type returned by strlen().
> Since "p" has been returned by a call to strchr() with "path" as its
> argument, we know for sure that it's bigger than "path", so the
> difference must be positive. So a cast to an unsigned type is valid.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  livetree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/livetree.c b/livetree.c
> index cf2f63d..ef141a0 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -526,7 +526,7 @@ struct node *get_node_by_path(struct node *tree, const char *path)
>  	p = strchr(path, '/');
>  
>  	for_each_child(tree, child) {
> -		if (p && strprefixeq(path, p - path, child->name))
> +		if (p && strprefixeq(path, (size_t)(p - path), child->name))
>  			return get_node_by_path(child, p+1);
>  		else if (!p && streq(path, child->name))
>  			return child;

-- 
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 --]

[PATCH v2 7/8] checks: Fix signedness comparisons warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
comparisons in various parts in checks.c.

Fix those by making all affected variables unsigned. This covers return
values of the (unsigned) size_t type, phandles, variables holding sizes
in general and loop counters only ever counting positives values.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 checks.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/checks.c b/checks.c
index 67647fa..e8bfe4a 100644
--- a/checks.c
+++ b/checks.c
@@ -312,7 +312,7 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
 static void check_node_name_chars(struct check *c, struct dt_info *dti,
 				  struct node *node)
 {
-	int n = strspn(node->name, c->data);
+	size_t n = strspn(node->name, c->data);
 
 	if (n < strlen(node->name))
 		FAIL(c, dti, node, "Bad character '%c' in node name",
@@ -386,7 +386,7 @@ static void check_property_name_chars(struct check *c, struct dt_info *dti,
 	struct property *prop;
 
 	for_each_property(node, prop) {
-		int n = strspn(prop->name, c->data);
+		size_t n = strspn(prop->name, c->data);
 
 		if (n < strlen(prop->name))
 			FAIL_PROP(c, dti, node, prop, "Bad character '%c' in property name",
@@ -403,7 +403,7 @@ static void check_property_name_chars_strict(struct check *c,
 
 	for_each_property(node, prop) {
 		const char *name = prop->name;
-		int n = strspn(name, c->data);
+		size_t n = strspn(name, c->data);
 
 		if (n == strlen(prop->name))
 			continue;
@@ -579,7 +579,7 @@ static void check_name_properties(struct check *c, struct dt_info *dti,
 	if (!prop)
 		return; /* No name property, that's fine */
 
-	if ((prop->val.len != node->basenamelen+1)
+	if ((prop->val.len != node->basenamelen + 1U)
 	    || (memcmp(prop->val.val, node->name, node->basenamelen) != 0)) {
 		FAIL(c, dti, node, "\"name\" property is incorrect (\"%s\" instead"
 		     " of base node name)", prop->val.val);
@@ -1388,7 +1388,7 @@ static void check_property_phandle_args(struct check *c,
 				          const struct provider *provider)
 {
 	struct node *root = dti->dt;
-	int cell, cellsize = 0;
+	unsigned int cell, cellsize = 0;
 
 	if (!is_multiple_of(prop->val.len, sizeof(cell_t))) {
 		FAIL_PROP(c, dti, node, prop,
@@ -1596,7 +1596,8 @@ static void check_interrupts_property(struct check *c,
 	struct node *root = dti->dt;
 	struct node *irq_node = NULL, *parent = node;
 	struct property *irq_prop, *prop = NULL;
-	int irq_cells, phandle;
+	int irq_cells;
+	cell_t phandle;
 
 	irq_prop = get_property(node, "interrupts");
 	if (!irq_prop)
@@ -1762,7 +1763,7 @@ WARNING(graph_port, check_graph_port, NULL, &graph_nodes);
 static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
 					struct node *endpoint)
 {
-	int phandle;
+	cell_t phandle;
 	struct node *node;
 	struct property *prop;
 
@@ -1910,7 +1911,7 @@ static void enable_warning_error(struct check *c, bool warn, bool error)
 
 static void disable_warning_error(struct check *c, bool warn, bool error)
 {
-	int i;
+	unsigned int i;
 
 	/* Lowering level, also lower it for things this is the prereq
 	 * for */
@@ -1931,7 +1932,7 @@ static void disable_warning_error(struct check *c, bool warn, bool error)
 
 void parse_checks_option(bool warn, bool error, const char *arg)
 {
-	int i;
+	unsigned int i;
 	const char *name = arg;
 	bool enable = true;
 
@@ -1958,7 +1959,7 @@ void parse_checks_option(bool warn, bool error, const char *arg)
 
 void process_checks(bool force, struct dt_info *dti)
 {
-	int i;
+	unsigned int i;
 	int error = 0;
 
 	for (i = 0; i < ARRAY_SIZE(check_table); i++) {
-- 
2.17.5

Re: [PATCH v2 7/8] checks: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:39PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in various parts in checks.c.
> 
> Fix those by making all affected variables unsigned. This covers return
> values of the (unsigned) size_t type, phandles, variables holding sizes
> in general and loop counters only ever counting positives values.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  checks.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 67647fa..e8bfe4a 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -312,7 +312,7 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
>  static void check_node_name_chars(struct check *c, struct dt_info *dti,
>  				  struct node *node)
>  {
> -	int n = strspn(node->name, c->data);
> +	size_t n = strspn(node->name, c->data);
>  
>  	if (n < strlen(node->name))
>  		FAIL(c, dti, node, "Bad character '%c' in node name",
> @@ -386,7 +386,7 @@ static void check_property_name_chars(struct check *c, struct dt_info *dti,
>  	struct property *prop;
>  
>  	for_each_property(node, prop) {
> -		int n = strspn(prop->name, c->data);
> +		size_t n = strspn(prop->name, c->data);
>  
>  		if (n < strlen(prop->name))
>  			FAIL_PROP(c, dti, node, prop, "Bad character '%c' in property name",
> @@ -403,7 +403,7 @@ static void check_property_name_chars_strict(struct check *c,
>  
>  	for_each_property(node, prop) {
>  		const char *name = prop->name;
> -		int n = strspn(name, c->data);
> +		size_t n = strspn(name, c->data);
>  
>  		if (n == strlen(prop->name))
>  			continue;
> @@ -579,7 +579,7 @@ static void check_name_properties(struct check *c, struct dt_info *dti,
>  	if (!prop)
>  		return; /* No name property, that's fine */
>  
> -	if ((prop->val.len != node->basenamelen+1)
> +	if ((prop->val.len != node->basenamelen + 1U)
>  	    || (memcmp(prop->val.val, node->name, node->basenamelen) != 0)) {
>  		FAIL(c, dti, node, "\"name\" property is incorrect (\"%s\" instead"
>  		     " of base node name)", prop->val.val);
> @@ -1388,7 +1388,7 @@ static void check_property_phandle_args(struct check *c,
>  				          const struct provider *provider)
>  {
>  	struct node *root = dti->dt;
> -	int cell, cellsize = 0;
> +	unsigned int cell, cellsize = 0;
>  
>  	if (!is_multiple_of(prop->val.len, sizeof(cell_t))) {
>  		FAIL_PROP(c, dti, node, prop,
> @@ -1596,7 +1596,8 @@ static void check_interrupts_property(struct check *c,
>  	struct node *root = dti->dt;
>  	struct node *irq_node = NULL, *parent = node;
>  	struct property *irq_prop, *prop = NULL;
> -	int irq_cells, phandle;
> +	int irq_cells;
> +	cell_t phandle;

The compiler doesn't complain about it, but irq_cells should logically
be a cell_t as well, might as well fix that at the same time.

>  
>  	irq_prop = get_property(node, "interrupts");
>  	if (!irq_prop)
> @@ -1762,7 +1763,7 @@ WARNING(graph_port, check_graph_port, NULL, &graph_nodes);
>  static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
>  					struct node *endpoint)
>  {
> -	int phandle;
> +	cell_t phandle;
>  	struct node *node;
>  	struct property *prop;
>  
> @@ -1910,7 +1911,7 @@ static void enable_warning_error(struct check *c, bool warn, bool error)
>  
>  static void disable_warning_error(struct check *c, bool warn, bool error)
>  {
> -	int i;
> +	unsigned int i;
>  
>  	/* Lowering level, also lower it for things this is the prereq
>  	 * for */
> @@ -1931,7 +1932,7 @@ static void disable_warning_error(struct check *c, bool warn, bool error)
>  
>  void parse_checks_option(bool warn, bool error, const char *arg)
>  {
> -	int i;
> +	unsigned int i;
>  	const char *name = arg;
>  	bool enable = true;
>  
> @@ -1958,7 +1959,7 @@ void parse_checks_option(bool warn, bool error, const char *arg)
>  
>  void process_checks(bool force, struct dt_info *dti)
>  {
> -	int i;
> +	unsigned int i;
>  	int error = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(check_table); i++) {

-- 
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 --]

[PATCH v2 8/8] Makefile: add -Wsign-compare to warning options

[Andre Przywara]

Now that all signedness comparison warnings in the source tree have been
fixed, let's enable the warning option, to avoid them creeping in again.

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

diff --git a/Makefile b/Makefile
index ea8c659..ee77115 100644
--- a/Makefile
+++ b/Makefile
@@ -21,7 +21,7 @@ CONFIG_LOCALVERSION =
 ASSUME_MASK ?= 0
 
 CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
-WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
+WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs -Wsign-compare \
 	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
 CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
 
-- 
2.17.5

Re: [PATCH v2 0/8] dtc: Fix signed/unsigned comparison warnings

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:10:32PM +0100, Andre Przywara wrote:
> Hi,
> 
> sorry for dropping the ball on this, but here in an update for the half
> finished series from last year to fix signedness comparison warnings in
> the tools (dtc/tests) - the libfdt fixes were already merged.
> I addressed all (but one) of David's comments back from October, hopefully
> this is good to go now.

Thanks for doing this.  LGTM overall, there are a few little nits in
implementation which I've commented on.

-- 
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 --]