[PATCH 00/11] dtc: Fix signed/unsigned comparison warnings

[PATCH 00/11] dtc: Fix signed/unsigned comparison warnings

[Andre Przywara]

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 (11):
  tests: Fix signedness comparisons warnings
  convert-dtsv0: Fix signedness comparisons warning
  fdtdump: Fix signedness comparisons warnings
  fdtget: Fix signedness comparisons warnings
  dtc: Wrap phandle validity check
  dtc: Fix signedness comparisons warnings: change types
  dtc: Fix signedness comparisons warnings: reservednum
  dtc: Fix signedness comparisons warnings: Wrap (-1)
  dtc: Fix signedness comparisons warnings: pointer diff
  checks: Fix signedness comparisons warnings
  Makefile: add -Wsign-compare to warning options

 Makefile                    |  2 +-
 checks.c                    | 31 ++++++++++++++++---------------
 convert-dtsv0-lexer.l       |  2 +-
 data.c                      |  6 +++---
 dtc.c                       |  2 +-
 dtc.h                       | 12 +++++++-----
 fdtdump.c                   |  6 +++---
 fdtget.c                    |  4 ++--
 flattree.c                  | 10 +++++-----
 livetree.c                  |  8 ++++----
 tests/dumptrees.c           |  2 +-
 tests/fs_tree1.c            |  2 +-
 tests/get_name.c            |  2 +-
 tests/integer-expressions.c |  2 +-
 tests/nopulate.c            |  3 ++-
 tests/overlay.c             |  2 +-
 tests/phandle_format.c      |  2 +-
 tests/property_iterate.c    |  2 +-
 tests/references.c          |  2 +-
 tests/set_name.c            |  4 ++--
 tests/subnode_iterate.c     |  2 +-
 tests/tests.h               |  2 +-
 tests/testutils.c           |  6 +++---
 yamltree.c                  |  6 +++---
 24 files changed, 63 insertions(+), 59 deletions(-)

-- 
2.17.5

[PATCH 01/11] 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.

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            | 2 +-
 tests/integer-expressions.c | 2 +-
 tests/nopulate.c            | 3 ++-
 tests/overlay.c             | 2 +-
 tests/phandle_format.c      | 2 +-
 tests/property_iterate.c    | 2 +-
 tests/references.c          | 2 +-
 tests/set_name.c            | 4 ++--
 tests/subnode_iterate.c     | 2 +-
 tests/tests.h               | 2 +-
 tests/testutils.c           | 6 +++---
 13 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/tests/dumptrees.c b/tests/dumptrees.c
index aecb326..49ef525 100644
--- a/tests/dumptrees.c
+++ b/tests/dumptrees.c
@@ -30,7 +30,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..7b757ec 100644
--- a/tests/get_name.c
+++ b/tests/get_name.c
@@ -39,7 +39,7 @@ static void check_name(void *fdt, const char *path)
 		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..c96f6f2 100644
--- a/tests/overlay.c
+++ b/tests/overlay.c
@@ -35,7 +35,7 @@ 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 || ((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..cd2b9aa 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));
 
