| Message ID | CAAu8pHtJefidoG2-rFFOVScKgztMdy=-tD2HF-MvEhRmWxZuwQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 09/04/2011 05:52 PM, Blue Swirl wrote: > Avoid this warning by clang analyzer by defining a default case: > /src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value > returned to caller > return retval; > > Signed-off-by: Blue Swirl<blauwirbel@gmail.com> > --- > hw/openpic.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/hw/openpic.c b/hw/openpic.c > index 26c96e2..4b883ac 100644 > --- a/hw/openpic.c > +++ b/hw/openpic.c > @@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t > *opp, int n_IRQ, uint32_t reg) > case IRQ_IPVP: > retval = opp->src[n_IRQ].ipvp; > break; > + default: > case IRQ_IDE: > retval = opp->src[n_IRQ].ide; > break; Looks wrong, perhaps it should return 0? Paolo
On Mon, Sep 5, 2011 at 6:48 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 09/04/2011 05:52 PM, Blue Swirl wrote: >> >> Avoid this warning by clang analyzer by defining a default case: >> /src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value >> returned to caller >> return retval; >> >> Signed-off-by: Blue Swirl<blauwirbel@gmail.com> >> --- >> hw/openpic.c | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/hw/openpic.c b/hw/openpic.c >> index 26c96e2..4b883ac 100644 >> --- a/hw/openpic.c >> +++ b/hw/openpic.c >> @@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t >> *opp, int n_IRQ, uint32_t reg) >> case IRQ_IPVP: >> retval = opp->src[n_IRQ].ipvp; >> break; >> + default: >> case IRQ_IDE: >> retval = opp->src[n_IRQ].ide; >> break; > > Looks wrong, perhaps it should return 0? The only possible values are IRQ_IDE and IRQ_IPVP. The function is actually baroque, it's as easy to use read_IRQreg(opp, IRQ_DBL0 + n_dbl, IRQ_IPVP); as the shorter opp->src[IRQ_DBL0 + n_dbl].ipvp; The reason seems to be that write_IRQreg is more complex. I'd replace both with {read,write}_{ide,ipvp} without the switch.
On 05.09.2011, at 20:41, Blue Swirl wrote: > On Mon, Sep 5, 2011 at 6:48 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 09/04/2011 05:52 PM, Blue Swirl wrote: >>> >>> Avoid this warning by clang analyzer by defining a default case: >>> /src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value >>> returned to caller >>> return retval; >>> >>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com> >>> --- >>> hw/openpic.c | 1 + >>> 1 files changed, 1 insertions(+), 0 deletions(-) >>> >>> diff --git a/hw/openpic.c b/hw/openpic.c >>> index 26c96e2..4b883ac 100644 >>> --- a/hw/openpic.c >>> +++ b/hw/openpic.c >>> @@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t >>> *opp, int n_IRQ, uint32_t reg) >>> case IRQ_IPVP: >>> retval = opp->src[n_IRQ].ipvp; >>> break; >>> + default: >>> case IRQ_IDE: >>> retval = opp->src[n_IRQ].ide; >>> break; >> >> Looks wrong, perhaps it should return 0? > > The only possible values are IRQ_IDE and IRQ_IPVP. > > The function is actually baroque, it's as easy to use > read_IRQreg(opp, IRQ_DBL0 + n_dbl, IRQ_IPVP); > as the shorter > opp->src[IRQ_DBL0 + n_dbl].ipvp; > > The reason seems to be that write_IRQreg is more complex. I'd replace > both with {read,write}_{ide,ipvp} without the switch. I agree. Let me assemble a patch. Alex
On 09/04/2011 10:52 AM, Blue Swirl wrote: > Avoid this warning by clang analyzer by defining a default case: > /src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value > returned to caller > return retval; > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > hw/openpic.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/hw/openpic.c b/hw/openpic.c > index 26c96e2..4b883ac 100644 > --- a/hw/openpic.c > +++ b/hw/openpic.c > @@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t > *opp, int n_IRQ, uint32_t reg) > case IRQ_IPVP: > retval = opp->src[n_IRQ].ipvp; > break; > + default: > case IRQ_IDE: > retval = opp->src[n_IRQ].ide; > break; What's special about IDE? Shouldn't it return 0xffffffff as some other functions (e.g. openpic_gbl_read) do with unrecognized registers? Then there's openpic_src_read() which has still different behavior for the same registers. :-P Note that this function is only ever called with a constant in "reg". Since it's a static function and all call sites could have been verified, this could be considered a flaw in clang's analyzer. This workaround will prevent GCC from issuing a warning if a new caller is added that passes a different constant value. The best answer is probably to just get rid of this function and have the caller refer to opp->src[n_irq].whatever directly. write_IRQreg() could be split into something like set_src_ipvp() and set_src_ide(). -Scott
On Mon, Sep 12, 2011 at 5:12 PM, Scott Wood <scottwood@freescale.com> wrote: > On 09/04/2011 10:52 AM, Blue Swirl wrote: >> Avoid this warning by clang analyzer by defining a default case: >> /src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value >> returned to caller >> return retval; >> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> --- >> hw/openpic.c | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/hw/openpic.c b/hw/openpic.c >> index 26c96e2..4b883ac 100644 >> --- a/hw/openpic.c >> +++ b/hw/openpic.c >> @@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t >> *opp, int n_IRQ, uint32_t reg) >> case IRQ_IPVP: >> retval = opp->src[n_IRQ].ipvp; >> break; >> + default: >> case IRQ_IDE: >> retval = opp->src[n_IRQ].ide; >> break; > > What's special about IDE? Shouldn't it return 0xffffffff as some other > functions (e.g. openpic_gbl_read) do with unrecognized registers? Then > there's openpic_src_read() which has still different behavior for the > same registers. :-P > > Note that this function is only ever called with a constant in "reg". > Since it's a static function and all call sites could have been > verified, this could be considered a flaw in clang's analyzer. This > workaround will prevent GCC from issuing a warning if a new caller is > added that passes a different constant value. > > The best answer is probably to just get rid of this function and have > the caller refer to opp->src[n_irq].whatever directly. write_IRQreg() > could be split into something like set_src_ipvp() and set_src_ide(). Alex posted patches to that effect: http://lists.nongnu.org/archive/html/qemu-devel/2011-09/msg00947.html http://lists.nongnu.org/archive/html/qemu-devel/2011-09/msg00948.html
From 3c4f48ac6edafd3fae07d081e66b40e8144b1818 Mon Sep 17 00:00:00 2001
Message-Id: <3c4f48ac6edafd3fae07d081e66b40e8144b1818.1315150286.git.blauwirbel@gmail.com>
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwirbel@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwirbel@gmail.com>
From: Blue Swirl <blauwirbel@gmail.com>
Date: Sun, 4 Sep 2011 11:19:33 +0000
Subject: [PATCH 7/9] openpic: avoid a warning from clang analyzer
Avoid this warning by clang analyzer by defining a default case:
/src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value returned to caller
return retval;
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
hw/openpic.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/hw/openpic.c b/hw/openpic.c
index 26c96e2..4b883ac 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t *opp, int n_IRQ, uint32_t reg)
case IRQ_IPVP:
retval = opp->src[n_IRQ].ipvp;
break;
+ default:
case IRQ_IDE:
retval = opp->src[n_IRQ].ide;
break;
--
1.7.2.5
Avoid this warning by clang analyzer by defining a default case: /src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value returned to caller return retval; Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/openpic.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) break;