Summary: | sh*: Immediate signedness is incorrect in and/or/tst/xor #imm | ||
---|---|---|---|
Product: | binutils | Reporter: | Andrew Church <achurch+sourceware> |
Component: | binutils | Assignee: | Not yet assigned to anyone <unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | fenugrec, nickc |
Priority: | P2 | ||
Version: | 2.29 | ||
Target Milestone: | --- | ||
Host: | Target: | ||
Build: | Last reconfirmed: |
Description
Andrew Church
2018-01-11 06:10:53 UTC
Correct. To recap: 1) some opcodes need the sign-extended 8-bit immediate value: - add #imm, Rn (OK) - cmp/eq #imm, R0 (OK) - mov #imm, Rn (OK) 2) others take the zero-extended imm8 : - bitwise logic operators {and, and.b,...} : broken - trapa : broken 3) some I haven't personally checked : - ldrc / setrc - other DSP , SH2A, SH4A opcodes I would suggest reworking the relevant entries in opcodes/sh-opc.h (maybe replace IMM0_8 by IMM0_8S and IMM0_8U to ensure every occurence is verified manually; and add the proper handling in opcodes/sh-dis.c Is there a regression test for this already ? still present in binutils 2.34. How is still marked "Unconfirmed" ? The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=5c936ef50f02fe21a6e1306e30849b4487c65b2c commit 5c936ef50f02fe21a6e1306e30849b4487c65b2c Author: Nick Clifton <nickc@redhat.com> Date: Wed Apr 29 13:13:55 2020 +0100 Fix the disassmbly of SH instructions which have an unsigned 8-bit immediate operand. PR 22699 opcodes * sh-opc.h (IMM0_8): Replace with IMM0_8S and IMM0_8U. Use IMM0_8S for arithmetic insns and IMM0_8U for logical insns. * sh-dis.c (print_insn_sh): Change IMM0_8 case to IMM0_8S and add IMM0_8U case. gas * config/tc-sh.c (build_Mytes): Change operand type IMM0_8 to IMM0_8S and add support for IMM0_8U. * testsuite/gas/sh/sh4a.s: Add test of a logical insn using an unsigned 8-bit immediate. * testsuite/gas/sh/sh4a.d: Extended expected disassembly. Hi Guys, The PR was set to UNCONFIRMED because no maintainer has taken a look at the problem, confirmed that it existed and decided to fix it. I have now done this, and I agree with the suggestion to rename IMM0_8 to IMM0_8S and IMM0_8U. So I have checked in a patch which does this. I did not alter the LDRC, SETRC or other instructions however, on the assumption that they probably do take signed values. Cheers Nick Thanks for looking into this, Nick. I finally read up on the setrc/ldrc instructions and found a nice table summarizing immediate value formats, in https://www.renesas.com/en-us/doc/products/tool/doc/001/r20ut0704ej0101_shc_optimz.pdf "SuperH C/C++ Compiler Package V.9.04 User's Manual" . Table 11.10, page 850 . setrc and ldrc work with the "repeat count" register and take an unsigned 8-bit immediate, allowed range from 0x01-0xFF. Other than that, the patch looks good ! Oops, forgot to add, the detailed instruction descriptions for ldrc/setrc are in https://www.renesas.com/en-us/doc/products/mpumcu/001/rej09b0002_sh4aldsp.pdf (SH4AL-DSP Software Manual), respectively at 11.2.6 page 444, and 11.2.12 page 454. The "RC" register is "repeat count" and actually holds a 12-bit value; the 8U immediate is naturally zero-extended. The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9654d51a96ebe9ea1b2191afd47e1f3306317d2b commit 9654d51a96ebe9ea1b2191afd47e1f3306317d2b Author: Nick Clifton <nickc@redhat.com> Date: Wed Apr 29 16:09:38 2020 +0100 Also use unsigned 8-bit immediate values for the LDRC and SETRC insns. PR 22699 * sh-opc.h: Also use unsigned 8-bit immediate values for the LDRC and SETRC insns. OK - I have now update the entries for LDRC and SETRC as well. The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=241e541d00a315b3de4b1e25783139a05ad9dc47 commit 241e541d00a315b3de4b1e25783139a05ad9dc47 Author: Nick Clifton <nickc@redhat.com> Date: Wed Apr 29 17:18:56 2020 +0100 Update expected disassembly after recent update. PR 22699 * testsuite/gas/sh/sh4al-dsp.d: Update expected disassembly. |