@@ -56,7 +56,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, 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..ff215fc 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;
@@ -112,13 +112,13 @@ 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 ((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

[PATCH 02/11] convert-dtsv0: Fix signedness comparisons warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
comparisons in the generated lexer code.

In this case we walk over an array, and never use negative indicies, so
we can change the loop counter variable to be unsigned.

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

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

diff --git a/convert-dtsv0-lexer.l b/convert-dtsv0-lexer.l
index f52e8a1..7a66f57 100644
--- a/convert-dtsv0-lexer.l
+++ b/convert-dtsv0-lexer.l
@@ -94,7 +94,7 @@ static const struct {
 <INITIAL>[0-9a-fA-F]+	{
 			unsigned long long val;
 			int obase = 16, width = 0;
-			int i;
+			unsigned int i;
 
 			val = strtoull(yytext, NULL, cbase);
 
-- 
2.17.5

[PATCH 03/11] 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 9613bef..ec67f99 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 || (unsigned)(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

[PATCH 04/11] 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 blow up the uint8_t to a larger *unsigned* type,
before doing the left shift. That keeps all parts of the conditional
operator "unsigned", and prevents the original warning.

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

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

diff --git a/fdtget.c b/fdtget.c
index 777582e..c5685d9 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -62,8 +62,8 @@ 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;
+		value = (size == 4) ? fdt32_ld((const fdt32_t *)p) :
+			((size == 2) ? ((unsigned)(*p) << 8) | p[1] : *p);
 		printf(fmt, value);
 	}
 
-- 
2.17.5

[PATCH 05/11] 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 macro 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      |  2 ++
 livetree.c |  4 ++--
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/checks.c b/checks.c
index 17cb689..7cb9011 100644
--- a/checks.c
+++ b/checks.c
@@ -497,7 +497,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_VALID_PHANDLE(phandle)) {
 		FAIL_PROP(c, dti, node, prop, "bad value (0x%x) in %s property",
 		     phandle, prop->name);
 		return 0;
@@ -1379,14 +1379,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_VALID_PHANDLE(phandle)) {
 			/* Give up if this is an overlay with external references */
 			if (dti->dtsflags & DTSF_PLUGIN)
 				break;
@@ -1603,7 +1603,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_VALID_PHANDLE(phandle)) {
 				/* Give up if this is an overlay with
 				 * external references */
 				if (dti->dtsflags & DTSF_PLUGIN)
@@ -1760,7 +1760,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_VALID_PHANDLE(phandle))
 		return NULL;
 
 	node = get_node_by_phandle(dti->dt, phandle);
diff --git a/dtc.h b/dtc.h
index a08f415..f2f96ec 100644
--- a/dtc.h
+++ b/dtc.h
@@ -49,6 +49,8 @@ extern int annotate;		/* annotate .dts with input source location */
 #define PHANDLE_EPAPR	0x2
 #define PHANDLE_BOTH	0x3
 
+#define FDT_VALID_PHANDLE(x)	((x) != 0 && (x) != ~0U)
+
 typedef uint32_t cell_t;
 
 static inline uint16_t dtb_ld16(const void *p)
diff --git a/livetree.c b/livetree.c
index 032df58..aea6095 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_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_VALID_PHANDLE(node->phandle))
 		return node->phandle;
 
 	while (get_node_by_phandle(root, phandle))
-- 
2.17.5

[PATCH 06/11] dtc: Fix signedness comparisons warnings: change types

[Andre Przywara]

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

Many variables are using signed types unnecessarily, as we never use
negative value in them.
Change their types to be unsigned, to prevent issues with comparisons.

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

diff --git a/data.c b/data.c
index 0a43b6d..fdb0407 100644
--- a/data.c
+++ b/data.c
@@ -21,10 +21,10 @@ void data_free(struct data d)
 		free(d.val);
 }
 
-struct data data_grow_for(struct data d, int xlen)
+struct data data_grow_for(struct data d, unsigned int xlen)
 {
 	struct data nd;
-	int newsize;
+	unsigned int newsize;
 
 	if (xlen == 0)
 		return d;
diff --git a/dtc.h b/dtc.h
index f2f96ec..511dda1 100644
--- a/dtc.h
+++ b/dtc.h
@@ -107,13 +107,13 @@ extern const char *markername(enum markertype markertype);
 
 struct  marker {
 	enum markertype type;
-	int offset;
+	unsigned int offset;
 	char *ref;
 	struct marker *next;
 };
 
 struct data {
-	int len;
+	unsigned int len;
 	char *val;
 	struct marker *markers;
 };
@@ -131,7 +131,7 @@ size_t type_marker_length(struct marker *m);
 
 void data_free(struct data d);
 
-struct data data_grow_for(struct data d, int xlen);
+struct data data_grow_for(struct data d, unsigned int xlen);
 
 struct data data_copy_mem(const char *mem, int len);
 struct data data_copy_escape_string(const char *s, int len);
@@ -255,7 +255,7 @@ void append_to_property(struct node *node,
 const char *get_unitname(struct node *node);
 struct property *get_property(struct node *node, const char *propname);
 cell_t propval_cell(struct property *prop);
-cell_t propval_cell_n(struct property *prop, int n);
+cell_t propval_cell_n(struct property *prop, unsigned int n);
 struct property *get_property_by_label(struct node *tree, const char *label,
 				       struct node **node);
 struct marker *get_marker_label(struct node *tree, const char *label,
diff --git a/flattree.c b/flattree.c
index 07f10d2..4659afb 100644
--- a/flattree.c
+++ b/flattree.c
@@ -149,7 +149,7 @@ static void asm_emit_align(void *e, int a)
 static void asm_emit_data(void *e, struct data d)
 {
 	FILE *f = e;
-	int off = 0;
+	unsigned int off = 0;
 	struct marker *m = d.markers;
 
 	for_each_marker_of_type(m, LABEL)
@@ -219,7 +219,7 @@ static struct emitter asm_emitter = {
 
 static int stringtable_insert(struct data *d, const char *str)
 {
-	int i;
+	unsigned int i;
 
 	/* FIXME: do this more efficiently? */
 
@@ -345,7 +345,7 @@ static void make_fdt_header(struct fdt_header *fdt,
 void dt_to_blob(FILE *f, struct dt_info *dti, int version)
 {
 	struct version_info *vi = NULL;
-	int i;
+	unsigned int i;
 	struct data blob       = empty_data;
 	struct data reservebuf = empty_data;
 	struct data dtbuf      = empty_data;
@@ -446,7 +446,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
 void dt_to_asm(FILE *f, struct dt_info *dti, int version)
 {
 	struct version_info *vi = NULL;
-	int i;
+	unsigned int i;
 	struct data strbuf = empty_data;
 	struct reserve_info *re;
 	const char *symprefix = "dt";
diff --git a/livetree.c b/livetree.c
index aea6095..8393637 100644
--- a/livetree.c
+++ b/livetree.c
@@ -438,7 +438,7 @@ cell_t propval_cell(struct property *prop)
 	return fdt32_to_cpu(*((fdt32_t *)prop->val.val));
 }
 
-cell_t propval_cell_n(struct property *prop, int n)
+cell_t propval_cell_n(struct property *prop, unsigned int n)
 {
 	assert(prop->val.len / sizeof(cell_t) >= n);
 	return fdt32_to_cpu(*((fdt32_t *)prop->val.val + n));
diff --git a/yamltree.c b/yamltree.c
index 4e93c12..e63d32f 100644
--- a/yamltree.c
+++ b/yamltree.c
@@ -29,11 +29,11 @@ char *yaml_error_name[] = {
 		    (emitter)->problem, __func__, __LINE__);		\
 })
 
-static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, char *data, int len, int width)
+static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, char *data, unsigned int len, int width)
 {
 	yaml_event_t event;
 	void *tag;
-	int off, start_offset = markers->offset;
+	unsigned int off, start_offset = markers->offset;
 
 	switch(width) {
 		case 1: tag = "!u8"; break;
@@ -112,7 +112,7 @@ static void yaml_propval_string(yaml_emitter_t *emitter, char *str, int len)
 static void yaml_propval(yaml_emitter_t *emitter, struct property *prop)
 {
 	yaml_event_t event;
-	int len = prop->val.len;
+	unsigned int len = prop->val.len;
 	struct marker *m = prop->val.markers;
 
 	/* Emit the property name */
-- 
2.17.5

[PATCH 07/11] 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      | 2 +-
 dtc.h      | 2 +-
 flattree.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/dtc.c b/dtc.c
index bdb3f59..eed69d5 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 */
diff --git a/dtc.h b/dtc.h
index 511dda1..bebdb22 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

[PATCH 08/11] dtc: Fix signedness comparisons warnings: Wrap (-1)

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in dtc's data_copy_file().

Even though maxlen is of an unsigned type, we compare against "-1",
which is passed in from the parser to indicate an unknown size.

Cast the "-1" to an unsigned size to make the comparison match.

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

diff --git a/data.c b/data.c
index fdb0407..1473423 100644
--- a/data.c
+++ b/data.c
@@ -84,7 +84,7 @@ struct data data_copy_file(FILE *f, size_t maxlen)
 	while (!feof(f) && (d.len < maxlen)) {
 		size_t chunksize, ret;
 
-		if (maxlen == -1)
+		if (maxlen == (size_t)-1)
 			chunksize = 4096;
 		else
 			chunksize = maxlen - d.len;
-- 
2.17.5

[PATCH 09/11] 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 8393637..b8d0745 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, (unsigned)(p - path), child->name))
 			return get_node_by_path(child, p+1);
 		else if (!p && streq(path, child->name))
 			return child;
-- 
2.17.5

[PATCH 10/11] 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 7cb9011..e2c70d0 100644
--- a/checks.c
+++ b/checks.c
@@ -303,7 +303,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);
+	unsigned int n = strspn(node->name, c->data);
 
 	if (n < strlen(node->name))
 		FAIL(c, dti, node, "Bad character '%c' in node name",
@@ -363,7 +363,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);
+		unsigned int n = strspn(prop->name, c->data);
 
 		if (n < strlen(prop->name))
 			FAIL_PROP(c, dti, node, prop, "Bad character '%c' in property name",
@@ -380,7 +380,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);
+		unsigned int n = strspn(name, c->data);
 
 		if (n == strlen(prop->name))
 			continue;
@@ -556,7 +556,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);
@@ -1367,7 +1367,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 (prop->val.len % sizeof(cell_t)) {
 		FAIL_PROP(c, dti, node, prop,
@@ -1584,7 +1584,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)
@@ -1750,7 +1751,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;
 
@@ -1882,7 +1883,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 */
@@ -1903,7 +1904,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;
 
@@ -1930,7 +1931,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

[PATCH 11/11] 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 c187d5f..024d1b4 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 02/11] convert-dtsv0: Fix signedness comparisons warning

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:39PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in the generated lexer code.
> 
> In this case we walk over an array, and never use negative indicies, so
> we can change the loop counter variable to be unsigned.
> 
> This fixes "make convert-dtsv0", when compiled with -Wsign-compare.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  convert-dtsv0-lexer.l | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/convert-dtsv0-lexer.l b/convert-dtsv0-lexer.l
> index f52e8a1..7a66f57 100644
> --- a/convert-dtsv0-lexer.l
> +++ b/convert-dtsv0-lexer.l
> @@ -94,7 +94,7 @@ static const struct {
>  <INITIAL>[0-9a-fA-F]+	{
>  			unsigned long long val;
>  			int obase = 16, width = 0;
> -			int i;
> +			unsigned int i;
>  
>  			val = strtoull(yytext, NULL, cbase);
>  

-- 
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 03/11] fdtdump: Fix signedness comparisons warnings

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:40PM +0100, Andre Przywara wrote:
65;6003;1c> 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 9613bef..ec67f99 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 || (unsigned)(endp - p) < sizeof(struct fdt_header))

I think (size_t) would be more appropriate here, since it will match
the other size of the comparison, though it's very unlikely to cause a
problem in practice.

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

Re: [PATCH 04/11] fdtget: Fix signedness comparisons warnings

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:41PM +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 blow up the uint8_t to a larger *unsigned* type,
> before doing the left shift. That keeps all parts of the conditional
> operator "unsigned", and prevents the original warning.
> 
> This fixes "make fdtget", when compiled with -Wsign-compare.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  fdtget.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fdtget.c b/fdtget.c
> index 777582e..c5685d9 100644
> --- a/fdtget.c
> +++ b/fdtget.c
> @@ -62,8 +62,8 @@ 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;
> +		value = (size == 4) ? fdt32_ld((const fdt32_t *)p) :
> +			((size == 2) ? ((unsigned)(*p) << 8) | p[1] : *p);

Oof, yeah, that's a complicated expression.  I wonder if we should add
an fdt16_ld() to simplify this.

>  		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 05/11] dtc: Wrap phandle validity check

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:42PM +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 macro in dtc.h, and use ~0U instead of -1 on the
> way, to keep everything in the unsigned realm.

We can probably make this a bit safer still, if we made it a typed
inline, instead of a macro.

> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  checks.c   | 10 +++++-----
>  dtc.h      |  2 ++
>  livetree.c |  4 ++--
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 17cb689..7cb9011 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -497,7 +497,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_VALID_PHANDLE(phandle)) {
>  		FAIL_PROP(c, dti, node, prop, "bad value (0x%x) in %s property",
>  		     phandle, prop->name);
>  		return 0;
> @@ -1379,14 +1379,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_VALID_PHANDLE(phandle)) {
>  			/* Give up if this is an overlay with external references */
>  			if (dti->dtsflags & DTSF_PLUGIN)
>  				break;
> @@ -1603,7 +1603,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_VALID_PHANDLE(phandle)) {
>  				/* Give up if this is an overlay with
>  				 * external references */
>  				if (dti->dtsflags & DTSF_PLUGIN)
> @@ -1760,7 +1760,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_VALID_PHANDLE(phandle))
>  		return NULL;
>  
>  	node = get_node_by_phandle(dti->dt, phandle);
> diff --git a/dtc.h b/dtc.h
> index a08f415..f2f96ec 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -49,6 +49,8 @@ extern int annotate;		/* annotate .dts with input source location */
>  #define PHANDLE_EPAPR	0x2
>  #define PHANDLE_BOTH	0x3
>  
> +#define FDT_VALID_PHANDLE(x)	((x) != 0 && (x) != ~0U)
> +
>  typedef uint32_t cell_t;
>  
>  static inline uint16_t dtb_ld16(const void *p)
> diff --git a/livetree.c b/livetree.c
> index 032df58..aea6095 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_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_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 --]

Re: [PATCH 06/11] dtc: Fix signedness comparisons warnings: change types

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:43PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in various parts of dtc.
> 
> Many variables are using signed types unnecessarily, as we never use
> negative value in them.
> Change their types to be unsigned, to prevent issues with comparisons.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  data.c     | 4 ++--
>  dtc.h      | 8 ++++----
>  flattree.c | 8 ++++----
>  livetree.c | 2 +-
>  yamltree.c | 6 +++---
>  5 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/data.c b/data.c
> index 0a43b6d..fdb0407 100644
> --- a/data.c
> +++ b/data.c
> @@ -21,10 +21,10 @@ void data_free(struct data d)
>  		free(d.val);
>  }
>  
> -struct data data_grow_for(struct data d, int xlen)
> +struct data data_grow_for(struct data d, unsigned int xlen)
>  {
>  	struct data nd;
> -	int newsize;
> +	unsigned int newsize;
>  
>  	if (xlen == 0)
>  		return d;
> diff --git a/dtc.h b/dtc.h
> index f2f96ec..511dda1 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -107,13 +107,13 @@ extern const char *markername(enum markertype markertype);
>  
>  struct  marker {
>  	enum markertype type;
> -	int offset;
> +	unsigned int offset;
>  	char *ref;
>  	struct marker *next;
>  };
>  
>  struct data {
> -	int len;
> +	unsigned int len;
>  	char *val;
>  	struct marker *markers;
>  };
> @@ -131,7 +131,7 @@ size_t type_marker_length(struct marker *m);
>  
>  void data_free(struct data d);
>  
> -struct data data_grow_for(struct data d, int xlen);
> +struct data data_grow_for(struct data d, unsigned int xlen);
>  
>  struct data data_copy_mem(const char *mem, int len);
>  struct data data_copy_escape_string(const char *s, int len);
> @@ -255,7 +255,7 @@ void append_to_property(struct node *node,
>  const char *get_unitname(struct node *node);
>  struct property *get_property(struct node *node, const char *propname);
>  cell_t propval_cell(struct property *prop);
> -cell_t propval_cell_n(struct property *prop, int n);
> +cell_t propval_cell_n(struct property *prop, unsigned int n);
>  struct property *get_property_by_label(struct node *tree, const char *label,
>  				       struct node **node);
>  struct marker *get_marker_label(struct node *tree, const char *label,
> diff --git a/flattree.c b/flattree.c
> index 07f10d2..4659afb 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -149,7 +149,7 @@ static void asm_emit_align(void *e, int a)
>  static void asm_emit_data(void *e, struct data d)
>  {
>  	FILE *f = e;
> -	int off = 0;
> +	unsigned int off = 0;
>  	struct marker *m = d.markers;
>  
>  	for_each_marker_of_type(m, LABEL)
> @@ -219,7 +219,7 @@ static struct emitter asm_emitter = {
>  
>  static int stringtable_insert(struct data *d, const char *str)
>  {
> -	int i;
> +	unsigned int i;
>  
>  	/* FIXME: do this more efficiently? */
>  
> @@ -345,7 +345,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>  void dt_to_blob(FILE *f, struct dt_info *dti, int version)
>  {
>  	struct version_info *vi = NULL;
> -	int i;
> +	unsigned int i;
>  	struct data blob       = empty_data;
>  	struct data reservebuf = empty_data;
>  	struct data dtbuf      = empty_data;
> @@ -446,7 +446,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
>  void dt_to_asm(FILE *f, struct dt_info *dti, int version)
>  {
>  	struct version_info *vi = NULL;
> -	int i;
> +	unsigned int i;
>  	struct data strbuf = empty_data;
>  	struct reserve_info *re;
>  	const char *symprefix = "dt";
> diff --git a/livetree.c b/livetree.c
> index aea6095..8393637 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -438,7 +438,7 @@ cell_t propval_cell(struct property *prop)
>  	return fdt32_to_cpu(*((fdt32_t *)prop->val.val));
>  }
>  
> -cell_t propval_cell_n(struct property *prop, int n)
> +cell_t propval_cell_n(struct property *prop, unsigned int n)
>  {
>  	assert(prop->val.len / sizeof(cell_t) >= n);
>  	return fdt32_to_cpu(*((fdt32_t *)prop->val.val + n));
> diff --git a/yamltree.c b/yamltree.c
> index 4e93c12..e63d32f 100644
> --- a/yamltree.c
> +++ b/yamltree.c
> @@ -29,11 +29,11 @@ char *yaml_error_name[] = {
>  		    (emitter)->problem, __func__, __LINE__);		\
>  })
>  
> -static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, char *data, int len, int width)
> +static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, char *data, unsigned int len, int width)
>  {
>  	yaml_event_t event;
>  	void *tag;
> -	int off, start_offset = markers->offset;
> +	unsigned int off, start_offset = markers->offset;
>  
>  	switch(width) {
>  		case 1: tag = "!u8"; break;
> @@ -112,7 +112,7 @@ static void yaml_propval_string(yaml_emitter_t *emitter, char *str, int len)
>  static void yaml_propval(yaml_emitter_t *emitter, struct property *prop)
>  {
>  	yaml_event_t event;
> -	int len = prop->val.len;
> +	unsigned int len = prop->val.len;
>  	struct marker *m = prop->val.markers;
>  
>  	/* Emit the property name */

-- 
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 07/11] dtc: Fix signedness comparisons warnings: reservednum

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:44PM +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.

If you change this, then you also need to change from strtol() to
strtoul() at the point this assigned.

> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  dtc.c      | 2 +-
>  dtc.h      | 2 +-
>  flattree.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index bdb3f59..eed69d5 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 */
> diff --git a/dtc.h b/dtc.h
> index 511dda1..bebdb22 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 --]

Re: [PATCH 08/11] dtc: Fix signedness comparisons warnings: Wrap (-1)

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:45PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in dtc's data_copy_file().
> 
> Even though maxlen is of an unsigned type, we compare against "-1",
> which is passed in from the parser to indicate an unknown size.
> 
> Cast the "-1" to an unsigned size to make the comparison match.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  data.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/data.c b/data.c
> index fdb0407..1473423 100644
> --- a/data.c
> +++ b/data.c
> @@ -84,7 +84,7 @@ struct data data_copy_file(FILE *f, size_t maxlen)
>  	while (!feof(f) && (d.len < maxlen)) {
>  		size_t chunksize, ret;
>  
> -		if (maxlen == -1)
> +		if (maxlen == (size_t)-1)
>  			chunksize = 4096;
>  		else
>  			chunksize = maxlen - d.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 09/11] dtc: Fix signedness comparisons warnings: pointer diff

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:46PM +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>

Again, (size_t) would be more strictly correct, I think.

> ---
>  livetree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/livetree.c b/livetree.c
> index 8393637..b8d0745 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, (unsigned)(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 --]

Re: [PATCH 10/11] checks: Fix signedness comparisons warnings

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:47PM +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 7cb9011..e2c70d0 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -303,7 +303,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);
> +	unsigned int n = strspn(node->name, c->data);

Better to make it an actual size_t, I think

>  	if (n < strlen(node->name))
>  		FAIL(c, dti, node, "Bad character '%c' in node name",
> @@ -363,7 +363,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);
> +		unsigned int n = strspn(prop->name, c->data);
>  
>  		if (n < strlen(prop->name))
>  			FAIL_PROP(c, dti, node, prop, "Bad character '%c' in property name",
> @@ -380,7 +380,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);
> +		unsigned int n = strspn(name, c->data);
>  
>  		if (n == strlen(prop->name))
>  			continue;
> @@ -556,7 +556,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);
> @@ -1367,7 +1367,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 (prop->val.len % sizeof(cell_t)) {
>  		FAIL_PROP(c, dti, node, prop,
> @@ -1584,7 +1584,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)
> @@ -1750,7 +1751,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;
>  
> @@ -1882,7 +1883,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 */
> @@ -1903,7 +1904,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;
>  
> @@ -1930,7 +1931,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 --]

Re: [PATCH 01/11] tests: Fix signedness comparisons warnings

[David Gibson]

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

On Mon, Oct 12, 2020 at 05:19:38PM +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.
> 
> 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>

Oof, this is a big one.

> ---
>  tests/dumptrees.c           | 2 +-
>  tests/fs_tree1.c            | 2 +-
>  tests/get_name.c            | 2 +-
>  tests/integer-expressions.c | 2 +-
>  tests/nopulate.c            | 3 ++-
>  tests/overlay.c             | 2 +-
>  tests/phandle_format.c      | 2 +-
>  tests/property_iterate.c    | 2 +-
>  tests/references.c          | 2 +-
>  tests/set_name.c            | 4 ++--
>  tests/subnode_iterate.c     | 2 +-
>  tests/tests.h               | 2 +-
>  tests/testutils.c           | 6 +++---
>  13 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/dumptrees.c b/tests/dumptrees.c
> index aecb326..49ef525 100644
> --- a/tests/dumptrees.c
> +++ b/tests/dumptrees.c
> @@ -30,7 +30,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..7b757ec 100644
> --- a/tests/get_name.c
> +++ b/tests/get_name.c
> @@ -39,7 +39,7 @@ static void check_name(void *fdt, const char *path)
>  		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));

So, here you're relying on fdt_get_name() returning NULL in all the
cases where it would return a negative len.  But.. this is test code,
so we should actually verify that, rather than just assume it, and
fail the test if len is negative here.

> 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..c96f6f2 100644
> --- a/tests/overlay.c
> +++ b/tests/overlay.c
> @@ -35,7 +35,7 @@ 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 || ((unsigned)len < (sizeof(uint32_t) * (poffset + 1))))


Likewise here, we should extend the test to explicitly verify than len
>= 0 in the !!val case.

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

I'm wondering if we should actually add an exported (inline) function
to libfdt to do this test, since things like this are changed in a
couple of places through this series.

>  		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..cd2b9aa 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));
>  
> @@ -56,7 +56,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, newname);
>  
> -	if (len != strlen(getname))
> +	if ((unsigned)len != strlen(getname))

