From a7794de99d054524ca4084a20a336f56392514a7 Mon Sep 17 00:00:00 2001 From: Ayke van Laethem Date: Tue, 24 Sep 2019 13:48:42 +0200 Subject: [PATCH] compiler: fix interface lowering miscompilation with reflect When using reflect, arbitrary types can be synthesized. This invalidates a few assumptions in the interface-lowering pass, that think they can see all types that are in use in a program and optimize accordingly. The file size impact depends on the specific program. Sometimes it's nonexistent, sometimes it's rather hefty (up to 30% bigger). Especially the samd21 targets seem to be affected, with a 2000-6000 bytes increase in code size. A moderately large case (the stdlib test) increases by 4%/6%/15% depending on the target. I hope that this increase could be mitigated, but I don't see an obvious way to do that. --- compiler/interface-lowering.go | 52 ++++++---------------------------- testdata/reflect.go | 12 ++++++++ testdata/reflect.txt | 1 + 3 files changed, 22 insertions(+), 43 deletions(-) diff --git a/compiler/interface-lowering.go b/compiler/interface-lowering.go index b8e57278..1aa9862f 100644 --- a/compiler/interface-lowering.go +++ b/compiler/interface-lowering.go @@ -17,17 +17,10 @@ package compiler // Replaced with an icmp instruction so it can be directly used in a type // switch. This is very easy to optimize for LLVM: it will often translate a // type switch into a regular switch statement. -// When this type assert is not possible (the type is never used in an -// interface), this call is replaced with a constant false to optimize the -// type assert away completely. // // interfaceImplements: // This call is translated into a call that checks whether the underlying // type is one of the types implementing this interface. -// When there is only one type implementing this interface, the check is -// replaced with a simple icmp instruction, just like a type assert. -// When there is no type at all that implements this interface, it is -// replaced with a constant false to optimize it completely. // // interfaceMethod: // This call is replaced with a call to a function that calls the @@ -353,32 +346,13 @@ func (p *lowerInterfacesPass) run() { methodSet := use.Operand(1).Operand(0) // global variable itf := p.interfaces[methodSet.Name()] - if len(itf.types) == 0 { - // There are no types implementing this interface, so this assert - // can never succeed. - // Signal this to the optimizer by branching on constant false. It - // should remove the "then" block. - use.ReplaceAllUsesWith(llvm.ConstInt(p.ctx.Int1Type(), 0, false)) - use.EraseFromParentAsInstruction() - } else if len(itf.types) == 1 { - // There is only one type implementing this interface. - // Transform this interface assert into comparison against a - // constant. - p.builder.SetInsertPointBefore(use) - assertedType := p.builder.CreatePtrToInt(itf.types[0].typecode, p.uintptrType, "typeassert.typecode") - commaOk := p.builder.CreateICmp(llvm.IntEQ, assertedType, actualType, "typeassert.ok") - use.ReplaceAllUsesWith(commaOk) - use.EraseFromParentAsInstruction() - } else { - // There are multiple possible types implementing this interface. - // Create a function that does a type switch on all available types - // that implement this interface. - fn := p.getInterfaceImplementsFunc(itf) - p.builder.SetInsertPointBefore(use) - commaOk := p.builder.CreateCall(fn, []llvm.Value{actualType}, "typeassert.ok") - use.ReplaceAllUsesWith(commaOk) - use.EraseFromParentAsInstruction() - } + // Create a function that does a type switch on all available types + // that implement this interface. + fn := p.getInterfaceImplementsFunc(itf) + p.builder.SetInsertPointBefore(use) + commaOk := p.builder.CreateCall(fn, []llvm.Value{actualType}, "typeassert.ok") + use.ReplaceAllUsesWith(commaOk) + use.EraseFromParentAsInstruction() } // Make a slice of types sorted by frequency of use. @@ -411,16 +385,8 @@ func (p *lowerInterfacesPass) run() { for _, use := range typeAssertUses { actualType := use.Operand(0) assertedTypeGlobal := use.Operand(1) - t := p.types[assertedTypeGlobal.Name()] - var commaOk llvm.Value - if t.countMakeInterfaces == 0 { - // impossible type assert: optimize accordingly - commaOk = llvm.ConstInt(p.ctx.Int1Type(), 0, false) - } else { - // regular type assert - p.builder.SetInsertPointBefore(use) - commaOk = p.builder.CreateICmp(llvm.IntEQ, llvm.ConstPtrToInt(assertedTypeGlobal, p.uintptrType), actualType, "typeassert.ok") - } + p.builder.SetInsertPointBefore(use) + commaOk := p.builder.CreateICmp(llvm.IntEQ, llvm.ConstPtrToInt(assertedTypeGlobal, p.uintptrType), actualType, "typeassert.ok") use.ReplaceAllUsesWith(commaOk) use.EraseFromParentAsInstruction() } diff --git a/testdata/reflect.go b/testdata/reflect.go index 4fe8cbb3..40b1969c 100644 --- a/testdata/reflect.go +++ b/testdata/reflect.go @@ -254,6 +254,16 @@ func main() { if rv.Len() != 2 || rv.Index(0).Int() != 3 { panic("slice was changed while setting part of it") } + + // Test types that are created in reflect and never created elsewhere in a + // value-to-interface conversion. + v := reflect.ValueOf(new(unreferencedType)) + switch v.Elem().Interface().(type) { + case unreferencedType: + println("type assertion succeeded for unreferenced type") + default: + println("type assertion failed (but should succeed)") + } } func emptyFunc() { @@ -340,3 +350,5 @@ func assertSize(ok bool, typ string) { panic("size mismatch for type " + typ) } } + +type unreferencedType int diff --git a/testdata/reflect.txt b/testdata/reflect.txt index 8bb87877..28361306 100644 --- a/testdata/reflect.txt +++ b/testdata/reflect.txt @@ -335,3 +335,4 @@ float32 4 32 float64 8 64 complex64 8 64 complex128 16 128 +type assertion succeeded for unreferenced type