AVR fails with `-opt=1`

[aykevl]

I'm afraid that #2428 broke AVR support. The following command worked before #2428, but doesn't work anymore after it:

tinygo run -target=atmega1284p -scheduler=tasks -opt=1 ./testdata/alias.go

I found it while investigating some AVR issues in #2365 and bisected it to 92150bd.

@zdima

[zdima]

@aykevl , I can see it doesn't work with -opt 0 or 1 on my Uno as well.

I was able to trace the issue to a single line of code that when commented, all is working again:

tinygo/src/machine/machine_avr.go

Line 155 in fcd8835

Ticks += tickNanos

I can't find a method to execute this single line successfully with -opt=1. I tried to use volatile and move to separate function.
None worked for me.

I also try to replace it with various code, and what interesting, controller will crash when Ticks updated with changing value. However, controller won't crash when Ticks updated with the same value:

This code doesn't crash controller:

        interrupt.New(avr.IRQ_TIMER0_OVF, func(i interrupt.Interrupt) {
                // Ticks += tickNanos
                v := int64(55)
                Ticks = v
        })

This code doesn't crash controller:

        interrupt.New(avr.IRQ_TIMER0_OVF, func(i interrupt.Interrupt) {
                // Ticks += tickNanos
                v := int64(Ticks)
                Ticks = v
        })

This code crash controller:

        interrupt.New(avr.IRQ_TIMER0_OVF, func(i interrupt.Interrupt) {
                // Ticks += tickNanos
                v := int64(Ticks+1)
                Ticks = v
        })

Any idea why this happening and why optimization have such effect?
I have run above with:
tinygo version 0.22.0-dev darwin/arm64 (using go version go1.17.3 and LLVM version 11.1.0)

[aykevl]

I'm not sure what's going on, but I did find an issue. You have to read the machine.Ticks variable using a volatile read. At the moment, runtime.ticks is optimized to the following:

define internal fastcc void @runtime.ticks() unnamed_addr addrspace(1) {
entry:
  %0 = call fastcc addrspace(1) i8 @"runtime/interrupt.Disable"()
  call fastcc addrspace(1) void @"runtime/interrupt.Restore"(i8 %0)
  ret void
}

This is equivalent to the following Go code:

// ticks return time since start in nanoseconds
func ticks() {
	state := interrupt.Disable()
	interrupt.Restore(state)
	return
}

Because you don't read machine.Ticks using a volatile operation, the compiler assumes it won't change unexpectedly and therefore optimizes the read of the variable.

This doesn't fix the current bug, unfortunately.

[aykevl]

Any idea why this happening and why optimization have such effect?

To be clear: optimizations shouldn't have this effect. It likely indicates a bug that is also possible with -opt=z (the default) but is simply hidden.

It is possible we're triggering an AVR backend bug here, but that would not be my first guess.

[aykevl]

I think I found the bug, and it has nothing to do with #2428 but with the fact that interrupts don't work correctly on AVR. This might be a LLVM bug. In short, the interrupt handler __vector_TIMER0_OVF (which calls machine.InitMonotonicTimer$1) does not seem to be lowered as an interrupt handler. I'll investigate more later today.

[aykevl]

More proof that this is a bug that already existed but just got more visible with #2428. tinygo flash -target=arduino -opt=1 examples/echo doesn't work on branch release-0.21.0. (Reading from the UART requires interrupts).

[aykevl]

I have found the source of the bug in LLVM and will send a patch soon.

[aykevl]

Here is a fix: https://reviews.llvm.org/D116551
Essentially, whenever an interrupt handler calls a function, that's a miscompile. We normally don't hit this because inlining avoids this situation. But with -opt=1, inlining is explicitly disabled.

So... in the meantime, I think we should just avoid -opt=1 with AVR. There might be workaround possible from TinyGo but I don't really want to go down that route if it's something that should really be fixed in LLVM.

[deadprogram]

Should we add some code to prevent this in https://github.com/tinygo-org/tinygo/blob/dev/compileopts/options.go#L50 ?

[aykevl]

I have committed https://reviews.llvm.org/D116551 so this should be fixed once we upgrade to LLVM 14 (released in about 2-3 months probably). We could add it, but honestly I don't think it's worth the effort.