diff --git a/builder/sizes_test.go b/builder/sizes_test.go index fd985a897..5717c769c 100644 --- a/builder/sizes_test.go +++ b/builder/sizes_test.go @@ -42,9 +42,9 @@ func TestBinarySize(t *testing.T) { // This is a small number of very diverse targets that we want to test. tests := []sizeTest{ // microcontrollers - {"hifive1b", "examples/echo", 3680, 280, 0, 2252}, - {"microbit", "examples/serial", 2694, 342, 8, 2248}, - {"wioterminal", "examples/pininterrupt", 7074, 1510, 120, 7248}, + {"hifive1b", "examples/echo", 3817, 299, 0, 2252}, + {"microbit", "examples/serial", 2816, 356, 8, 2248}, + {"wioterminal", "examples/pininterrupt", 7206, 1510, 120, 7248}, // TODO: also check wasm. Right now this is difficult, because // wasm binaries are run through wasm-opt and therefore the @@ -99,7 +99,7 @@ func TestSizeFull(t *testing.T) { t.Fatal("could not read program size:", err) } for _, pkg := range sizes.sortedPackageNames() { - if pkg == "(padding)" || pkg == "(unknown)" { + if pkg == "(padding)" || pkg == "(unknown)" || pkg == "Go types" { // TODO: correctly attribute all unknown binary size. continue } diff --git a/compiler/asserts.go b/compiler/asserts.go index c890a3317..8296cca52 100644 --- a/compiler/asserts.go +++ b/compiler/asserts.go @@ -252,6 +252,9 @@ func (b *builder) createRuntimeAssert(assert llvm.Value, blockPrefix, assertFunc // Fail: the assert triggered so panic. b.SetInsertPointAtEnd(faultBlock) + if b.hasDeferFrame() { + b.createFaultCheckpoint() + } b.createRuntimeCall(assertFunc, nil, "") b.CreateUnreachable() diff --git a/compiler/compiler.go b/compiler/compiler.go index eaf7b58b1..ebb7bf223 100644 --- a/compiler/compiler.go +++ b/compiler/compiler.go @@ -1895,6 +1895,12 @@ func (b *builder) createBuiltin(argTypes []types.Type, argValues []llvm.Value, c // not of the current function. useParentFrame = 1 } + // Prevent inlining of functions that call recover(), matching the + // Go compiler's behavior. If this function were inlined into a + // deferred function, recover() would incorrectly succeed because + // the inlined code runs in the deferred function's context. + noinline := b.ctx.CreateEnumAttribute(llvm.AttributeKindID("noinline"), 0) + b.llvmFn.AddFunctionAttr(noinline) return b.createRuntimeCall("_recover", []llvm.Value{llvm.ConstInt(b.ctx.Int1Type(), useParentFrame, false)}, ""), nil case "ssa:wrapnilchk": // TODO: do an actual nil check? diff --git a/compiler/defer.go b/compiler/defer.go index 1ca953966..8863bfd87 100644 --- a/compiler/defer.go +++ b/compiler/defer.go @@ -241,6 +241,17 @@ func (b *builder) createInvokeCheckpoint() { b.currentBlockInfo.exit = continueBB } +// createFaultCheckpoint is like createInvokeCheckpoint but for use in fault +// blocks (e.g., bounds check failures). Unlike createInvokeCheckpoint, it does +// not update currentBlockInfo.exit because the fault block is a dead-end that +// does not participate in phi node resolution. +func (b *builder) createFaultCheckpoint() { + isZero := b.createCheckpoint(b.deferFrame) + continueBB := b.insertBasicBlock("") + b.CreateCondBr(isZero, continueBB, b.landingpad) + b.SetInsertPointAtEnd(continueBB) +} + // isInLoop checks if there is a path from the current block to itself. // Use Tarjan's strongly connected components algorithm to search for cycles. // A one-node SCC is a cycle iff there is an edge from the node to itself. diff --git a/compiler/interface.go b/compiler/interface.go index 5f7e7e345..a589b0d82 100644 --- a/compiler/interface.go +++ b/compiler/interface.go @@ -796,40 +796,56 @@ func (b *builder) createTypeAssert(expr *ssa.TypeAssert) llvm.Value { prevBlock := b.GetInsertBlock() okBlock := b.insertBasicBlock("typeassert.ok") - nextBlock := b.insertBasicBlock("typeassert.next") - b.currentBlockInfo.exit = nextBlock // adjust outgoing block for phi nodes - b.CreateCondBr(commaOk, okBlock, nextBlock) - - // Retrieve the value from the interface if the type assert was - // successful. - b.SetInsertPointAtEnd(okBlock) - var valueOk llvm.Value - if _, ok := expr.AssertedType.Underlying().(*types.Interface); ok { - // Type assert on interface type. Easy: just return the same - // interface value. - valueOk = itf - } else { - // Type assert on concrete type. Extract the underlying type from - // the interface (but only after checking it matches). - valueOk = b.extractValueFromInterface(itf, assertedType) - } - b.CreateBr(nextBlock) - - // Continue after the if statement. - b.SetInsertPointAtEnd(nextBlock) - phi := b.CreatePHI(assertedType, "typeassert.value") - phi.AddIncoming([]llvm.Value{llvm.ConstNull(assertedType), valueOk}, []llvm.BasicBlock{prevBlock, okBlock}) if expr.CommaOk { + nextBlock := b.insertBasicBlock("typeassert.next") + b.currentBlockInfo.exit = nextBlock + b.CreateCondBr(commaOk, okBlock, nextBlock) + + // Retrieve the value from the interface if the type assert was + // successful. + b.SetInsertPointAtEnd(okBlock) + var valueOk llvm.Value + if _, ok := expr.AssertedType.Underlying().(*types.Interface); ok { + // Type assert on interface type. Easy: just return the same + // interface value. + valueOk = itf + } else { + // Type assert on concrete type. Extract the underlying type from + // the interface (but only after checking it matches). + valueOk = b.extractValueFromInterface(itf, assertedType) + } + b.CreateBr(nextBlock) + + // Continue after the if statement. + b.SetInsertPointAtEnd(nextBlock) + phi := b.CreatePHI(assertedType, "typeassert.value") + phi.AddIncoming([]llvm.Value{llvm.ConstNull(assertedType), valueOk}, []llvm.BasicBlock{prevBlock, okBlock}) + tuple := b.ctx.ConstStruct([]llvm.Value{llvm.Undef(assertedType), llvm.Undef(b.ctx.Int1Type())}, false) // create empty tuple tuple = b.CreateInsertValue(tuple, phi, 0, "") // insert value tuple = b.CreateInsertValue(tuple, commaOk, 1, "") // insert 'comma ok' boolean return tuple } else { - // This is kind of dirty as the branch above becomes mostly useless, - // but hopefully this gets optimized away. - b.createRuntimeCall("interfaceTypeAssert", []llvm.Value{commaOk}, "") - return phi + // Type assert without comma-ok. If it fails, panic. + faultBlock := b.ctx.AddBasicBlock(b.llvmFn, "typeassert.throw") + b.currentBlockInfo.exit = okBlock + b.CreateCondBr(commaOk, okBlock, faultBlock) + + // Fault: emit a checkpoint (for recover) and panic. + b.SetInsertPointAtEnd(faultBlock) + if b.hasDeferFrame() { + b.createFaultCheckpoint() + } + b.createRuntimeCall("interfaceTypeAssert", []llvm.Value{llvm.ConstInt(b.ctx.Int1Type(), 0, false)}, "") + b.CreateUnreachable() + + // OK: extract the value from the interface. + b.SetInsertPointAtEnd(okBlock) + if _, ok := expr.AssertedType.Underlying().(*types.Interface); ok { + return itf + } + return b.extractValueFromInterface(itf, assertedType) } } diff --git a/src/runtime/error.go b/src/runtime/error.go index 3ae5ea3aa..cd5429d1d 100644 --- a/src/runtime/error.go +++ b/src/runtime/error.go @@ -6,3 +6,9 @@ type Error interface { RuntimeError() } + +// plainError is a runtime.Error implementation for plain string messages. +type plainError string + +func (e plainError) Error() string { return string(e) } +func (e plainError) RuntimeError() {} diff --git a/src/runtime/panic.go b/src/runtime/panic.go index 86a6c3aa7..cdd323267 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -93,6 +93,17 @@ func runtimePanicAt(addr unsafe.Pointer, msg string) { if panicStrategy() == tinygo.PanicStrategyTrap { trap() } + if supportsRecover() && !interrupt.In() { + frame := (*deferFrame)(task.Current().DeferFrame) + if frame != nil { + // Use the normal panic mechanism so that this runtime error + // can be recovered with recover(). + frame.PanicValue = plainError(msg) + frame.Panicking = panicTrue + tinygo_longjmp(frame) + // unreachable + } + } if hasReturnAddr { // Note: the string "panic: runtime error at " is also used in // runtime_cortexm_hardfault.go. It is kept the same so that the string @@ -149,6 +160,16 @@ func destroyDeferFrame(frame *deferFrame) { // panicking goroutine. // useParentFrame is set when the caller of runtime._recover has a defer frame // itself. In that case, recover() shouldn't check that frame but one frame up. +// +// TODO: Go only allows recover() to succeed when called directly from a +// deferred function, not from a sub-call (e.g. defer func() { sub() }() where +// sub() calls recover()). The Go compiler enforces this by walking the stack +// to count frames between gorecover and gopanic. TinyGo currently does not +// have a stack unwinder, so this restriction is not enforced at runtime. +// Functions calling recover() are marked noinline to prevent the most common +// case (inlined sub-call), but non-inlined sub-calls can still incorrectly +// recover. Fixing this properly requires either frame pointer support or a +// lightweight stack unwinder. func _recover(useParentFrame bool) interface{} { if !supportsRecover() || interrupt.In() { // Either we're compiling without stack unwinding support, or we're @@ -157,9 +178,6 @@ func _recover(useParentFrame bool) interface{} { // function. return nil } - // TODO: somehow check that recover() is called directly by a deferred - // function in a panicking goroutine. Maybe this can be done by comparing - // the frame pointer? frame := (*deferFrame)(task.Current().DeferFrame) if useParentFrame { // Don't recover panic from the current frame (which can't be panicking diff --git a/testdata/recover.go b/testdata/recover.go index 6fdf282e7..37b59607f 100644 --- a/testdata/recover.go +++ b/testdata/recover.go @@ -30,8 +30,17 @@ func main() { println("\n# defer panic") deferPanic() + println("\n# indirect recover") + indirectRecover() + println("\n# runtime.Goexit") runtimeGoexit() + + println("\n# repanic") + recoverRepanic() + + println("\n# recover runtime errors") + recoverRuntimeError() } func recoverSimple() { @@ -114,6 +123,24 @@ func deferPanic() { println("defer panic") } +// TODO: Go only allows recover() to succeed when called directly from a +// deferred function. Update this test once runtime recover can distinguish it. +func indirectRecover() { + defer func() { + if r := indirectRecoverHelper(); r == nil { + println("indirect recover returned nil") + } else { + printitf("indirect recover returned:", r) + } + }() + panic("indirect panic") +} + +//go:noinline +func indirectRecoverHelper() interface{} { + return recover() +} + func runtimeGoexit() { wg.Add(1) go func() { @@ -127,6 +154,27 @@ func runtimeGoexit() { wg.Wait() } +// Test that a repanic inside a deferred function propagates correctly +// instead of re-running the same defer. This is a regression test for +// tinygo-org/tinygo issue 3449. +func recoverRepanic() { + // Two defers: inner recovers and repanics, outer should catch it. + defer func() { + r := recover() + if r != nil { + printitf("outer recovered:", r) + } + }() + defer func() { + r := recover() + if r != nil { + println("inner, repanicking") + panic(r) + } + }() + panic("repanic value") +} + func printitf(msg string, itf interface{}) { switch itf := itf.(type) { case string: @@ -135,3 +183,61 @@ func printitf(msg string, itf interface{}) { println(msg, itf) } } + +// Test recovering from runtime errors (bounds checks, type assertions, etc.) +func recoverRuntimeError() { + recoverMustPanic("index", func() { + s := make([]int, 5) + _ = s[99] + }) + recoverMustPanic("index from helper", func() { + s := []byte{1} + _ = readOutOfBounds(s) + }) + recoverMustPanic("slice", func() { + s := make([]int, 5) + _ = s[3:99] + }) + recoverMustPanic("type assert", func() { + var x interface{} = 1 + _ = x.(string) + }) + recoverEmptyInterfaceTypeAssert() +} + +//go:noinline +func readOutOfBounds(s []byte) byte { + return s[2] +} + +func recoverEmptyInterfaceTypeAssert() { + defer func() { + r := recover() + if r != nil { + println(" failed empty interface type assert") + } else { + println(" recovered: empty interface type assert") + } + }() + var intf interface{} = 3 + typed := intf.(interface{}) + useEmptyInterface(typed) +} + +func useEmptyInterface(typed interface{}) { + if typed.(int) != 3 { + println(" failed empty interface value") + } +} + +func recoverMustPanic(name string, f func()) { + defer func() { + r := recover() + if r != nil { + println(" recovered:", name) + } else { + println(" failed to recover:", name) + } + }() + f() +} diff --git a/testdata/recover.txt b/testdata/recover.txt index 87e4ba5d1..2b6b47af9 100644 --- a/testdata/recover.txt +++ b/testdata/recover.txt @@ -28,5 +28,19 @@ recovered: panic 2 defer panic recovered from deferred call: deferred panic +# indirect recover +indirect recover returned: indirect panic + # runtime.Goexit Goexit deferred function, recover is nil: true + +# repanic +inner, repanicking +outer recovered: repanic value + +# recover runtime errors + recovered: index + recovered: index from helper + recovered: slice + recovered: type assert + recovered: empty interface type assert