Again, add explicit negative check.

>  		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..ff215fc 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;
> @@ -112,13 +112,13 @@ 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 ((unsigned)namelen != strlen(propname))


And here.
>  		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)

And here.

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

[Andre Przywara]

On Tue, 13 Oct 2020 16:09:27 +1100
David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:

Hi,

eventually finding some time to come back to this:

> On Mon, Oct 12, 2020 at 05:19:38PM +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.
> > 
> > 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>  
> 
> Oof, this is a big one.
> 
> > ---
> >  tests/dumptrees.c           | 2 +-
> >  tests/fs_tree1.c            | 2 +-
> >  tests/get_name.c            | 2 +-
> >  tests/integer-expressions.c | 2 +-
> >  tests/nopulate.c            | 3 ++-
> >  tests/overlay.c             | 2 +-
> >  tests/phandle_format.c      | 2 +-
> >  tests/property_iterate.c    | 2 +-
> >  tests/references.c          | 2 +-
> >  tests/set_name.c            | 4 ++--
> >  tests/subnode_iterate.c     | 2 +-
> >  tests/tests.h               | 2 +-
> >  tests/testutils.c           | 6 +++---
> >  13 files changed, 17 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tests/dumptrees.c b/tests/dumptrees.c
> > index aecb326..49ef525 100644
> > --- a/tests/dumptrees.c
> > +++ b/tests/dumptrees.c
> > @@ -30,7 +30,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..7b757ec 100644
> > --- a/tests/get_name.c
> > +++ b/tests/get_name.c
> > @@ -39,7 +39,7 @@ static void check_name(void *fdt, const char *path)
> >  		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));  
> 
> So, here you're relying on fdt_get_name() returning NULL in all the
> cases where it would return a negative len.  But.. this is test code,
> so we should actually verify that, rather than just assume it, and
> fail the test if len is negative here.
> 
> > 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..c96f6f2 100644
> > --- a/tests/overlay.c
> > +++ b/tests/overlay.c
> > @@ -35,7 +35,7 @@ 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 || ((unsigned)len < (sizeof(uint32_t) * (poffset + 1))))  
> 
> 
> Likewise here, we should extend the test to explicitly verify than len
> >= 0 in the !!val case.  
> 
> >  		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))  
> 
> I'm wondering if we should actually add an exported (inline) function
> to libfdt to do this test, since things like this are changed in a
> couple of places through this series.

