compiler question: What could be causing these false positive heap escapes?

[soypat]

Question

I have a program that escapes variables which should not be escaped to heap. When I try to build an MWE I'm surprised how edge casey the escape condition seems. What could be causing this?

I've even changed the spi type on the Device to be a concrete SPIbb type and this still happens.

$ tinygo build -target=pico -print-allocs='cyw43439' ./reproducer
... other escapes
/home/pato/src/tg/cyw43439/cy_io.go:291:6: object allocated on the heap: escapes at line 311 // LINE X escapes on LINE Y
/home/pato/src/tg/cyw43439/bbspi.go:48:6: object allocated on the heap: escapes at unknown line // LINE A

Line numbers may vary

Observations

If we comment out LINE B (in second dropdown) line both heap allocations dissapear. For some reason this line causes heap allocations of both aux variable and the buf variable in the Write32S Device's method, which is a [4]byte type.
buf ONLY escapes when passed in as the second r argument to Tx.

Furthermore, I can't replicate these heap allocations by writing a function that performs nearly identical operations with buf...

Code

Attempt at minimal reproducer

REMOVE return TO OBSERVE BEHAVIOR!

package main

import (
	"encoding/binary"

	"github.com/soypat/cyw43439"
)

func main() {
	spi, cs, wl, irq := cyw43439.PicoWSpi(0)
	err := Write32S(spi, cyw43439.FuncBus, 0, 0)
	println(err.Error())
	return // REMOVE RETURN TO OBSERVE BEHAVIOR.
	_, _, _ = cs, wl, irq
	dev := cyw43439.NewDevice(spi, cs, wl, irq, irq)
	dev.Write32S(cyw43439.FuncBus, 0, 0)
}

// Attempt at a MWE of the behavior seen in Write32S.
//
//go:noinline
func Write32S(spi *cyw43439.SPIbb, fn cyw43439.Function, addr, val uint32) error {
	cmd := uint32(fn)
	var buf [4]byte

	binary.BigEndian.PutUint32(buf[:], cmd)
	err := spi.Tx(buf[:], nil)
	if err != nil {
		return err
	}
	binary.BigEndian.PutUint32(buf[:], (val))
	err = spi.Tx(buf[:], nil)
	if err != nil {
		return err
	}
	// Read Status.
	buf = [4]byte{}
	spi.Tx(buf[:], buf[:])
	return err
}

SPIbb.Tx, problematic assignment

func (s *SPIbb) Tx(w []byte, r []byte) (err error) {
	var aux [1]byte // LINE A
	mocking := s.MockTo != nil
	if len(w) != 0 {
		if len(r) == 0 {
			r = aux[:] // LINE B
		}
		r[0] = s.firstTransfer(w[0], mocking)
		w = w[1:]
		r = r[1:]
	}
	switch {
	case len(r) == len(w):
		for i, b := range w {
			r[i] = s.transfer(b, mocking)
			_, _ = i, b
		}
	case len(w) != 0:
		for _, b := range w {
			s.transfer(b, mocking)
		}
	case len(r) != 0:
		for i := range r {
			_ = i
			r[i] = s.transfer(0, mocking)
		}
	default:
		err = errors.New("unhandled SPI buffer length mismatch case")
	}
	return err
}

// Write32S writes register and swaps big-endian 16bit word length. Used only at initialization.
func (d *Device) Write32S(fn Function, addr, val uint32) error {
	cmd := make_cmd(true, true, fn, addr, 4)
	var buf [4]byte // LINE X
	d.csLow()
	if sharedDATA {
		d.sharedSD.Configure(machine.PinConfig{Mode: machine.PinOutput})
	}
	binary.BigEndian.PutUint32(buf[:], swap32(cmd))
	err := d.spi.Tx(buf[:], nil)
	if err != nil {
		d.csHigh()
		return err
	}
	binary.BigEndian.PutUint32(buf[:], swap32(val))
	err = d.spi.Tx(buf[:], nil)
	Debug("cyw43_write_reg_u32_swap", fn.String(), addr, "=", val, err)
	if err != nil || !d.enableStatusWord {
		d.csHigh()
		return err
	}
	// Read Status.
	buf = [4]byte{}
	d.spi.Tx(buf[:], buf[:]) // LINE Y
	d.csHigh()
	status := Status(swap32(binary.BigEndian.Uint32(buf[:])))
	if status.DataUnavailable() {
		println("got status:", status)
		err = ErrDataNotAvailable
	}
	return err
}

[aykevl]

How can I reproduce this issue? That would help a lot to narrow down the issue.

I suspect the issue is a phi node. Those can happen in a loop, and in that case it wouldn't be correct to optimize the heap allocation to a stack allocation (it would reuse the allocation). However, in this case there is no loop and converting to a stack allocation would be correct.

[soypat]

To reproduce:

git clone [email protected]:soypat/cyw43439.git
cd cyw43439
git checkout  6a473f8b76
tinygo build -target=pico -print-allocs='cyw43439' ./cmd/reproducer/

The output:

/home/pato/src/cyw43439/cy43439.go:148:16: object allocated on the heap: object size 4248 exceeds maximum stack allocation size 256
/home/pato/src/cyw43439/cy_io.go:303:45: object allocated on the heap: escapes at line 303
/home/pato/src/cyw43439/cy_io.go:290:6: object allocated on the heap: escapes at line 310
/home/pato/src/cyw43439/cy43439.go:80:21: object allocated on the heap: escapes at line 80
/home/pato/src/cyw43439/cy43439.go:74:14: object allocated on the heap: escapes at line 87
/home/pato/src/cyw43439/bbspi.go:46:6: object allocated on the heap: escapes at unknown line

[dgryski]

(BTW, I'd love to fix the max stack size allocation issue: #3331 )

[dgryski]

I wonder if it's because buf is passed twice and ends up aliasing r and w, and so when w is passed to firstTransfer the whole thing escape? (This is a wild guess based on just looking at the code...)

BTW, is this still happening? I've been thinking of looking at some escape analysis tweaks.

[dgryski]

It doesn't like the [...]byte -> []byte conversion. See here: #2237

[aykevl]

I see this code in bbspi.go:

	var aux [1]byte
	mocking := s.MockTo != nil
	if len(w) != 0 {
		if len(r) == 0 {
			r = aux[:]
		}
		r[0] = s.firstTransfer(w[0], mocking)
		w = w[1:]
		r = r[1:]
	}

Here, the aux value is never used (only written to). It can easily be removed entirely:

	mocking := s.MockTo != nil
	if len(w) != 0 {
		result := s.firstTransfer(w[0], mocking)
		if len(r) != 0 {
			r[0] = result
		}
		w = w[1:]
		r = r[1:]
	}

And in cy_io.go, in Write32S, a way to avoid the heap allocation of buf would be to put it in the Device struct directly so it can be reused across calls.

While the first one (in bbspi.go) could perhaps be optimized by the compiler (with some dataflow analysis), the second one is going to be very difficult: the compiler doesn't know that the d.spi.Tx method does not let the parameters escape and so cannot optimize the heap allocations away.

[deadprogram]

@soypat has this question been answered now?

[soypat]

Yes! @aykevl's code correction looks to be correct and a clear cut solution to the heap alloc :)

Discussion about the why of the heap alloc should probably continue here: #2237

[deadprogram]

Thanks everyone! Now closing.