lkml.org 
[lkml]   [2015]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] MIPS: UAPI: Fix unrecognized opcode WSBH/DSBH/DSHD when using MIPS16.
Hi, Maciej, first of all, thank you for your time on this,
appreciate it.

Comments inline

On 5 September 2015 at 22:25, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Sat, 5 Sep 2015, Yousong Zhou wrote:
>
>> > This can't be true. The compiler does not intepret the contents of an
>> > inline asm and therefore cannot decide whether to inline a function
>> > containing one or not based on the lone presence or the absence of an
>> > assembly directive within.
>> >
>>
>> Most of the time I trust my compiler and never meddle with the
>> toolchain. Anyway I made a patch because it really did not work for
>> me. No big deal. It's not the end of world. It started with a
>> comment from OpenWrt packages feeds [1]. Actually this "unrecognized
>> opcode" problem have occurred within OpenWrt quite a few times before.
>>
>> [1] https://github.com/openwrt/packages/commit/1e29676a8ac74f797f8ca799364681cec575ae6f#commitcomment-12901931
>
> The bug certainly was there, it's just your analysis and consequently the
> fix that are wrong in the general case for some reason, maybe a buggy
> compiler.
>

This is the compiler "--version",

mips-openwrt-linux-gcc (OpenWrt/Linaro GCC 4.8-2014.04 r46763) 4.8.3

>> > It looks to me you're papering something over here -- when you use a
>> > `.set nomips16' directive the assembler will happily switch your
>> > instruction set in the middle of an instruction stream. Consequently if
>> > this function is (for whatever reason; it should not really) inlined in
>> > MIPS16 code, then you'll get a MIPS32 instruction in the middle, which
>> > will obviously be interpreted differently in the MIPS16 execution mode and
>> > is therefore surely a recipe for disaster.
>>
>> If by "papering" you mean "made up", then whatever. Yeah, it's
>> disaster, an "invalid instruction" abort.
>
> By "papering over" I mean forcing source code to compile successfully at
> the risk of producing incorrect binary code.

Learned a new phrase/idiom :)

>
>> > How did you test your change and made the conclusion quoted with your
>> > patch description?
>> >
>>
>> Compile the following program with a MIPS 24kc big endian variant compiler with
>> flag "-mips32r2 -mips16 -Os".
>>
>> #include <stdio.h>
>> #include <stdint.h>
>>
>> uint16_t __attribute__((noinline)) f(uint16_t v)
>> {
>> v = __cpu_to_le16(v);
>> return v;
>> }
>>
>> int main()
>> {
>> printf("%x\n", f(0xbeef));
>>
>> return 0;
>> }
>>
>> When only ".set nomips16" was specified in __arch_swab16(), this was output
>> from objdump.
>>
>> 242 004003e0 <f>:
>> 243 4003e0: 7c0410a0 wsbh v0,a0
>> 244 4003e4: e820ea31 swc2 $0,-5583(at)
>> 245 4003e8: 65006500 0x65006500
>> 246 4003ec: 65006500 0x65006500
>
> Quite obviously.
>
> For the record: the first instruction has been assembled in the regular
> MIPS mode and that propagated to symbol annotation. Consequently `f' is
> seen by `objdump' as a regular MIPS function and disassembles it all as
> such. You can put a global label immediately after the WSBH instruction
> in your source code to have the rest of the function disassembled
> correctly (of course this won't make this code work at the run time).

Thanks for the trick. Objdump really disassembled it
correctly.

>
>> __arch_swab16() was indeed inlined. That swc2 instruction can be got from
>> assembler with the following code (it's from the "-S" result of GCC).
>>
>> .set mips16
>> .set noreorder
>> .set nomacro
>> j $31
>> zeh $2
>>
>> When only nomips16 function attribute was specified, this time GCC failed with
>> unrecognized opcode
>>
>> /tmp/ccaGCouL.s: Assembler messages:
>> /tmp/ccaGCouL.s:21: Error: unrecognized opcode `wsbh $2,$4'
>>
>> The generated assembly was something in the following form. Looks like the
>> assembler did not automatically switch to MIPS32 mode when ".set arch=mip32r2"
>
> There's no switch to regular MIPS mode implied with `.set arch=mip32r2',
> the directive merely switches the ISA level, affecting both the regular
> MIPS and the MIPS16 mode (the MIPS32r2 ISA level adds extra MIPS16
> instructions too, e.g. ZEH is a new addition).

Agreed

>
>> .set mips16
>>
>> .ent f
>> .type f, @function
>> f
>> ...
>> .set push
>> .set arch=mips32r2
>> wsbh $2,$4
>> .pop
>> j $31
>> zeh $2
>> .end f
>> ...
>
> That's exactly the papered-over buggy code scenario I've been referring
> to above. This is clearly MIPS16 code: ZEH is a MIPS16 instruction only,
> there's no such regular MIPS counterpart. And it also obviously fails to
> assemble because on the contrary there's no MIPS16 WSBH instruction.
>

