| Message ID | alpine.DEB.2.10.1612091729060.22778@sstabellini-ThinkPad-X260 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stefano, On 10/12/16 01:31, Stefano Stabellini wrote: > The rt variable can only be 0 or 7, no need to check if it's 15. Be careful, Coverity may point to dead code but it does not mean that deleting it is the right thing to do. The code which lead to this conclusion may be invalid. In this case, the dead code is happening because the rt variable has been miscalculated (see below). > > Coverity-ID: 1381835 > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > index c6f49a5..decd9dd 100644 > --- a/xen/arch/arm/decode.c > +++ b/xen/arch/arm/decode.c > @@ -58,11 +58,6 @@ static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) > /* Undefined opcodes */ > goto bad_thumb2; > > - /* Store/Load single data item */ > - if ( rt == 15 ) > - /* XXX: Rt == 15 is only invalid for store instruction */ > - goto bad_thumb2; > - Looking at the encoding of Thumb2 instruction (see encoding T2 for LDRB A8.8.70 in ARM DDI 0406C.c), this check is correct. However, the line 44 "rt = (hw2 >> 12) & 7" will not retrieve the correct register as it should be encoded with 4 bits. So the code should have been rt = (hw2 >> 12) & 0xf; Regards,
On Tue, 13 Dec 2016, Julien Grall wrote: > Hi Stefano, > > On 10/12/16 01:31, Stefano Stabellini wrote: > > The rt variable can only be 0 or 7, no need to check if it's 15. > > Be careful, Coverity may point to dead code but it does not mean that deleting > it is the right thing to do. The code which lead to this conclusion may be > invalid. In this case, the dead code is happening because the rt variable has > been miscalculated (see below). > > > > > Coverity-ID: 1381835 > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > > index c6f49a5..decd9dd 100644 > > --- a/xen/arch/arm/decode.c > > +++ b/xen/arch/arm/decode.c > > @@ -58,11 +58,6 @@ static int decode_thumb2(register_t pc, struct hsr_dabt > > *dabt, uint16_t hw1) > > /* Undefined opcodes */ > > goto bad_thumb2; > > > > - /* Store/Load single data item */ > > - if ( rt == 15 ) > > - /* XXX: Rt == 15 is only invalid for store instruction */ > > - goto bad_thumb2; > > - > > Looking at the encoding of Thumb2 instruction (see encoding T2 for LDRB > A8.8.70 in ARM DDI 0406C.c), this check is correct. > > However, the line 44 "rt = (hw2 >> 12) & 7" will not retrieve the correct > register as it should be encoded with 4 bits. So the code should have been > > rt = (hw2 >> 12) & 0xf; Indeed, you are right. I'll resubmit.
diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c index c6f49a5..decd9dd 100644 --- a/xen/arch/arm/decode.c +++ b/xen/arch/arm/decode.c @@ -58,11 +58,6 @@ static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) /* Undefined opcodes */ goto bad_thumb2; - /* Store/Load single data item */ - if ( rt == 15 ) - /* XXX: Rt == 15 is only invalid for store instruction */ - goto bad_thumb2; - if ( !load && sign ) /* Store instruction doesn't support sign extension */ goto bad_thumb2;
The rt variable can only be 0 or 7, no need to check if it's 15. Coverity-ID: 1381835 Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>