runtime: skip negative sleep durations in sleepTicks

[eliasnaur]

I can't get make test to run on my Nix/macOS setup, but here's a program that hangs without this change on target pico:

package main

import (
	"fmt"
	"time"
)

func main() {
	timer := time.NewTicker(10 * time.Millisecond)
	defer timer.Stop()
	ticks := 0
	for i := 0; i < 10; i++ {
		select {
		case <-timer.C:
			ticks++
		}
	}
	for i := 0; i < 10; i++ {
		secondary()
		fmt.Println("tick", ticks, i)
	}
	fmt.Println("PASS")
}

func secondary() {
	ticks := 0
	timer2 := time.NewTicker(10 * time.Microsecond)
	defer timer2.Stop()
	done := make(chan struct{})
loop:
	for i := 0; i < 100; i++ {
		select {
		case <-timer2.C:
			ticks++
		case <-done:
			break loop
		}
	}
	fmt.Println("tick2", ticks)
}

I tried minimizing it even further, but removing a print or dummy channel makes the hang disappear.

[eliasnaur]

The CI failure

compiler_test.go:254: could not parse test case basic.go: could not compile: /Users/runner/work/tinygo/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)

is surprising. It seems that TinyGo fails to compile the expression

timeLeft := ^timeUnit(0)

[dgryski]

WorksForMe(tm):

~/go/src/github.com/dgryski/bug/timeunit $ go run main.go
-1
~/go/src/github.com/dgryski/bug/timeunit $ tinygo run main.go
-1
~/go/src/github.com/dgryski/bug/timeunit $ cat main.go
package main

type timeUnit int64

func main() {
	t := ^timeUnit(0)
	println(t)
}

[eliasnaur]

Works for me locally as well, but not in CI. I can reproduce the error locally with this diff:

diff --git a/src/runtime/scheduler.go b/src/runtime/scheduler.go
index 41186ef1..e77796f7 100644
--- a/src/runtime/scheduler.go
+++ b/src/runtime/scheduler.go
@@ -200,7 +200,7 @@ func scheduler() {
                                continue
                        }

-                       var timeLeft timeUnit
+                       timeLeft := ^timeUnit(0)
                        if sleepQueue != nil {
                                dt := (now - sleepQueueBaseTime)
                                timeLeft = timeUnit(sleepQueue.Data) - dt

and

$ go test ./compiler
# github.com/tinygo-org/tinygo/compiler.test
ld: warning: directory not found for option '-L/opt/homebrew/opt/llvm@17/lib'
ld: warning: directory not found for option '-L/opt/homebrew/opt/llvm@17/lib'
--- FAIL: TestCompilerErrors (0.12s)
    compiler_test.go:254: could not parse test case errors.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
--- FAIL: TestCompiler (1.64s)
    --- FAIL: TestCompiler/basic.go (0.12s)
        compiler_test.go:254: could not parse test case basic.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
    --- FAIL: TestCompiler/pointer.go (0.08s)
        compiler_test.go:254: could not parse test case pointer.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
    --- FAIL: TestCompiler/slice.go (0.08s)
        compiler_test.go:254: could not parse test case slice.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
    --- FAIL: TestCompiler/string.go (0.08s)
        compiler_test.go:254: could not parse test case string.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
    --- FAIL: TestCompiler/float.go (0.13s)
        compiler_test.go:254: could not parse test case float.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
    --- FAIL: TestCompiler/interface.go (0.16s)
        compiler_test.go:254: could not parse test case interface.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
    --- FAIL: TestCompiler/func.go (0.14s)
        compiler_test.go:254: could not parse test case func.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
    --- FAIL: TestCompiler/pragma.go (0.07s)
        compiler_test.go:254: could not parse test case pragma.go: could not compile: /Users/a/proj/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)
...

[aykevl]

The CI failure

compiler_test.go:254: could not parse test case basic.go: could not compile: /Users/runner/work/tinygo/tinygo/src/runtime/scheduler.go:203:17: invalid operation: operator ^ not defined on timeUnit(0) (constant 0 of type timeUnit)

is surprising.

For which target does this fail? If it's for wasm, then that makes sense: we use float64 for timeUnit (because JavaScript uses float64 too and I wanted to avoid conversion errors).

I need to look at the logic of this PR another time.

[dgryski]

@aykevl Ah, float for the js runtime. Good catch! I'll see if that's the case.

[dgryski]

Yup, almost certainly. I don't think we can reasonably do anything to fix the compilation error, since we're using wasm for those unit tests.

[aykevl]

The real issue seems to be that rp2040 is the only system where timeUnit is unsigned. That looks like a bug.
Here is a fix: #4242. @eliasnaur can you test that PR?

With that fix, every target (except for wasm) will use int64. I think it makes sense to convert wasm to int64 too to get rid of inconsistencies like these and just define timeUnit as int64 everywhere.

[eliasnaur]

Thanks @aykevl, I completely missed both the inconsistent timerUnit and the WASM float variant.

I've updated the PR with the only remaining issue: sleepTicks should return early for negative durations, otherwise the cast to an unsigned number of ticks for machineLightSleep underflows.

[aykevl]

Yes that change looks entirely reasonable! Thank you for the fix!