| Message ID | 1459149797-24108-1-git-send-email-marcel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 28.03.2016 um 09:23 schrieb Marcel Apfelbaum: > Fix 'error: shift exponent -1 is negative' warning > by adding a corresponding assert. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/pci/pci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index e67664d..a1d41aa 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -163,11 +163,15 @@ int pci_bar(PCIDevice *d, int reg) > > static inline int pci_irq_state(PCIDevice *d, int irq_num) > { > + assert(irq_num >= 0); > + > return (d->irq_state >> irq_num) & 0x1; > } > > static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) > { > + assert(irq_num >= 0); > + > d->irq_state &= ~(0x1 << irq_num); > d->irq_state |= level << irq_num; > } Do we use negative values for irq_num anywhere? If not, using an unsigned irq_num might be a better solution. Stefan
On 03/28/2016 11:43 AM, Stefan Weil wrote: > Am 28.03.2016 um 09:23 schrieb Marcel Apfelbaum: >> Fix 'error: shift exponent -1 is negative' warning >> by adding a corresponding assert. >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> hw/pci/pci.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index e67664d..a1d41aa 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -163,11 +163,15 @@ int pci_bar(PCIDevice *d, int reg) >> >> static inline int pci_irq_state(PCIDevice *d, int irq_num) >> { >> + assert(irq_num >= 0); >> + >> return (d->irq_state >> irq_num) & 0x1; >> } >> >> static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) >> { >> + assert(irq_num >= 0); >> + >> d->irq_state &= ~(0x1 << irq_num); >> d->irq_state |= level << irq_num; >> } > > Do we use negative values for irq_num anywhere? Hi Stefan, I didn't see any irq_nu assignments to a negative value but there are some cases where it can be negative due to arithmetical operations like: hw/pci-host/bonito.c:651: int internal_irq = irq_num - BONITO_IRQ_BASE; On other cases we look for negative value: hw/ppc/ppc4xx_devs.c:171: if (irq_num < 0 || irq_num > 3) or hw/ppc/ppc4xx_pci.c:263: if (irq_num < 0) > If not, using an unsigned irq_num might be a better solution. All of the above are manageable, of course, but we are close to hard freeze (tomorrow) and the scope of this patch is much smaller (fix a compilation warning) I think is definitely worth looking into it, but maybe as part of QEMU 2.7. Thanks, Marcel > > Stefan >
On 03/28/2016 10:23 AM, Marcel Apfelbaum wrote: > Fix 'error: shift exponent -1 is negative' warning > by adding a corresponding assert. > Ping. Thanks, Marcel > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/pci/pci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index e67664d..a1d41aa 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -163,11 +163,15 @@ int pci_bar(PCIDevice *d, int reg) > > static inline int pci_irq_state(PCIDevice *d, int irq_num) > { > + assert(irq_num >= 0); > + > return (d->irq_state >> irq_num) & 0x1; > } > > static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) > { > + assert(irq_num >= 0); > + > d->irq_state &= ~(0x1 << irq_num); > d->irq_state |= level << irq_num; > } >
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index e67664d..a1d41aa 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -163,11 +163,15 @@ int pci_bar(PCIDevice *d, int reg) static inline int pci_irq_state(PCIDevice *d, int irq_num) { + assert(irq_num >= 0); + return (d->irq_state >> irq_num) & 0x1; } static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) { + assert(irq_num >= 0); + d->irq_state &= ~(0x1 << irq_num); d->irq_state |= level << irq_num; }