Yes, it won't assemble.

> Now if you stick `.set nomips16' just above WSBH, then this code will
> happily assemble, because this single instruction only (`.set pop' reverts
> any previous `.set' directives; I'm assuming you wrote above by hand and
> `.pop' is a typo) will assemble in the regular MIPS instruction mode. But
> if this code is ever reached, then the processor will still execute the
> machine code produced by the assembler from the WSBH instruction in the
> MIPS16 mode.

Yes, I hand-copied it from the output of "gcc -S" just to
show the form/pattern (the original output is too long for
this conversation). No, that `.pop' is not a typo (I just
did a double-check).

If I stick `.set nomips16` just above WSBH, that's just what
the original patch tries to do, papering over the fact that
it did not compile/assemble without it.

>
> For example the encoding of:
>
> wsbh $2, $4
>
> is (as you've shown in a dump above) 0x7c0410a0, which in the MIPS16 mode
> yields (in the big-endian mode):
>
> 00000000 <f>:
> 0: 7c04 sd s0,32(a0)
> 2: 10a0 b 144 <f+0x144>
>
> -- so this won't do what you might expect, you'll get a Reserved
> Instruction exception on the SD instruction, which is not supported in the
> 32-bit mode, and consequently SIGILL.

Agreed.

>
>> The patch was run tested on QEMU Malta and an router with Atheros
>> AR9331 SoC. I didn't test __arch_swab64() though. I have done many
>> other trial-and-error tests while preparing this patch. It was a mess
>> when I was sure I should expect some sensible behaviour from the
>> compiler while it actually just did not behave that way.
>
> I've compiled your example provided and as I stated in the original mail
> `__arch_swab16' is always produced as a separate function, whether `.set
> nomips16' is present in the inline assembly placed there or not. This is
> with (unreleased) GCC 6.0.0.
>
> However if you happen to have a buggy compiler that fails to emit
> `__arch_swab16' as a separate function despite the `nomips16' attribute,
> then it's better if the resulting generated assembly code fails to
> assemble rather than if it goes astray at the run time.

Now, my compiler refused to emit `__arch_swab16' as a
separate function even with the `nomips16' function
attribute. This is the kind of "mess" I just meant above, again.
I expect that it should emit a separate function and call it
with a jump observing that the caller `f' is in MIPS16 mode
yet the `__arch_swab16' is noMIPS16. sigh~~

>
>> > So setting aside the correctness issues discussed above, for MIPS16 code
>> > this has to be put out of line by the compiler, with all the usual
>> > overhead of a function call, causing a performance loss rather than a gain
>> > expected here. Especially as switching the ISA mode requires draining the
>> > whole pipeline so that subsequent instructions are interpreted in the new
>> > execution mode. This is expensive in performance terms.
>> >
>> > I'm fairly sure simply disabling these asms (#ifndef __mips16) and
>> > letting the compiler generate the right mask and shift operations from
>> > plain C code will be cheaper in performance terms and possibly cheaper in
>> > code size as well, especially in otherwise leaf functions where an extra
>> > function call will according to the ABI clobber temporaries. So I suggest
>> > going in that direction instead.
>>
>> I agree. Then you will provide the fix right? I am just curious
>> where that __mips16 should be placed or is it from compiler and
>> assembler?
>
> No, it's your bug after all. I think the last paragraph I wrote quoted
> above combined with the source code in question make it clear what to do.

Okay, I will try. Most of the time when textbooks read
clearly/obviously/apparently, things go astray ;)

>
> I have also checked what the difference in generated MIPS16 code is
> between a call to `__arch_swab16':
>
> 0000000c <f>:
> c: 64c4 save 32,ra
> e: 1800 0000 jal 0 <__arch_swab16>
> e: R_MIPS16_26 __arch_swab16
> 12: ec31 zeh a0
> 14: 6444 restore 32,ra
> 16: e8a0 jrc ra
>
> and equivalent generic code:
>
> 0000000c <f>:
> c: ec31 zeh a0
> e: 3280 sll v0,a0,8
> 10: 3482 srl a0,8
> 12: ea8d or v0,a0
> 14: e820 jr ra
> 16: ea31 zeh v0
>
> so the win is I think clear.
>
> Finally the MIPS64 `__arch_swab64' case does not matter as we have no
> MIPS16 support for 64-bit code in Linux, the toolchain will simply refuse
> to build it (only bare metal is supported).

Thanks for the information. Never played with MIPS64
before.

Regards

yousong


\
 
 \ /
  Last update: 2015-09-06 05:01    [W:2.669 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site