[Bug c/112413] New: Wrong switch jump table offset

[Bug c/112413] New: Wrong switch jump table offset

[vincent.riviere at freesbee dot fr]

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

            Bug ID: 112413
           Summary: Wrong switch jump table offset
           Product: gcc
           Version: 13.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vincent.riviere at freesbee dot fr
  Target Milestone: ---

In some circumstances, gcc produces bad code for switch instruction.

Main goal of the testcase is to force gcc to produce a jump table.

$ cat swi.c
int g;

void f(int i)
{
        switch (i)
        {
                case 0: g = 6082; break;
                case 1: g = 9332; break;
                case 2: g = 5642; break;
                case 3: g = 1423; break;
                case 4: g = 2152; break;
                case 5: g = 6779; break;
                case 6: g = 7074; break;
                case 7: g = 8280; break;
        }
}

$ m68k-linux-gcc -Os -S -o - swi.c -mlong-jump-table-offsets -malign-int
#NO_APP
        .file   "swi.c"
        .text
        .align  2
        .globl  f
        .type   f, @function
f:
        move.l 4(%sp),%d0
        moveq #7,%d1
        cmp.l %d0,%d1
        jcs .L1
        lsl.l #2,%d0
        move.l .L4(%pc,%d0.l),%d0
        jmp %pc@(2,%d0:l)
        .balignw 4,0x284c   |Potential bug here
.L4:
        .long .L11-.L4
        .long .L10-.L4
        .long .L9-.L4
        .long .L8-.L4
        .long .L7-.L4
        .long .L6-.L4
        .long .L5-.L4
        .long .L3-.L4
.L11:
        move.l #6082,g
.L1:
        rts
.L10:
        move.l #9332,g
        jra .L1
...

As the jmp may not be aligned on a multiple of 4, the .balignw directive may
introduce a 2-byte filler, causing jmp to use a wrong offset.

Same happens with m68k-elf-gcc.

[Bug target/112413] Wrong switch jump table offset

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c                           |target

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I don't see any issues with the output of gcc. Are you sure this is not a
binutils gnu as issue where the offsets are done incorrectly there.

[Bug target/112413] Wrong switch jump table offset

[vincent.riviere at freesbee dot fr]

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

--- Comment #2 from Vincent Riviere <vincent.riviere at freesbee dot fr> ---
Cause is in gcc/config/m68k/linux.h, macro ASM_RETURN_CASE_JUMP:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/m68k/linux.h;h=2e1cb5498b86f053d1e9b7c530648dfa186ca4c4;hb=HEAD#l96

jmp %%pc@(2,%0:w)

Offset 2 is hardcoded in the macro. Ideally, it should be replaced with the
label of the first jump table entry. But I guess it isn't possible inside that
macro.

A solution is to force ADDR_VEC_ALIGN to 0, in order to completely disable the
jump table alignment. That's consistent with ASM_RETURN_CASE_JUMP expectations.

#define ADDR_VEC_ALIGN(ADDR_VEC) 0

This should be done for all m68k targets.

[Bug target/112413] Wrong switch jump table offset

[vincent.riviere at freesbee dot fr]

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

