On Fri, Mar 03, 2017 at 03:50:31PM +0000, Peter Maydell wrote: > In read_insn_microblaze() we assemble 4 bytes into an 'unsigned > long'. If 'unsigned long' is 64 bits and the high byte has its top > bit set, then C's implicit conversion from 'unsigned char' to 'int' > for the shift will result in an unintended sign extension which sets > the top 32 bits in 'inst'. Add casts to prevent this. (Spotted by > Coverity, CID 1005401.)
I'm OK with this but perhaps it would have been more readable to change inst to uint32_t ? (All microblaze insns are 32bit). Anyway: Reviewed-by: Edgar E. Iglesias <[email protected]> Thanks, Edgar > > Signed-off-by: Peter Maydell <[email protected]> > --- > disas/microblaze.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/disas/microblaze.c b/disas/microblaze.c > index 91b30ac..407c0a3 100644 > --- a/disas/microblaze.c > +++ b/disas/microblaze.c > @@ -748,9 +748,11 @@ read_insn_microblaze (bfd_vma memaddr, > } > > if (info->endian == BFD_ENDIAN_BIG) > - inst = (ibytes[0] << 24) | (ibytes[1] << 16) | (ibytes[2] << 8) | > ibytes[3]; > + inst = ((unsigned)ibytes[0] << 24) | (ibytes[1] << 16) > + | (ibytes[2] << 8) | ibytes[3]; > else if (info->endian == BFD_ENDIAN_LITTLE) > - inst = (ibytes[3] << 24) | (ibytes[2] << 16) | (ibytes[1] << 8) | > ibytes[0]; > + inst = ((unsigned)ibytes[3] << 24) | (ibytes[2] << 16) > + | (ibytes[1] << 8) | ibytes[0]; > else > abort (); > > -- > 2.7.4 >
