transform (LowerInterfaces): link-time type renaming results in type errors

[niaow]

Originally, the TinyGo frontend worked by compiling and dumping all used functions and types into a single LLVM module.
This lead to a nice property in the IR: every Go type corresponded to exactly one LLVM type.

However, in #1612 we switched to building each package independently and then linking them together.

Now that we link LLVM modules together, it is possible to have a Go type map to multiple LLVM types - one from each package that uses the type. Most of the time, the module linker can merge the types. However, it seems unable to merge self-referencing types (or at least I think that is the unmerged scenario?). When the unmerged types meet, the linker adds bitcasts to convert between them.

As a result of this renaming process, different implementations of an interface may use different-but-equivalent LLVM types. The interface lowering pass does not handle this, so it can create type errors.

[aykevl]

Does this fix it? #1898

[dgryski]

Yes. Merging #1898 into the asyncify branch fixes the problem.

[dgryski]

Interesting. While it does fix the small test case, it didn't appear to fix effectively the same issue in a larger codebase:

Both operands to ICmp instruction are not of the same type!
  %7 = icmp eq %runtime.channel.531* %6, %runtime.channel.56* null, !dbg !15728
Stored value type does not match pointer operand type!
  store %runtime.channel.531* %6, %runtime.channel.56** %.fca.0.gep, align 4, !dbg !15721
 %runtime.channel.56*Both operands to ICmp instruction are not of the same type!
  %1 = icmp eq %runtime.channel.531* %0, %runtime.channel.56* bitcast ({ i32, i32, i32, i8*, i32, i32, i32, i8* }* @"context$alloc.538" to %runtime.channel.56*), !dbg !15877
Both operands to ICmp instruction are not of the same type!
  %2 = icmp eq %runtime.channel.531* %0, %runtime.channel.56* null
Both operands to ICmp instruction are not of the same type!
  %9 = icmp ne %runtime.channel.56* %typeassert.value15, %runtime.channel.531* %0, !dbg !15894
Stored value type does not match pointer operand type!
  store %runtime.channel.531* %5, %runtime.channel.56** %.fca.0.gep, align 4, !dbg !15899
 %runtime.channel.56*Function return type does not match operand type of return inst!
  ret %runtime.channel.531* %3
 %runtime.channel.56*Function return type does not match operand type of return inst!
  ret %runtime.channel.531* %2
 %runtime.channel.56*Function return type does not match operand type of return inst!
  ret %runtime.channel.531* %3
 %runtime.channel.56*Function return type does not match operand type of return inst!
  ret %runtime.channel.531* %3
 %runtime.channel.56*Function return type does not match operand type of return inst!
  ret %runtime.channel.531* %2
 %runtime.channel.56*Stored value type does not match pointer operand type!
  store %runtime.channel.531* %9, %runtime.channel.199** %.fca.0.gep10, align 4, !dbg !36774

(The compiler building the above actually is more than just asyncify + fix-issue1887. It also has precise-alloc, recursive-func-types, plus my small stdlib fixes.)

[niaow]

@dgryski If you cannot share the code, can you try to identify which pass is introducing the type error?
Here is a snippet which you can place between passes to check for type errors:

if err := llvm.VerifyModule(mod, llvm.PrintMessageAction); err != nil {
	println()
	mod.Dump()
	println()
	panic(err)
}

The passes are all run in transform/optimizer.go.

[dgryski]

Yes, I'll try to isolate this bug today.

[aykevl]

You might also want to try #2202. There is a chance it fixes this bug as a side effect.

[dgryski]

@aykevl Merging the go-on-interfaces branch into my already overloaded working branch seemed to work. I didn't fix any of the conflicts in the .ll files, and only needed this patch to get the tree to build:

diff --git a/transform/interface-lowering.go b/transform/interface-lowering.go
index e1dfd510..7c11c609 100644
--- a/transform/interface-lowering.go
+++ b/transform/interface-lowering.go
@@ -438,6 +438,7 @@ func (p *lowerInterfacesPass) defineInterfaceMethodFunc(fn llvm.Value, itf *inte
                        paramTypes = append(paramTypes, param.Type())
                }
                calledFunctionType := function.Type()
+               returnType := calledFunctionType.ElementType().ReturnType()
                sig := llvm.PointerType(llvm.FunctionType(returnType, paramTypes, false), calledFunctionType.PointerAddressSpace())
                if sig != function.Type() {
                        function = p.builder.CreateBitCast(function, sig, "")

Allows our codebase to build and pass tests! Thanks for all your great work on this.

(Now to get all these in-flight PRs merged...)

[niaow]

Okay, I am going to close this as a duplicate of #1887