Re: [Qemu-devel] [PATCH for-2.9 4/6] disas/microblaze: Avoid unintended sign extension

[Edgar E. Iglesias]

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 <address@hidden>

Thanks,
Edgar


> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  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
>