[Bug target/55146] New: jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[Bug target/55146] New: jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[marcin.bukat at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

             Bug #: 55146
           Summary: jumptables with byte entries produce wrong code with
                    -Os/-O2 for SH-1
    Classification: Unclassified
           Product: gcc
           Version: 4.6.3
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: marcin.bukat@gmail.com


Created attachment 28580
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28580
testcase C file

Switch statement with enough cases inside is translated to jumptable. If the
offsets are small enough, byte entries are used. If there is odd number of
cases this implies padding byte (since all SH instructions are 2 byte wide). In
the corner case this padding byte can cause the offset to switch from positive
to negative since move.b instruction used to fetch jumptable entry sign extends
the byte. This bug is very old and dates back at least to 4.0.3 days. See
attachment.
When compiled with sh-elf-gcc -Os -c test-jens.c -o sh-test.o it produces wrong
jumping code. http://pastebin.com/ZgJK3bgS has slightly commented disassembly
to point out what I mean.

The workaround is to decrease MIN_OFFSET by one for byte case in
gcc/config/sh/sh.h . This is not a proper fix though.

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |sh-elf
           Severity|critical                    |normal

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |olegendo at gcc dot gnu.org

--- Comment #1 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-12-10 09:15:15 UTC ---
(In reply to comment #0)
> Created attachment 28580 [details]
> testcase C file
> 
> Switch statement with enough cases inside is translated to jumptable. If the
> offsets are small enough, byte entries are used. If there is odd number of
> cases this implies padding byte (since all SH instructions are 2 byte wide). In
> the corner case this padding byte can cause the offset to switch from positive
> to negative since move.b instruction used to fetch jumptable entry sign extends
> the byte. This bug is very old and dates back at least to 4.0.3 days. See
> attachment.
> When compiled with sh-elf-gcc -Os -c test-jens.c -o sh-test.o it produces wrong
> jumping code. http://pastebin.com/ZgJK3bgS has slightly commented disassembly
> to point out what I mean.
> 
> The workaround is to decrease MIN_OFFSET by one for byte case in
> gcc/config/sh/sh.h . This is not a proper fix though.

Thanks for reporting this.  Could you please attach the file that you had on
pastebin?  The link seems dead.

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[marcin.bukat at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

--- Comment #2 from wodz <marcin.bukat at gmail dot com> 2012-12-10 21:19:48 UTC ---
Created attachment 28919
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28919
slightly commented dissasembly

This is the file which was on pastebin (link dead now unfortunately)

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-01-30
                 CC|                            |kkojima at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> 2013-01-30 22:38:34 UTC ---
I've checked this issue on rev. 195555 (GCC 4.8) and the extu.b insn is emitted
for the test case.

On the 4.7 branch, the error is present.  There the code for the insn
'casesi_worker_1' seems to have a bug:

Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md    (revision 195590)
+++ gcc/config/sh/sh.md    (working copy)
@@ -9093,7 +9093,8 @@
     case QImode:
       if (ADDR_DIFF_VEC_FLAGS (diff_vec).offset_unsigned)
     return \"mov.b    @(r0,%1),%0\;extu.b    %0,%0\";
-      return \"mov.b    @(r0,%1),%0\";
+      else
+        return \"mov.b    @(r0,%1),%0\";
     default:
       gcc_unreachable ();
     }

However, that still doesn't fix the problem.  For some reason 'offset_unsigned'
is always zero and thus the insn doesn't get the chance to output the correct
insn sequence.  The macro 'CASE_VECTOR_SHORTEN_MODE' in sh.h which is supposed
to set the 'offset_unsigned' field to non-zero looks exactly the same on 4.7
and 4.8, so I guess the bug must be somewhere else.

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

--- Comment #4 from Oleg Endo <olegendo at gcc dot gnu.org> 2013-02-04 22:43:50 UTC ---
Author: olegendo
Date: Mon Feb  4 22:43:47 2013
New Revision: 195743

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195743
Log:
    PR target/55146
    * gcc.target/sh/pr55146.c: New.


Added:
    trunk/gcc/testsuite/gcc.target/sh/pr55146.c
Modified:
    trunk/gcc/testsuite/ChangeLog

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

--- Comment #5 from Oleg Endo <olegendo at gcc dot gnu.org> 2013-03-09 13:00:33 UTC ---
Kaz, do you have any suggestion what to do with this issue on < 4.8 ?
Although I haven't looked deeper into the problem, I'm afraid the actual bug is
not in the sh-target but somewhere else ...

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

--- Comment #6 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2013-03-10 00:23:38 UTC ---
(In reply to comment #5)

I think that your observation is correct.  It looks the offset_unsigned
field is handled wrongly somewhere on 4.7 and earlier.  I can't find
suspicious part, though.

[Bug target/55146] jumptables with byte entries produce wrong code with -Os/-O2 for SH-1

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55146

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #7 from Oleg Endo <olegendo at gcc dot gnu.org> ---
GCC 4.7 branch has been closed.  I'm closing this PR, since the problem seems
to have been fixed in 4.8.