[PATCH v3 0/5] dtc: Fix signed/unsigned comparison warnings

[PATCH v3 0/5] dtc: Fix signed/unsigned comparison warnings

[Andre Przywara]

Hi,

trying to fix the remaining comments from David:

Changelog v2 .. v3:
- dropped 2/8, 5/8 and 6/8, which are already merged
- replace fdt_is_valid_phandle() with phandle_is_valid()
- use cell_t instead of unsigned, where appropriate
- replace cumbersome statement with fdt16_ld()
- avoid masking errors in overlay tests
- check for "len" being non-negative in check_set_name()

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 (5):
  tests: Fix signedness comparisons warnings
  fdtget: Fix signedness comparisons warnings
  dtc: Wrap phandle validity check
  checks: Fix signedness comparisons warnings
  Makefile: add -Wsign-compare to warning options

 Makefile                    |  2 +-
 checks.c                    | 30 +++++++++++++++---------------
 dtc.h                       |  5 +++++
 fdtget.c                    | 10 ++++++++--
 libfdt/libfdt.h             |  7 +++++++
 livetree.c                  |  4 ++--
 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             |  6 +++++-
 tests/phandle_format.c      |  2 +-
 tests/property_iterate.c    |  2 +-
 tests/references.c          |  2 +-
 tests/set_name.c            | 10 ++++++++--
 tests/subnode_iterate.c     |  2 +-
 tests/tests.h               |  2 +-
 tests/testutils.c           | 12 +++++++++---
 19 files changed, 73 insertions(+), 36 deletions(-)

-- 
2.17.5

[PATCH v3 1/5] 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             |  6 +++++-
 tests/phandle_format.c      |  2 +-
 tests/property_iterate.c    |  2 +-
 tests/references.c          |  2 +-
 tests/set_name.c            | 10 ++++++++--
 tests/subnode_iterate.c     |  2 +-
 tests/tests.h               |  2 +-
 tests/testutils.c           | 12 +++++++++---
 13 files changed, 35 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..f3dd310 100644
--- a/tests/overlay.c
+++ b/tests/overlay.c
@@ -35,7 +35,11 @@ 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)
+		FAIL("fdt_getprop() returns negative length");
+	if (!val && len < 0)
+		return len;
+	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..5020305 100644
--- a/tests/set_name.c
+++ b/tests/set_name.c
@@ -39,7 +39,11 @@ 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 (len < 0)
+		FAIL("fdt_get_name(%s) returned negative length: %d",
+		     path, len);
+
+	if ((unsigned)len != strlen(getname))
 		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
 		     path, len, strlen(getname));
 
@@ -51,12 +55,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

[PATCH v3 2/5] 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 ++++++++--
 libfdt/libfdt.h |  7 +++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/fdtget.c b/fdtget.c
index 777582e..54fc6a0 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 = fdt16_ld((const fdt16_t *)p); break;
+		case 1:
+		default:
+			value = *p;
+			break;
+		}
 		printf(fmt, value);
 	}
 
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 73467f7..7f117e8 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -131,6 +131,13 @@ uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset);
  * to work even with unaligned pointers on platforms (such as ARMv5) that don't
  * like unaligned loads and stores.
  */
