On Fri, Mar 03, 2017 at 04:02:07PM +0000, Peter Maydell wrote: > On 3 March 2017 at 15:58, Edgar E. Iglesias <edgar.igles...@gmail.com> wrote: > > 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). > > I thought about that, but it was more invasive a change than > I was happy with: you'd want to change not just 'inst' but > also the return type of read_insn_microblaze, the opcode_mask > field of struct op_code_struct, 'inst' and 'prev_inst' in > print_insn_microblaze, the type of the instr arguments to > get_field, get_field_imm, etc...
Ah, I see, didn't think about that... Cheers, Edgar
