Volatile structs compile but do not work as expected from interrupt handlers

[deadprogram]

I am trying to create a struct that wraps around some volatile members. The code compiles, but the interrupt handlers do not seem to be able to access them as expected, aka nothing seems to happen.

I tried this initially:

//go:volatile
type volatileByte byte

type RingBuffer struct {
   rxbuffer [bufferSize]volatileByte
   head volatileByte
   tail volatileByte
}

and also this

//go:volatile
type volatileByte byte

//go:volatile
type RingBuffer struct {
   rxbuffer [bufferSize]volatileByte
   head volatileByte
   tail volatileByte
}

And also this

//go:volatile
type RingBuffer struct {
   rxbuffer [bufferSize]byte
   head byte
   tail byte
}

They increased the size of my BIN file considerably, but did not work as expected.

[aykevl]

You could test it by adding -printir and finding the offending function. The relevant load and store should have the volatile flag, but probably doesn't.
Unlike most programming languages, IR puts volatile on loads and stores instead of on types.

The first code sample should be the correct one.

[deadprogram]

This is some of the Go code:

//go:volatile
type volatileByte byte

type RingBuffer struct {
	rxbuffer [bufferSize]volatileByte
	head     volatileByte
	tail     volatileByte
}

// NewRingBuffer returns a new ring buffer.
func NewRingBuffer() *RingBuffer { //buf [bufferSize]volatileByte) *RingBuffer {
	//return &RingBuffer{rxbuffer: buf}
	return &RingBuffer{}
}

// Used returns how many bytes in buffer have been used.
func (rb RingBuffer) Used() uint8 {
	return uint8(rb.head - rb.tail)
}

// Put stores a byte in the buffer.
func (rb RingBuffer) Put(val byte) {
	if rb.Used() != bufferSize {
		rb.head++
		rb.rxbuffer[rb.head%bufferSize] = volatileByte(val)
	}
}

//go:export SERCOM0_IRQHandler
func handleUART0() {
	// should reset IRQ
	UART0.Buffer.Put(byte((UART0.Bus.DATA & 0xFF)))
	UART0.Bus.INTFLAG |= sam.SERCOM_USART_INTFLAG_RXC
}

and some of the IR generated:

define internal void @"(machine.RingBuffer).Put"([64 x i8], i8, i8, i8, i8* %context, i8* %parentHandle) unnamed_addr !dbg !376 {
entry:
  %4 = insertvalue %machine.RingBuffer zeroinitializer, [64 x i8] %0, 0, !dbg !383
  %5 = insertvalue %machine.RingBuffer %4, i8 %1, 1, !dbg !383
  %6 = insertvalue %machine.RingBuffer %5, i8 %2, 2, !dbg !383
  %rb = alloca %machine.RingBuffer, !dbg !384
  store %machine.RingBuffer zeroinitializer, %machine.RingBuffer* %rb, !dbg !384
  store %machine.RingBuffer %6, %machine.RingBuffer* %rb, !dbg !385
  %7 = load %machine.RingBuffer, %machine.RingBuffer* %rb, !dbg !386
  %8 = extractvalue %machine.RingBuffer %7, 0, !dbg !387
  %9 = extractvalue %machine.RingBuffer %7, 1, !dbg !387
  %10 = extractvalue %machine.RingBuffer %7, 2, !dbg !387
  %11 = call i8 @"(machine.RingBuffer).Used"([64 x i8] %8, i8 %9, i8 %10, i8* undef, i8* undef), !dbg !387
  %12 = icmp ne i8 %11, 64, !dbg !388
  br i1 %12, label %if.then, label %if.done, !dbg !385

if.then:                                          ; preds = %entry
  %13 = getelementptr %machine.RingBuffer, %machine.RingBuffer* %rb, i32 0, i32 1, !dbg !389
  %14 = load volatile i8, i8* %13, !dbg !389
  %15 = add i8 %14, 1, !dbg !390
  store volatile i8 %15, i8* %13, !dbg !389
  %16 = getelementptr %machine.RingBuffer, %machine.RingBuffer* %rb, i32 0, i32 0, !dbg !391
  %17 = getelementptr %machine.RingBuffer, %machine.RingBuffer* %rb, i32 0, i32 1, !dbg !392
  %18 = load volatile i8, i8* %17, !dbg !392
  %19 = urem i8 %18, 64, !dbg !393
  %20 = sext i8 %19 to i32, !dbg !385
  call void @runtime.lookupBoundsCheck(i32 64, i32 %20, i8* undef, i8* null), !dbg !394
  %21 = getelementptr [64 x i8], [64 x i8]* %16, i32 0, i32 %20, !dbg !394
  store volatile i8 %3, i8* %21, !dbg !394
  br label %if.done, !dbg !385

if.done:                                          ; preds = %if.then, %entry
  ret void, !dbg !385
}

