does gcc9.2.0 emit incorrect code for functions with __attribute((interrupt)) ?

[xinx]

I need help for this issue I stumbled across by chance:

The C-code:

    static uint32_t _timer_ticks = 0;

    void m_timer_interrupt(void)    __attribute((interrupt ("machine")));

    void m_timer_interrupt(void)
    {
        _timer_ticks++;
    }

When I invoke gcc this way...

    gcc -Wa,-march=rv32imc  ...

... this assembler code is generated which looks very good to me (a4, a5 are saved at stack):

void m_timer_interrupt(void)
{
     112:   1141                    addi sp,sp,-16
     114:   c63a                    sw   a4,12(sp)
     116:   c43e                    sw   a5,8(sp)
    _timer_ticks++;
     118:   8181a783                lw   a5,-2024(gp) # 40018 <_timer_ticks>
     11c:   00178713                addi a4,a5,1
     120:   80e1ac23                sw   a4,-2024(gp) # 40018 <_timer_ticks>
     124:   0001                    nop
     126:   4732                    lw   a4,12(sp)
     128:   47a2                    lw   a5,8(sp)
     12a:   0141                    addi sp,sp,16
     12c:   30200073                mret
}

But when I use optimization...

    gcc -Wa,-march=rv32imc -O1 ...

... the generated code is strange (a5 is stored at stack but not used at all) and not entirely correct (content of t0, t1 are used but not saved at stack):

     0000013a <m_timer_interrupt>:
     {
          _timer_ticks++;

          13a: 1141                  addi sp,sp,-16
          13c: c63a                  sw   a4,12(sp)
          13e: c43e                  sw   a5,8(sp)
          140: 80c18293              addi t0,gp,-2036 # 4000c <__DATA_END__>
          144: 0002a703              lw   a4,0(t0)
          148: 00170313              addi t1,a4,1
          14c: 0062a023              sw   t1,0(t0)
          150: 4732                  lw   a4,12(sp)
          152: 47a2                  lw   a5,8(sp)
          154: 0141                  addi sp,sp,16
          156: 30200073              mret
     }

Of course, I can't believe that this is gcc's fault and that's why I am looking forward for an explanation and how to force gcc to save all used registers in an interrupt handler routine.

Thank you for your time and help!

[Jim Wilson]

On Tue, Jan 14, 2020 at 10:06 AM xinx <goxi...@gmail.com> wrote:
> ... the generated code is strange (a5 is stored at stack but not used at all) and not entirely correct (content of t0, t1 are used but not saved at stack):

The code emitted by the compiler uses a5, but linker relaxation
eliminates the need for a5 for address calculation. Linker relaxation
can't remove the save/restore of a5 on the stack though. So this is
unfortunate, but not wrong, just not optimized as well as possible,
and not easy to fix. You can see this if you compile with -S and look
at the assembly code.

The use of t0 and t1 look wrong, but I get correct code when I try it
with my copy of the compiler. You mentioned you have gcc-9.2, but you
didn't mention which exact gcc-9.2 you have. Different vendors have
different patch sets applied to their compilers, and each tree will
have different patches applied at different times. So I'd need a git
commit id or something to reproduce. Or maybe the bug was already
fixed, and you just need to update. If you don't see the t0 or t1
references in the assembly code, then maybe this is a linker
relaxation error, but the upstream binutils doesn't change register
allocation, so you would have to have some modified vendor binutils if
this is the problem.

I see you are using -Wa,-march=rv32imc which is odd. This will tell
the assembler to use a different architecture than the compiler, which
is unlikely to work. The compiler will pass march options to the
assembler, so you should just use -march directly.

Jim

[xinx]

Jim,

many thanks for your immediate and valuable response.

