GCC with "-g -Wall -Wextra" option throws warning message as below:
error: comparison of integer expressions of different signedness:
‘int’ and ‘unsigned int’ [-Werror=sign-compare]
Silence the warning by correcting the integer type.
Signed-off-by: Paran Lee <p4ranlee@gmail.com>
---
xen/common/device_tree.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4aae281e89..402e465c7a 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -79,7 +79,7 @@ struct dt_bus
const char *addresses;
bool_t (*match)(const struct dt_device_node *node);
void (*count_cells)(const struct dt_device_node *child,
- int *addrc, int *sizec);
+ unsigned int *addrc, unsigned int *sizec);
u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
int (*translate)(__be32 *addr, u64 offset, int na);
unsigned int (*get_flags)(const __be32 *addr);
@@ -569,7 +569,7 @@ static bool_t dt_bus_default_match(const struct dt_device_node *node)
}
static void dt_bus_default_count_cells(const struct dt_device_node *dev,
- int *addrc, int *sizec)
+ unsigned int *addrc, unsigned int *sizec)
{
if ( addrc )
*addrc = dt_n_addr_cells(dev);
@@ -649,7 +649,7 @@ static bool_t dt_bus_pci_match(const struct dt_device_node *np)
}
static void dt_bus_pci_count_cells(const struct dt_device_node *np,
- int *addrc, int *sizec)
+ unsigned int *addrc, unsigned int *sizec)
{
if (addrc)
*addrc = 3;
@@ -737,7 +737,7 @@ static const struct dt_bus dt_busses[] =
static const struct dt_bus *dt_match_bus(const struct dt_device_node *np)
{
- int i;
+ long unsigned int i;
for ( i = 0; i < ARRAY_SIZE(dt_busses); i++ )
if ( !dt_busses[i].match || dt_busses[i].match(np) )
@@ -754,7 +754,8 @@ static const __be32 *dt_get_address(const struct dt_device_node *dev,
u32 psize;
const struct dt_device_node *parent;
const struct dt_bus *bus;
- int onesize, i, na, ns;
+ unsigned int i;
+ unsigned int onesize, na, ns;
/* Get parent & match bus type */
parent = dt_get_parent(dev);
@@ -797,8 +798,7 @@ static int dt_translate_one(const struct dt_device_node *parent,
int pna, const char *rprop)
{
const __be32 *ranges;
- unsigned int rlen;
- int rone;
+ unsigned int rlen, rone;
u64 offset = DT_BAD_ADDR;
ranges = dt_get_property(parent, rprop, &rlen);
@@ -857,7 +857,7 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
const struct dt_device_node *parent = NULL;
const struct dt_bus *bus, *pbus;
__be32 addr[DT_MAX_ADDR_CELLS];
- int na, ns, pna, pns;
+ unsigned int na, ns, pna, pns;
u64 result = DT_BAD_ADDR;
dt_dprintk("DT: ** translation for device %s **\n", dev->full_name);
@@ -966,8 +966,7 @@ int dt_for_each_range(const struct dt_device_node *dev,
const struct dt_bus *bus, *pbus;
const __be32 *ranges;
__be32 addr[DT_MAX_ADDR_CELLS];
- unsigned int rlen;
- int na, ns, pna, pns, rone;
+ unsigned int rlen, na, ns, pna, pns, rone;
bus = dt_match_bus(dev);
if ( !bus )
@@ -1134,7 +1133,7 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
u32 psize;
const struct dt_device_node *parent;
const struct dt_bus *bus;
- int onesize, na, ns;
+ unsigned int onesize, na, ns;
/* Get parent & match bus type */
parent = dt_get_parent(dev);
@@ -1169,8 +1168,8 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
const struct dt_device_node *ipar, *tnode, *old = NULL;
const __be32 *tmp, *imap;
u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0;
- u32 imaplen;
- int i, ret;
+ u32 i, imaplen;
+ int ret;
struct dt_raw_irq dt_raw_irq;
struct dt_irq dt_irq;
@@ -1354,8 +1353,8 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
const struct dt_device_node *ipar, *tnode, *old = NULL, *newpar = NULL;
const __be32 *tmp, *imap, *imask;
u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0;
- u32 imaplen;
- int match, i;
+ u32 i, imaplen;
+ int match;
dt_dprintk("dt_irq_map_raw: par=%s,intspec=[0x%08x 0x%08x...],ointsize=%d\n",
parent->full_name, be32_to_cpup(intspec),
@@ -1737,7 +1736,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
if ( out_args )
{
- int i;
+ u32 i;
WARN_ON(count > MAX_PHANDLE_ARGS);
if (count > MAX_PHANDLE_ARGS)
--
2.25.1
Hi,
On 19/04/2022 16:46, Paran Lee wrote:
> GCC with "-g -Wall -Wextra" option throws warning message as below:
s/warning/error/
>
> error: comparison of integer expressions of different signedness:
> ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
Can you post the full log?
>
> Silence the warning by correcting the integer type.
IIRC most of the code you touch below is from Linux and had very limited
changes afterwards. So while I agree the changes are good, AFAIK they
are not (latent?) bugs and we don't build by default with -Wextra.
Therefore, I would prefer if they are upstreamed to Linux first and then
backported to Xen.
>
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>
> ---
> xen/common/device_tree.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..402e465c7a 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -79,7 +79,7 @@ struct dt_bus
> const char *addresses;
> bool_t (*match)(const struct dt_device_node *node);
> void (*count_cells)(const struct dt_device_node *child,
> - int *addrc, int *sizec);
> + unsigned int *addrc, unsigned int *sizec);
> u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
> int (*translate)(__be32 *addr, u64 offset, int na);
> unsigned int (*get_flags)(const __be32 *addr);
> @@ -569,7 +569,7 @@ static bool_t dt_bus_default_match(const struct dt_device_node *node)
> }
>
> static void dt_bus_default_count_cells(const struct dt_device_node *dev,
> - int *addrc, int *sizec)
> + unsigned int *addrc, unsigned int *sizec)
> {
> if ( addrc )
> *addrc = dt_n_addr_cells(dev);
Technically, the prototype of dt_n_addr_cells() is "int ... (". So now
you are casting an "int" to an "unsigned int". AFAICT, dt_n_addr_cells()
will never return a negative value. So we should propage the "unsigned int".
> @@ -649,7 +649,7 @@ static bool_t dt_bus_pci_match(const struct dt_device_node *np)
> }
>
> static void dt_bus_pci_count_cells(const struct dt_device_node *np,
> - int *addrc, int *sizec)
> + unsigned int *addrc, unsigned int *sizec)
> {
> if (addrc)
> *addrc = 3;
> @@ -737,7 +737,7 @@ static const struct dt_bus dt_busses[] =
>
> static const struct dt_bus *dt_match_bus(const struct dt_device_node *np)
> {
> - int i;
> + long unsigned int i;
This should be size_t.
>
> for ( i = 0; i < ARRAY_SIZE(dt_busses); i++ )
> if ( !dt_busses[i].match || dt_busses[i].match(np) )
> @@ -754,7 +754,8 @@ static const __be32 *dt_get_address(const struct dt_device_node *dev,
> u32 psize;
> const struct dt_device_node *parent;
> const struct dt_bus *bus;
> - int onesize, i, na, ns;
> + unsigned int i;
> + unsigned int onesize, na, ns; >
> /* Get parent & match bus type */
> parent = dt_get_parent(dev);
> @@ -797,8 +798,7 @@ static int dt_translate_one(const struct dt_device_node *parent,
> int pna, const char *rprop)
> {
> const __be32 *ranges;
> - unsigned int rlen;
> - int rone;
> + unsigned int rlen, rone;
> u64 offset = DT_BAD_ADDR;
>
> ranges = dt_get_property(parent, rprop, &rlen);
> @@ -857,7 +857,7 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
> const struct dt_device_node *parent = NULL;
> const struct dt_bus *bus, *pbus;
> __be32 addr[DT_MAX_ADDR_CELLS];
> - int na, ns, pna, pns;
> + unsigned int na, ns, pna, pns;
> u64 result = DT_BAD_ADDR;
>
> dt_dprintk("DT: ** translation for device %s **\n", dev->full_name);
> @@ -966,8 +966,7 @@ int dt_for_each_range(const struct dt_device_node *dev,
> const struct dt_bus *bus, *pbus;
> const __be32 *ranges;
> __be32 addr[DT_MAX_ADDR_CELLS];
> - unsigned int rlen;
> - int na, ns, pna, pns, rone;
> + unsigned int rlen, na, ns, pna, pns, rone;
>
> bus = dt_match_bus(dev);
> if ( !bus )
> @@ -1134,7 +1133,7 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
> u32 psize;
> const struct dt_device_node *parent;
> const struct dt_bus *bus;
> - int onesize, na, ns;
> + unsigned int onesize, na, ns;
>
> /* Get parent & match bus type */
> parent = dt_get_parent(dev);
> @@ -1169,8 +1168,8 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
> const struct dt_device_node *ipar, *tnode, *old = NULL;
> const __be32 *tmp, *imap;
> u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0;
> - u32 imaplen;
> - int i, ret;
> + u32 i, imaplen;
> + int ret;
>
> struct dt_raw_irq dt_raw_irq;
> struct dt_irq dt_irq;
> @@ -1354,8 +1353,8 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
> const struct dt_device_node *ipar, *tnode, *old = NULL, *newpar = NULL;
> const __be32 *tmp, *imap, *imask;
> u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0;
> - u32 imaplen;
> - int match, i;
> + u32 i, imaplen;
> + int match;
>
> dt_dprintk("dt_irq_map_raw: par=%s,intspec=[0x%08x 0x%08x...],ointsize=%d\n",
> parent->full_name, be32_to_cpup(intspec),
> @@ -1737,7 +1736,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
>
> if ( out_args )
> {
> - int i;
> + u32 i;
>
> WARN_ON(count > MAX_PHANDLE_ARGS);
> if (count > MAX_PHANDLE_ARGS)
Cheers,
--
Julien Grall
On Wed, 20 Apr 2022, Paran Lee wrote:
> GCC with "-g -Wall -Wextra" option throws warning message as below:
>
> error: comparison of integer expressions of different signedness:
> ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
>
> Silence the warning by correcting the integer type.
>
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>
Thanks for the patch, this is a good improvement.
> ---
> xen/common/device_tree.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..402e465c7a 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -79,7 +79,7 @@ struct dt_bus
> const char *addresses;
> bool_t (*match)(const struct dt_device_node *node);
> void (*count_cells)(const struct dt_device_node *child,
> - int *addrc, int *sizec);
> + unsigned int *addrc, unsigned int *sizec);
> u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
> int (*translate)(__be32 *addr, u64 offset, int na);
> unsigned int (*get_flags)(const __be32 *addr);
> @@ -569,7 +569,7 @@ static bool_t dt_bus_default_match(const struct dt_device_node *node)
> }
>
> static void dt_bus_default_count_cells(const struct dt_device_node *dev,
> - int *addrc, int *sizec)
> + unsigned int *addrc, unsigned int *sizec)
> {
> if ( addrc )
> *addrc = dt_n_addr_cells(dev);
> @@ -649,7 +649,7 @@ static bool_t dt_bus_pci_match(const struct dt_device_node *np)
> }
>
> static void dt_bus_pci_count_cells(const struct dt_device_node *np,
> - int *addrc, int *sizec)
> + unsigned int *addrc, unsigned int *sizec)
> {
> if (addrc)
> *addrc = 3;
Although I don't necessarily mind these two changes to
dt_bus_default_count_cells and dt_bus_pci_count_cells as they are
improvements, I don't see how they are related to the "comparison of
integer expressions of different signedness" problem.
I definitely see the comparison between "i" and "imaplen" below for
instance but where is the problematic integers comparison in the case of
"na" and "ns"?
It would help to post the GCC output that you are getting with all the
warnings for this file.
> @@ -737,7 +737,7 @@ static const struct dt_bus dt_busses[] =
>
> static const struct dt_bus *dt_match_bus(const struct dt_device_node *np)
> {
> - int i;
> + long unsigned int i;
Just write is as "unsigned long" for uniformity with the rest of the
code
> for ( i = 0; i < ARRAY_SIZE(dt_busses); i++ )
> if ( !dt_busses[i].match || dt_busses[i].match(np) )
> @@ -754,7 +754,8 @@ static const __be32 *dt_get_address(const struct dt_device_node *dev,
> u32 psize;
> const struct dt_device_node *parent;
> const struct dt_bus *bus;
> - int onesize, i, na, ns;
> + unsigned int i;
> + unsigned int onesize, na, ns;
>
> /* Get parent & match bus type */
> parent = dt_get_parent(dev);
> @@ -797,8 +798,7 @@ static int dt_translate_one(const struct dt_device_node *parent,
> int pna, const char *rprop)
> {
> const __be32 *ranges;
> - unsigned int rlen;
> - int rone;
> + unsigned int rlen, rone;
> u64 offset = DT_BAD_ADDR;
>
> ranges = dt_get_property(parent, rprop, &rlen);
> @@ -857,7 +857,7 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
> const struct dt_device_node *parent = NULL;
> const struct dt_bus *bus, *pbus;
> __be32 addr[DT_MAX_ADDR_CELLS];
> - int na, ns, pna, pns;
> + unsigned int na, ns, pna, pns;
> u64 result = DT_BAD_ADDR;
>
> dt_dprintk("DT: ** translation for device %s **\n", dev->full_name);
In this function na and ns get printed a few times. We should convert
%d to %u.
Also, if we are going to change na and ns to unsigned int, then I think
we should also do it in dt_read_number and dt_translate_one. Not
necessarily in this patch though.
> @@ -966,8 +966,7 @@ int dt_for_each_range(const struct dt_device_node *dev,
> const struct dt_bus *bus, *pbus;
> const __be32 *ranges;
> __be32 addr[DT_MAX_ADDR_CELLS];
> - unsigned int rlen;
> - int na, ns, pna, pns, rone;
> + unsigned int rlen, na, ns, pna, pns, rone;
>
> bus = dt_match_bus(dev);
> if ( !bus )
> @@ -1134,7 +1133,7 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
> u32 psize;
> const struct dt_device_node *parent;
> const struct dt_bus *bus;
> - int onesize, na, ns;
> + unsigned int onesize, na, ns;
>
> /* Get parent & match bus type */
> parent = dt_get_parent(dev);
> @@ -1169,8 +1168,8 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
> const struct dt_device_node *ipar, *tnode, *old = NULL;
> const __be32 *tmp, *imap;
> u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0;
> - u32 imaplen;
> - int i, ret;
> + u32 i, imaplen;
> + int ret;
>
> struct dt_raw_irq dt_raw_irq;
> struct dt_irq dt_irq;
> @@ -1354,8 +1353,8 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
> const struct dt_device_node *ipar, *tnode, *old = NULL, *newpar = NULL;
> const __be32 *tmp, *imap, *imask;
> u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0;
> - u32 imaplen;
> - int match, i;
> + u32 i, imaplen;
> + int match;
>
> dt_dprintk("dt_irq_map_raw: par=%s,intspec=[0x%08x 0x%08x...],ointsize=%d\n",
> parent->full_name, be32_to_cpup(intspec),
> @@ -1737,7 +1736,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
>
> if ( out_args )
> {
> - int i;
> + u32 i;
>
> WARN_ON(count > MAX_PHANDLE_ARGS);
> if (count > MAX_PHANDLE_ARGS)
These changes are great, thanks! I wouldn't mind a patch that only
changes "i" in __dt_parse_phandle_with_args, dt_irq_map_raw,
dt_for_each_irq_map, dt_match_bus. Those changes are straightforward.
© 2016 - 2024 Red Hat, Inc.