Commit 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
added handling of microMIPS instructions to manipulate the stack
pointer. Unfortunately the decoding of the addiusp instruction was
incorrect, and performed a left shift by 2 bits to the raw immediate,
rather than decoding the immediate and then performing the shift, as
documented in the ISA.
This led to incomplete stack traces, due to incorrect frame sizes being
calculated. For example the instruction:
801faee0 <do_sys_poll>:
801faee0: 4e25 addiu sp,sp,-952
As decoded by objdump, would be interpreted by the existing code as
having manipulated the stack pointer by +1096.
Fix this by changing the order of decoding the immediate and applying
the left shift.
Fixes: 34c2f668d0f6 ("MIPS: microMIPS: Add unaligned access support.")
Signed-off-by: Matt Redfearn <[email protected]>
---
arch/mips/kernel/process.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
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);
} else {
tmp = (ip->halfword[0] >> 1);
info->frame_size = -(signed short)(tmp & 0xf);
--
2.7.4
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