I use a self-compiled gcc 9.2.0 and did a "git clone --recursive https://github.com/riscv/riscv-gnu-toolchain" about a week ago (unfortunately I can't recall the git commit id anymore and I removed the clone already from my system).

gcc -v tells me:

Using built-in specs.
COLLECT_GCC=/opt/riscv/bin/riscv32-unknown-elf-gcc
COLLECT_LTO_WRAPPER=/usr/local/riscv_gcc9.2.0_rv32imc/bin/../libexec/gcc/riscv32-unknown-elf/9.2.0/lto-wrapper
Target: riscv32-unknown-elf
Configured with: /tmp/riscv/riscv-gnu-toolchain/riscv-gcc/configure --target=riscv32-unknown-elf --prefix=/opt/riscv_gcc9.2.0_rv32imc --disable-shared --disable-threads --enable-languages=c,c++ --with-system-zlib --enable-tls --with-newlib --with-sysroot=/opt/riscv_gcc9.2.0_rv32imc/riscv32-unknown-elf --with-native-system-header-dir=/include --disable-libmudflap --disable-libssp --disable-libquadmath --disable-libgomp --disable-nls --disable-tm-clone-registry --src=.././riscv-gcc --disable-multilib --with-abi=ilp32 --with-arch=rv32imc --with-tune=rocket 'CFLAGS_FOR_TARGET=-Os   -mcmodel=medlow' 'CXXFLAGS_FOR_TARGET=-Os   -mcmodel=medlow'
Thread model: single
gcc version 9.2.0 (GCC)

I am going to clone and compile again and I hope that I will get the same (good) code as you.

> I see you are using -Wa,-march=rv32imc which is odd.  This will tell
> the assembler to use a different architecture than the compiler, which
> is unlikely to work.

Thank you for pointing out that. I applied both -march=rv32imc and -Wa,-march=rv32imc when compiling my sources so that's why it worked.

> The compiler will pass march options to the

> assembler, so you should just use -march directly.

I see and changed it now.

Michael

[xinx]

Jim,

I rebuilt gcc tool chain but the results unfortunately are unchanged.

I cloned riscv-gnu-toolchain from github, resulting in these commit id's:

git log -1
commit f25fc5448f6d5b23b60350187641908ff4546cba
Submodule path 'riscv-binutils': checked out 'd91cadb45f3ef9f96c6ebe8ffb20472824ed05a7'
Submodule path 'riscv-gcc': checked out '54945eb8ad5a066da2d4e6a62ffeb513d341eb41'

So, what goes wrong in my environment? May I ask you to have a look at the flags I use when invokeing gcc?

Below are the results when applying -S and with/without optimization:

gcc -funroll-loops -fpeel-loops -fgcse-sm -fgcse-las -ffreestanding -ffunction-sections -fdata-sections -g -static -mstrict-align -std=gnu99 -fno-common -fno-builtin-printf -Wall -Werror -Winline -march=rv32imc -mabi=ilp32 -ffast-math -fomit-frame-pointer -fno-exceptions -fno-asynchronous-unwind-tables -fno-unwind-tables -S -c -I./include src.c

m_timer_interrupt:
.LFB4:
    .loc 1 59 1
    .cfi_startproc
    addi   sp,sp,-16
    .cfi_def_cfa_offset 16
    sw     a4,12(sp)
    sw     a5,8(sp)
    .cfi_offset 14, -4
    .cfi_offset 15, -8
    .loc 1 60 17
    lui    a5,%hi(_timer_ticks)
    lw     a5,%lo(_timer_ticks)(a5)
    addi   a4,a5,1
    lui    a5,%hi(_timer_ticks)
    sw     a4,%lo(_timer_ticks)(a5)
    .loc 1 61 1
    nop
    lw     a4,12(sp)
    .cfi_restore 14
    lw     a5,8(sp)
    .cfi_restore 15
    addi   sp,sp,16
    .cfi_def_cfa_offset 0
    mret
    .cfi_endproc
.LFE4:
    .size     m_timer_interrupt, .-m_timer_interrupt
    .section .text.m_external_interrupt,"ax",@progbits
    .align    1
    .globl    m_external_interrupt
    .type     m_external_interrupt, @function

gcc -O1 -funroll-loops -fpeel-loops -fgcse-sm -fgcse-las -ffreestanding -ffunction-sections -fdata-sections -g -static -mstrict-align -std=gnu99 -fno-common -fno-builtin-printf -Wall -Werror -Winline -march=rv32imc -mabi=ilp32 -ffast-math -fomit-frame-pointer -fno-exceptions -fno-asynchronous-unwind-tables -fno-unwind-tables -S -c -I./include src.c

m_timer_interrupt:
.LFB4:
    .loc 1 59 1
    .cfi_startproc
    addi   sp,sp,-16
    .cfi_def_cfa_offset 16
    sw     a4,12(sp)
    sw     a5,8(sp)
    .cfi_offset 14, -4
    .cfi_offset 15, -8
    .loc 1 60 5
    .loc 1 60 17 is_stmt 0
    lui    a5,%hi(.LANCHOR0)
    addi   t0,a5,%lo(.LANCHOR0)
    lw     a4,0(t0)
    addi   t1,a4,1
    sw     t1,0(t0)
    .loc 1 61 1
    lw     a4,12(sp)
    .cfi_restore 14
    lw     a5,8(sp)
    .cfi_restore 15
    addi   sp,sp,16
    .cfi_def_cfa_offset 0
    mret
    .cfi_endproc
.LFE4:
    .size     m_timer_interrupt, .-m_timer_interrupt
    .section  .text.m_external_interrupt,"ax",@progbits
    .align    1
    .globl    m_external_interrupt
    .type    m_external_interrupt, @function

Many thanks for your time and help. I do appreciate this very much.

Michael

[Kito Cheng]

Hi Xinx:

Thanks your report, I can reproduce this bug now, I filed a bug on GCC
bugzilla and will send in next few days :)

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

> --
> You received this message because you are subscribed to the Google Groups "RISC-V SW Dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sw-dev+un...@groups.riscv.org.
> To view this discussion on the web visit https://groups.google.com/a/groups.riscv.org/d/msgid/sw-dev/ff84b6d1-f752-416c-b79e-59de6f38b118%40groups.riscv.org.

[xinx]

Hi Kito,

many thanks for having a look to my reports and for your support and especially for filing a patch. I am very happy that the revealed gcc behavior got an explanation (and will get a fix) that fast.

As the underlying root cause looks quite basic to me I am wondering whether I am the only one :) who codes interrupt routines using C (aren't it Sifive presentations which point to this feature? :) or is register renaming a more recent optimization step, especially for interrupt functions? As the register context was not preserved during interrupt processing I expected more hits while looking for similar issues.

Anyhow and again, many thanks for your support!

xinx