Messages in this thread | | | Subject | Re: [PATCH 2/4] MIPS: microMIPS: Fix decoding of addiusp instruction | From | Matt Redfearn <> | Date | Wed, 1 Mar 2017 13:51:19 +0000 |
| |
Hi Maciej,
On 28/02/17 22:04, Maciej W. Rozycki wrote: > On Tue, 28 Feb 2017, Matt Redfearn wrote: > >> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c >> index 5b1e932ae973..6ba5b775579c 100644 >> --- a/arch/mips/kernel/process.c >> +++ b/arch/mips/kernel/process.c >> @@ -386,8 +386,9 @@ static int get_frame_info(struct mips_frame_info *info) >> >> if (ip->halfword[0] & mm_addiusp_func) >> { >> - tmp = (((ip->halfword[0] >> 1) & 0x1ff) << 2); >> - info->frame_size = -(signed short)(tmp | ((tmp & 0x100) ? 0xfe00 : 0)); >> + tmp = (ip->halfword[0] >> 1) & 0x1ff; >> + tmp = tmp | ((tmp & 0x100) ? 0xfe00 : 0); >> + info->frame_size = -(signed short)(tmp << 2); > Ugh, this is unreadable -- can you please figure out a way to fit it in > 79 columns? Perhaps by factoring this piece out?
Yeah, it's not pretty. I've got a v2 which refactors this into is_sp_move_ins, which makes it work the same way as is_ra_save_ins and perform the immediate interpretation there, instead. But I've kept that as a separate patch so as to keep the functional fix and refactor separate.
> > Also this: > > tmp = (ip->halfword[0] >> 1) & 0x1ff; > tmp = tmp | ((tmp & 0x100) ? 0xfe00 : 0); > > will likely result in better code without the conditional, e.g.: > > tmp = (((ip->halfword[0] >> 1) & 0x1ff) ^ 0x100) - 0x100; > > (the usual way to sign-extend). > > Maciej
Yes, that looks nicer.
Thanks, Matt
| |