Bug 22699 - sh*: Immediate signedness is incorrect in and/or/tst/xor #imm
Summary: sh*: Immediate signedness is incorrect in and/or/tst/xor #imm
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.29
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
Depends on:
Reported: 2018-01-11 06:10 UTC by Andrew Church
Modified: 2020-04-29 16:19 UTC (History)
2 users (show)

See Also:
Last reconfirmed:


Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Church 2018-01-11 06:10:53 UTC
When objdump disassembles SuperH (sh2, sh4, etc.) instructions of the form "AND/OR/TST/XOR #imm,R0", it treats the immediate as a signed 8-bit value, but in fact the value is unsigned.

To reproduce: echo 'tst #128,r0' |sh4-as -o foo.o; sh4-objdump -d foo.o

Expected output:
   0:   80 c8           tst     #128,r0

Actual output:
   0:   80 c8           tst     #-128,r0
Comment 1 fenugrec 2018-01-25 16:41:21 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 ?
Comment 2 fenugrec 2020-04-28 16:03:30 UTC
still present in binutils 2.34. How is still marked "Unconfirmed" ?
Comment 3 cvs-commit@gcc.gnu.org 2020-04-29 12:15:03 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:


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.
Comment 4 Nick Clifton 2020-04-29 12:17:49 UTC
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.

Comment 5 fenugrec 2020-04-29 13:56:18 UTC
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 !
Comment 6 fenugrec 2020-04-29 14:03:37 UTC
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.
Comment 7 cvs-commit@gcc.gnu.org 2020-04-29 15:10:09 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:


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.
Comment 8 Nick Clifton 2020-04-29 15:15:24 UTC
OK - I have now update the entries for LDRC and SETRC as well.
Comment 9 cvs-commit@gcc.gnu.org 2020-04-29 16:19:29 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:


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.