| Message ID | 1426738458-24229-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
David Gibson <david@gibson.dropbear.id.au> writes: > spapr_pci.c contains a number of expressions of the form (uval == -1) or > (uval != -1), where 'uval' is an unsigned value. > > This mostly works in practice, because as long as the width of uval is > greater or equal than that of (int), the -1 will be promoted to the > unsigned type, which is the expected outcome. > > However, at least for the cases where uval is uint32_t, this would break > on platforms where sizeof(int) > 4 (and a few such do exist), because then > the uint32_t value would be promoted to the larger int type, and never be > equal to -1. We may not care for portability to such systems. However, a comparison between signed and unsigned values still makes careful readers pause to consider width. gcc can warn (-Wsign-compare), but we don't enable this warning, probably because we'd get too many of them. > This patch fixes these errors. The fixes for the (uint32_t) cases are > necessary as described above. I've made similar fixes to (uint64_t) and > (hwaddr) cases. Those are strictly theoretical, since I don't know of any > platforms where sizeof(int) > 8, but hey, it's not that hard so we might > as well be strictly C standard compliant. It fixes all -Wsign-compare warnings from this file (but not from included headers, but that's outside the scope of this patch). > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 19.03.15 05:14, David Gibson wrote: > spapr_pci.c contains a number of expressions of the form (uval == -1) or > (uval != -1), where 'uval' is an unsigned value. > > This mostly works in practice, because as long as the width of uval is > greater or equal than that of (int), the -1 will be promoted to the > unsigned type, which is the expected outcome. > > However, at least for the cases where uval is uint32_t, this would break > on platforms where sizeof(int) > 4 (and a few such do exist), because then > the uint32_t value would be promoted to the larger int type, and never be > equal to -1. > > This patch fixes these errors. The fixes for the (uint32_t) cases are > necessary as described above. I've made similar fixes to (uint64_t) and > (hwaddr) cases. Those are strictly theoretical, since I don't know of any > platforms where sizeof(int) > 8, but hey, it's not that hard so we might > as well be strictly C standard compliant. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Thanks, applied to ppc-next-2.4. Alex
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 05f4fac..03f6d96 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -742,12 +742,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) PCIBus *bus; uint64_t msi_window_size = 4096; - if (sphb->index != -1) { + if (sphb->index != (uint32_t)-1) { hwaddr windows_base; - if ((sphb->buid != -1) || (sphb->dma_liobn != -1) - || (sphb->mem_win_addr != -1) - || (sphb->io_win_addr != -1)) { + if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1) + || (sphb->mem_win_addr != (hwaddr)-1) + || (sphb->io_win_addr != (hwaddr)-1)) { error_setg(errp, "Either \"index\" or other parameters must" " be specified for PAPR PHB, not both"); return; @@ -768,22 +768,22 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF; } - if (sphb->buid == -1) { + if (sphb->buid == (uint64_t)-1) { error_setg(errp, "BUID not specified for PHB"); return; } - if (sphb->dma_liobn == -1) { + if (sphb->dma_liobn == (uint32_t)-1) { error_setg(errp, "LIOBN not specified for PHB"); return; } - if (sphb->mem_win_addr == -1) { + if (sphb->mem_win_addr == (hwaddr)-1) { error_setg(errp, "Memory window address not specified for PHB"); return; } - if (sphb->io_win_addr == -1) { + if (sphb->io_win_addr == (hwaddr)-1) { error_setg(errp, "IO window address not specified for PHB"); return; }
spapr_pci.c contains a number of expressions of the form (uval == -1) or (uval != -1), where 'uval' is an unsigned value. This mostly works in practice, because as long as the width of uval is greater or equal than that of (int), the -1 will be promoted to the unsigned type, which is the expected outcome. However, at least for the cases where uval is uint32_t, this would break on platforms where sizeof(int) > 4 (and a few such do exist), because then the uint32_t value would be promoted to the larger int type, and never be equal to -1. This patch fixes these errors. The fixes for the (uint32_t) cases are necessary as described above. I've made similar fixes to (uint64_t) and (hwaddr) cases. Those are strictly theoretical, since I don't know of any platforms where sizeof(int) > 8, but hey, it's not that hard so we might as well be strictly C standard compliant. Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_pci.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)