+static inline uint16_t fdt16_ld(const fdt16_t *p)
+{
+	const uint8_t *bp = (const uint8_t *)p;
+
+	return ((uint16_t)bp[0] << 8) | bp[1];
+}
+
 static inline uint32_t fdt32_ld(const fdt32_t *p)
 {
 	const uint8_t *bp = (const uint8_t *)p;
-- 
2.17.5

[PATCH v3 3/5] 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..cb71c73 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 (!phandle_is_valid(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 (!phandle_is_valid(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 (!phandle_is_valid(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 (!phandle_is_valid(phandle))
 		return NULL;
 
 	node = get_node_by_phandle(dti->dt, phandle);
diff --git a/dtc.h b/dtc.h
index 47664f2..cf2c6ac 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 phandle_is_valid(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 dfbae65..cc61237 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 (!phandle_is_valid(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 (phandle_is_valid(node->phandle))
 		return node->phandle;
 
 	while (get_node_by_phandle(root, phandle))
-- 
2.17.5

[PATCH v3 4/5] 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/checks.c b/checks.c
index cb71c73..e2690e9 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,7 @@ 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;
+	cell_t irq_cells, phandle;
 
 	irq_prop = get_property(node, "interrupts");
 	if (!irq_prop)
@@ -1762,7 +1762,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 +1910,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 +1931,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 +1958,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 v3 5/5] 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 v3 1/5] tests: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 18, 2021 at 06:20:26PM +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.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  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             |  6 +++++-
>  tests/phandle_format.c      |  2 +-
>  tests/property_iterate.c    |  2 +-
>  tests/references.c          |  2 +-
>  tests/set_name.c            | 10 ++++++++--
>  tests/subnode_iterate.c     |  2 +-
>  tests/tests.h               |  2 +-
>  tests/testutils.c           | 12 +++++++++---
>  13 files changed, 35 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..f3dd310 100644
> --- a/tests/overlay.c
> +++ b/tests/overlay.c
> @@ -35,7 +35,11 @@ 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)
> +		FAIL("fdt_getprop() returns negative length");
> +	if (!val && len < 0)
> +		return len;
> +	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..5020305 100644
> --- a/tests/set_name.c
> +++ b/tests/set_name.c
> @@ -39,7 +39,11 @@ 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 (len < 0)
> +		FAIL("fdt_get_name(%s) returned negative length: %d",
> +		     path, len);
> +
> +	if ((unsigned)len != strlen(getname))
>  		FAIL("fdt_get_name(%s) returned length %d instead of %zd",
>  		     path, len, strlen(getname));
>  
> @@ -51,12 +55,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);

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

[David Gibson]

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

On Fri, Jun 18, 2021 at 06:20:27PM +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>

Applied, thanks.

> ---
>  fdtget.c        | 10 ++++++++--
>  libfdt/libfdt.h |  7 +++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/fdtget.c b/fdtget.c
> index 777582e..54fc6a0 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 = fdt16_ld((const fdt16_t *)p); break;
> +		case 1:
> +		default:
> +			value = *p;
> +			break;
> +		}
>  		printf(fmt, value);
>  	}
>  
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 73467f7..7f117e8 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -131,6 +131,13 @@ uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset);
>   * to work even with unaligned pointers on platforms (such as ARMv5) that don't
>   * like unaligned loads and stores.
>   */
> +static inline uint16_t fdt16_ld(const fdt16_t *p)
> +{
> +	const uint8_t *bp = (const uint8_t *)p;
> +
> +	return ((uint16_t)bp[0] << 8) | bp[1];
> +}
> +
>  static inline uint32_t fdt32_ld(const fdt32_t *p)
>  {
>  	const uint8_t *bp = (const uint8_t *)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 v3 3/5] dtc: Wrap phandle validity check

[David Gibson]

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

On Fri, Jun 18, 2021 at 06:20:28PM +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>

Applied, thanks.

> ---
>  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..cb71c73 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 (!phandle_is_valid(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 (!phandle_is_valid(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 (!phandle_is_valid(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 (!phandle_is_valid(phandle))
>  		return NULL;
>  
>  	node = get_node_by_phandle(dti->dt, phandle);
> diff --git a/dtc.h b/dtc.h
> index 47664f2..cf2c6ac 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 phandle_is_valid(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 dfbae65..cc61237 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 (!phandle_is_valid(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 (phandle_is_valid(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 v3 4/5] checks: Fix signedness comparisons warnings

[David Gibson]

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

On Fri, Jun 18, 2021 at 06:20:29PM +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>

Applied, thanks.

> ---
>  checks.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index cb71c73..e2690e9 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,7 @@ 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;
> +	cell_t irq_cells, phandle;
>  
>  	irq_prop = get_property(node, "interrupts");
>  	if (!irq_prop)
> @@ -1762,7 +1762,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 +1910,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 +1931,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 +1958,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 v3 5/5] Makefile: add -Wsign-compare to warning options

[David Gibson]

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

On Fri, Jun 18, 2021 at 06:20:30PM +0100, Andre Przywara wrote:
> 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>

Applied, thanks.

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

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