define internal i8 @"(machine.RingBuffer).Used"([64 x i8], i8, i8, i8* %context, i8* %parentHandle) unnamed_addr !dbg !395 {
entry:
  %3 = insertvalue %machine.RingBuffer zeroinitializer, [64 x i8] %0, 0, !dbg !398
  %4 = insertvalue %machine.RingBuffer %3, i8 %1, 1, !dbg !398
  %5 = insertvalue %machine.RingBuffer %4, i8 %2, 2, !dbg !398
  %rb = alloca %machine.RingBuffer, !dbg !399
  store %machine.RingBuffer zeroinitializer, %machine.RingBuffer* %rb, !dbg !399
  store %machine.RingBuffer %5, %machine.RingBuffer* %rb, !dbg !400
  %6 = getelementptr %machine.RingBuffer, %machine.RingBuffer* %rb, i32 0, i32 1, !dbg !401
  %7 = load volatile i8, i8* %6, !dbg !401
  %8 = getelementptr %machine.RingBuffer, %machine.RingBuffer* %rb, i32 0, i32 2, !dbg !402
  %9 = load volatile i8, i8* %8, !dbg !402
  %10 = sub i8 %7, %9, !dbg !403
  ret i8 %10, !dbg !404
}

define void @SERCOM0_IRQHandler() !dbg !558 {
entry:
  %0 = load %machine.UART*, %machine.UART** @machine.UART0, !dbg !559
  %1 = getelementptr %machine.UART, %machine.UART* %0, i32 0, i32 1, !dbg !560
  %2 = load %machine.RingBuffer*, %machine.RingBuffer** %1, !dbg !560
  %3 = load %machine.RingBuffer, %machine.RingBuffer* %2, !dbg !561
  %4 = load %machine.UART*, %machine.UART** @machine.UART0, !dbg !562
  %5 = getelementptr %machine.UART, %machine.UART* %4, i32 0, i32 0, !dbg !563
  %6 = load %"device/sam.SERCOM_USART_Type"*, %"device/sam.SERCOM_USART_Type"** %5, !dbg !563
  %7 = getelementptr %"device/sam.SERCOM_USART_Type", %"device/sam.SERCOM_USART_Type"* %6, i32 0, i32 15, !dbg !564
  %8 = load volatile i16, i16* %7, !dbg !564
  %9 = and i16 %8, 255, !dbg !565
  %10 = trunc i16 %9 to i8, !dbg !566
  %11 = extractvalue %machine.RingBuffer %3, 0, !dbg !567
  %12 = extractvalue %machine.RingBuffer %3, 1, !dbg !567
  %13 = extractvalue %machine.RingBuffer %3, 2, !dbg !567
  call void @"(machine.RingBuffer).Put"([64 x i8] %11, i8 %12, i8 %13, i8 %10, i8* undef, i8* undef), !dbg !567
  %14 = load %machine.UART*, %machine.UART** @machine.UART0, !dbg !568
  %15 = getelementptr %machine.UART, %machine.UART* %14, i32 0, i32 0, !dbg !569
  %16 = load %"device/sam.SERCOM_USART_Type"*, %"device/sam.SERCOM_USART_Type"** %15, !dbg !569
  %17 = getelementptr %"device/sam.SERCOM_USART_Type", %"device/sam.SERCOM_USART_Type"* %16, i32 0, i32 10, !dbg !570
  %18 = load volatile i8, i8* %17, !dbg !570
  %19 = or i8 %18, 4, !dbg !568
  store volatile i8 %19, i8* %17, !dbg !570
  ret void, !dbg !561
}

[deadprogram]

This is my WIP branch that tries to implement this, not yet successfully:

https://github.com/hybridgroup/tinygo/tree/feature/multiple-uarts

[aykevl]

(note: I edited your comment to add the llvm language tag to the produced IR, for readability)

[aykevl]

I see the issue: you're not using a pointer receiver in Used and Put.

[deadprogram]

That worked, thanks.

I thought that this would also be a way to correct the issue. I tried switching the UART to this:

type UART struct {
	Buffer RingBuffer
}

UART0 = &UART{}

However, even with that change, I still could not use the receiver unless I changed the function to func (rb *RingBuffer) Used() uint8 as you suggested. This requires the slightly more complex form:

UART0 = &UART{Buffer: NewRingBuffer()}

Any thoughts on that before I finish my multiple UART PR and submit it?

[aykevl]

The method receiver must be a pointer, otherwise you cannot modify it. A method receiver is really just a parameter and has almost the same semantics. For example, this would also not work:

func setValue(value int) {
    value = 5
}

Methods work the exact same way: when it is not a pointer, you cannot modify the value.

My suggestion for UART would be this:

type UART struct {
	Buffer RingBuffer
	Bus *sam.Something
}

var UART0 = &UART{Bus: ...}

func (uart *UART) ReadByte() {
    b := uart.Buffer.Get() // etc
}

I quickly tested it and it should work: https://play.golang.org/p/NA4xGfg2-NV

[deadprogram]

It works in regular Go, and compiles in TinyGo, but does not work as volatile within an interrupt handler as expected in TinyGo unless defined as:

type UART struct {
	Buffer *RingBuffer
	Bus *sam.Something
}

[aykevl]

Modified tags as this is clearly a bug. It seems like nested struct members do not get marked volatile.

[aykevl]

This should be fixed now.

[firelizzard18]

Should this comment be changed?

[deadprogram]

Hmm yes @firelizzard18 I think you are correct, this issue was resolved a while ago.

[aykevl]

That's definitely an old comment and should be removed.

[aykevl]

Here is a PR: #908