--- Comment #3 from Vincent Riviere <vincent.riviere at freesbee dot fr> ---
(In reply to Andrew Pinski from comment #1)
> I don't see any issues with the output of gcc. Are you sure this is not a
> binutils gnu as issue where the offsets are done incorrectly there.

Yes, I'm sure it's a gcc bug.

With the testcase I initially provided, by chance it's the favourable case.
But if I artificially add a misalignment with a nop, for example, the wrong
result appears:

$ cat swi2.c
int g;

void f(int i)
{
        asm("nop");
        switch (i)
        {
                case 0: g = 6082; break;
                case 1: g = 9332; break;
                case 2: g = 5642; break;
                case 3: g = 1423; break;
                case 4: g = 2152; break;
                case 5: g = 6779; break;
                case 6: g = 7074; break;
                case 7: g = 8280; break;
        }
}

$ m68k-linux-gcc -Os -c swi2.c -mlong-jump-table-offsets -malign-int
$ m68k-linux-objdump -d swi2.o

swi2.o:    file format elf32-m68k


Disassembly of section .text:

00000000 <f>:
   0:   202f 0004       movel %sp@(4),%d0
   4:   4e71            nop
   6:   7207            moveq #7,%d1
   8:   b280            cmpl %d0,%d1
   a:   6536            bcss 42 <f+0x42>
   c:   e588            lsll #2,%d0
   e:   203b 0808       movel %pc@(18 <f+0x18>,%d0:l),%d0  |right offset
  12:   4efb 0802       jmp %pc@(16 <f+0x16>,%d0:l)  |wrong offset
  16:   284c            moveal %a4,%a4   |harmful filler
  18:   0000 0020       orib #32,%d0
  1c:   0000 002c       orib #44,%d0
  20:   0000 0038       orib #56,%d0

See that:
- actual jump table starts at offset 0x18
- at offset 0x16, a useless "moveal %a4,%a4" instruction is inserted as filler
- at offset 0xe, offset 0x18 is used appropriately for label .L4. So the right
jump table entry is properly read.
- but at offset 0x12, a *wrong* offset 0x16 is used for the jump. That's
actually the offset of the filler, while it should be 0x18 for label .L4.

This can't work:
- the jump table offsets are computed from the start of the jump table
- but jmp, with that "2" hardcoded as offset, expects offsets being relative to
the address right after itself.
So if a filler is inserted between jmp and actual table contents, as in the
example above, the jump occurs to a wrong address.

[Bug target/112413] Wrong switch jump table offset

[mikpelinux at gmail dot com]

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

Mikael Pettersson <mikpelinux at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpelinux at gmail dot com

--- Comment #4 from Mikael Pettersson <mikpelinux at gmail dot com> ---
Does the `.balignw` filler disappear if you drop `-malign-int`?

[Bug target/112413] Wrong switch jump table offset

[mikpelinux at gmail dot com]

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

--- Comment #5 from Mikael Pettersson <mikpelinux at gmail dot com> ---
Created attachment 56561
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56561&action=edit
proposed fix, only compile-tested for now

[Bug target/112413] Wrong switch jump table offset

[vincent.riviere at freesbee dot fr]

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

--- Comment #6 from Vincent Riviere <vincent.riviere at freesbee dot fr> ---
(In reply to Mikael Pettersson from comment #4)
> Does the `.balignw` filler disappear if you drop `-malign-int`?

No, it stays, but its value becomes 2, so it doesn't cause trouble.

[Bug target/112413] Wrong switch jump table offset

[mikpelinux at gmail dot com]

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

--- Comment #7 from Mikael Pettersson <mikpelinux at gmail dot com> ---
Patch posted after bootstrap and regression testing on m68k-linux-gnu:
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/640177.html

[Bug target/112413] Wrong switch jump table offset

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:eea25179d8d1406685b8b0995ba841605f895283

commit r14-6417-geea25179d8d1406685b8b0995ba841605f895283
Author: Mikael Pettersson <mikpelinux@gmail.com>
Date:   Mon Dec 11 08:40:41 2023 -0700

    [PATCH] wrong code on m68k with -mlong-jump-table-offsets and -malign-int
(PR target/112413)

    On m68k the compiler assumes that the PC-relative jump-via-jump-table
    instruction and the jump table are adjacent with no padding in between.

    When -mlong-jump-table-offsets is combined with -malign-int, a 2-byte
    nop may be inserted before the jump table, causing the jump to add the
    fetched offset to the wrong PC base and thus jump to the wrong address.

    Fixed by referencing the jump table via its label. On the test case
    in the PR the object code change is (the moveal at 16 is the nop):

        a:  6536            bcss 42 <f+0x42>
        c:  e588            lsll #2,%d0
        e:  203b 0808       movel %pc@(18 <f+0x18>,%d0:l),%d0
    -  12:  4efb 0802       jmp %pc@(16 <f+0x16>,%d0:l)
    +  12:  4efb 0804       jmp %pc@(18 <f+0x18>,%d0:l)
       16:  284c            moveal %a4,%a4
       18:  0000 0020       orib #32,%d0
       1c:  0000 002c       orib #44,%d0

    Bootstrapped and tested on m68k-linux-gnu, no regressions.

    Note: I don't have commit rights to I would need assistance applying this.

            PR target/112413
    gcc/

            * config/m68k/linux.h (ASM_RETURN_CASE_JUMP): For
            TARGET_LONG_JUMP_TABLE_OFFSETS, reference the jump table
            via its label.
            * config/m68k/m68kelf.h (ASM_RETURN_CASE_JUMP): Likewise.
            * config/m68k/netbsd-elf.h (ASM_RETURN_CASE_JUMP): Likewise.

[Bug target/112413] Wrong switch jump table offset

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED
                 CC|                            |law at gcc dot gnu.org

--- Comment #9 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Fixed on the trunk.  No plans to backport.

[Bug target/112413] Wrong switch jump table offset

[sjames at gcc dot gnu.org]

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

Sam James <sjames at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |14.0