So I have this function for dtc, but this is in dtc.h, which is
internal, so not available here.
Since it's just two occasions in all of the tests, I just keep it
explicit, for simplicity.

I fixed the rest of the issues you mentioned, in this patch and all the
others. Will send an update in a jiffy.

Cheers,
Andre

> 
> >  		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..cd2b9aa 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));
> >  
> > @@ -56,7 +56,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, newname);
> >  
> > -	if (len != strlen(getname))
> > +	if ((unsigned)len != strlen(getname))  
> 
> Again, add explicit negative check.
> 
> >  		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..ff215fc 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;
> > @@ -112,13 +112,13 @@ 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 ((unsigned)namelen != strlen(propname))  
> 
> 
> And here.
> >  		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)  
> 
> And here.
> 
> >  		FAIL("Length retrieved for \"%s\" by fdt_get_property()"
> >  		     " differs from stored length (%d != %d)",
> >  		     name, retlen, proplen);  
> 

Re: [PATCH 01/11] tests: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 11, 2021 at 06:11:34PM +0100, Andre Przywara wrote:
> On Tue, 13 Oct 2020 16:09:27 +1100
> David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> Hi,
> 
> eventually finding some time to come back to this:
> 
> > On Mon, Oct 12, 2020 at 05:19:38PM +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.
> > > 
> > > 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>  
> > 
> > Oof, this is a big one.
> > 
> > > ---
> > >  tests/dumptrees.c           | 2 +-
> > >  tests/fs_tree1.c            | 2 +-
> > >  tests/get_name.c            | 2 +-
> > >  tests/integer-expressions.c | 2 +-
> > >  tests/nopulate.c            | 3 ++-
> > >  tests/overlay.c             | 2 +-
> > >  tests/phandle_format.c      | 2 +-
> > >  tests/property_iterate.c    | 2 +-
> > >  tests/references.c          | 2 +-
> > >  tests/set_name.c            | 4 ++--
> > >  tests/subnode_iterate.c     | 2 +-
> > >  tests/tests.h               | 2 +-
> > >  tests/testutils.c           | 6 +++---
> > >  13 files changed, 17 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/tests/dumptrees.c b/tests/dumptrees.c
> > > index aecb326..49ef525 100644
> > > --- a/tests/dumptrees.c
> > > +++ b/tests/dumptrees.c
> > > @@ -30,7 +30,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..7b757ec 100644
> > > --- a/tests/get_name.c
> > > +++ b/tests/get_name.c
> > > @@ -39,7 +39,7 @@ static void check_name(void *fdt, const char *path)
> > >  		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));  
> > 
> > So, here you're relying on fdt_get_name() returning NULL in all the
> > cases where it would return a negative len.  But.. this is test code,
> > so we should actually verify that, rather than just assume it, and
> > fail the test if len is negative here.
> > 
> > > 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..c96f6f2 100644
> > > --- a/tests/overlay.c
> > > +++ b/tests/overlay.c
> > > @@ -35,7 +35,7 @@ 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 || ((unsigned)len < (sizeof(uint32_t) * (poffset + 1))))  
> > 
> > 
> > Likewise here, we should extend the test to explicitly verify than len
> > >= 0 in the !!val case.  
> > 
> > >  		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))  
> > 
> > I'm wondering if we should actually add an exported (inline) function
> > to libfdt to do this test, since things like this are changed in a
> > couple of places through this series.
> 
> So I have this function for dtc, but this is in dtc.h, which is
> internal, so not available here.
> Since it's just two occasions in all of the tests, I just keep it
> explicit, for simplicity.

Fair enough.

> 
> I fixed the rest of the issues you mentioned, in this patch and all the
> others. Will send an update in a jiffy.
> 
> Cheers,
> Andre
> 
> > 
> > >  		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..cd2b9aa 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));
> > >  
> > > @@ -56,7 +56,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, newname);
> > >  
> > > -	if (len != strlen(getname))
> > > +	if ((unsigned)len != strlen(getname))  
> > 
> > Again, add explicit negative check.
> > 
> > >  		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..ff215fc 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;
> > > @@ -112,13 +112,13 @@ 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 ((unsigned)namelen != strlen(propname))  
> > 
> > 
> > And here.
> > >  		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)  
> > 
> > And here.
> > 
> > >  		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 --]