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: https://github.com/tinygo-org/tinygo/pull/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!