reflect: tagged pointers

[dgryski]

Doesn't work yet. Still segfaults the compiler.

EDIT by @deadprogram it does work now 😸

[aykevl]

Note to self: I need to look into why OptimizeReflectImplements fails.

[aykevl]

Found so far:

• OptimizeReflectImplements breaks because some globals aren't constants (while they are before this change).
• They are non-constant because interp can't deal with the ptrtag method.

[aykevl]

I think I have a fix. Now I need to properly test it and clean up the code.

[aykevl]

This should fix most/all errors: #3737

I tested it by applying the 3 patches on top of this branch, and go test -short -v now passes while it did not pass before.

[aykevl]

@dgryski can you rebased this PR? My fix has been merged.

[dgryski]

Rebased.

[dgryski]

So my hacked up encoding/xml and encoding/json are still failing. I want to investigate why before merging this.

[dgryski]

So encoding/xml needs -stack-size=32KB for -target=wasi and passes on native.

encoding/asn1 fails with:

~/go/src/go.googlesource.com/go/src/encoding/xml $ tinygo test encoding/asn1
--- FAIL: TestUnmarshal (0.00s)
    Unmarshal failed at index 17 asn1: structure error: unsupported: *big.Int
    #17:
        have &asn1.TestBigInt{X:<nil>}
        want &asn1.TestBigInt{X:1193046}

encoding/json fails with:

(lldb) r -test.v
There is a running process, kill it and restart?: [Y/n] y
Process 29110 exited with status = 9 (0x00000009) killed
Process 29115 launched: '/Users/dgryski/go/src/go.googlesource.com/go/src/encoding/json/json.test' (x86_64)
=== RUN   TestMarshal
--- PASS: TestMarshal (0.00s)
=== RUN   TestMarshalBadUTF8
--- PASS: TestMarshalBadUTF8 (0.00s)
=== RUN   TestMarshalNumberZeroVal
--- PASS: TestMarshalNumberZeroVal (0.00s)
=== RUN   TestMarshalEmbeds
--- PASS: TestMarshalEmbeds (0.00s)
=== RUN   TestUnmarshal
--- PASS: TestUnmarshal (0.02s)
=== RUN   TestUnmarshalMarshal
Process 29115 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x1)
    frame #0: 0x0000000000000001
error: memory read failed for 0x0
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x1)
  * frame #0: 0x0000000000000001
    frame #1: 0x00000001000ad980 json.test`reflect/types.type:pointer:basic:float64 + 16

[aykevl]

@dgryski what's the status of this? Did these work before in #3570?

[dgryski]

Yes, I'm pretty sure they did. Checking the tinygo-dev logs shows successes for these packages.

[dgryski]

Ok, more playing around here. The hacked up encoding/json tests now pass if I reduce the size of randomly generated structures -- pretty sure I did this before too, but somehow it got lost in git stash. So it's just encoding/asn1 I need to figure out.

[dgryski]

The problem with encoding/asn1 is with type switches:

~/go/src/github.com/dgryski/bug/asn $ cat main.go
package main

import (
	"math/big"
	"reflect"
)

type B struct {
	X *big.Int
}

func main() {
	var b B
	v := reflect.ValueOf(&b)

	f := v.Elem().Field(0)

	switch x := f.Addr().Interface().(type) {
	case **big.Int:
		println("**big.Int")
	default:
		println("default", reflect.TypeOf(x).String())
	}
}
~/go/src/github.com/dgryski/bug/asn $ tinygo run main.go
default **big.Int
~/go/src/github.com/dgryski/bug/asn $ go run main.go
**big.Int

We can see that while the type of x is **big.Int, it matches the default case of the type switch instead of the **big.Int one.

[aykevl]

I think this might be an issue with the pass that lowers type asserts. With ptrTo gone, it doesn't know the pointer type can still be created.

[dgryski]

I was looking at https://github.com/tinygo-org/tinygo/blob/dev/compiler/interface.go#L661 and wondering if the issue is more related to the fact that we don't handle those pointer types because there aren't named globals for them (but I'm not sure what the fix needs to be...)

[aykevl]

I was looking at https://github.com/tinygo-org/tinygo/blob/dev/compiler/interface.go#L661 and wondering if the issue is more related to the fact that we don't handle those pointer types because there aren't named globals for them (but I'm not sure what the fix needs to be...)

Yes, exactly, that's more or less what I meant. There are no named globals because the ptrTo field is missing for these pointers.

One possible fix could be to change the code that lowers type asserts and make sure it also checks whether the element type exists. See the TODO in this code:

tinygo/transform/interface-lowering.go

Lines 288 to 300 in f366cd5

	if t, ok := p.types[name]; ok {
	// The type exists in the program, so lower to a regular pointer
	// comparison.
	p.builder.SetInsertPointBefore(use)
	commaOk := p.builder.CreateICmp(llvm.IntEQ, t.typecodeGEP, actualType, "typeassert.ok")
	use.ReplaceAllUsesWith(commaOk)
	} else {
	// The type does not exist in the program, so lower to a constant
	// false. This is trivially further optimized.
	// TODO: eventually it'll be necessary to handle reflect.PtrTo and
	// reflect.New calls which create new types not present in the
	// original program.
	use.ReplaceAllUsesWith(llvmFalse)

[aykevl]

Here is a very simple reproducer of the type switch issue:

package main

func main() {
        identify(0)
        identify(new(int))
        identify(new(*int))
}

func identify(itf any) {
        switch itf.(type) {
        case int:
                println("type is int")
        case *int:
                println("type is *int")
        case **int:
                println("type is **int")
        default:
                println("other type??")
        }
}

Go:

$ go run ./tmp/ptrptrtest.go 
type is int
type is *int
type is **int

TinyGo on the dev branch:

$ tinygo run ./tmp/ptrptrtest.go
type is int
type is *int
type is **int

This branch:

tinygo run ./tmp/ptrptrtest.go
type is int
type is *int
other type??

So the problem has nothing to do with reflect, just with type asserts (type switches are implemented as an if-else chain made of type asserts).

[aykevl]

Here is a fix:

diff --git a/compiler/interface.go b/compiler/interface.go
index bd6970b4..d9c0e138 100644
--- a/compiler/interface.go
+++ b/compiler/interface.go
@@ -673,11 +673,8 @@ func (b *builder) createTypeAssert(expr *ssa.TypeAssert) llvm.Value {
                commaOk = b.CreateCall(fn.GlobalValueType(), fn, []llvm.Value{actualTypeNum}, "")
 
        } else {
-               assertedTypeGlobal := b.getTypeCode(expr.AssertedType)
-               if !assertedTypeGlobal.IsAConstantExpr().IsNil() {
-                       assertedTypeGlobal = assertedTypeGlobal.Operand(0) // resolve the GEP operation
-               }
-               globalName := "reflect/types.typeid:" + strings.TrimPrefix(assertedTypeGlobal.Name(), "reflect/types.type:")
+               name, _ := getTypeCodeName(expr.AssertedType)
+               globalName := "reflect/types.typeid:" + name
                assertedTypeCodeGlobal := b.mod.NamedGlobal(globalName)
                if assertedTypeCodeGlobal.IsNil() {
                        // Create a new typecode global.
diff --git a/transform/interface-lowering.go b/transform/interface-lowering.go
index ebd47ff8..1cbebee8 100644
--- a/transform/interface-lowering.go
+++ b/transform/interface-lowering.go
@@ -285,11 +285,26 @@ func (p *lowerInterfacesPass) run() error {
        for _, use := range getUses(p.mod.NamedFunction("runtime.typeAssert")) {
                actualType := use.Operand(0)
                name := strings.TrimPrefix(use.Operand(1).Name(), "reflect/types.typeid:")
+               gepOffset := uint64(0)
+               for strings.HasPrefix(name, "pointer:pointer:") {
+                       // This is a type like **int, which has the name pointer:pointer:int
+                       // but is encoded using pointer tagging.
+                       // Calculate the pointer tag, which is emitted as a GEP instruction.
+                       name = name[len("pointer:"):]
+                       gepOffset++
+               }
                if t, ok := p.types[name]; ok {
                        // The type exists in the program, so lower to a regular pointer
                        // comparison.
                        p.builder.SetInsertPointBefore(use)
-                       commaOk := p.builder.CreateICmp(llvm.IntEQ, t.typecodeGEP, actualType, "typeassert.ok")
+                       typecodeGEP := t.typecodeGEP
+                       if gepOffset != 0 {
+                               // This is a tagged pointer.
+                               typecodeGEP = llvm.ConstInBoundsGEP(p.ctx.Int8Type(), typecodeGEP, []llvm.Value{
+                                       llvm.ConstInt(p.ctx.Int64Type(), gepOffset, false),
+                               })
+                       }
+                       commaOk := p.builder.CreateICmp(llvm.IntEQ, typecodeGEP, actualType, "typeassert.ok")
                        use.ReplaceAllUsesWith(commaOk)
                } else {
                        // The type does not exist in the program, so lower to a constant

[aykevl]

Was that last commit intended to be part of this PR?

[dgryski]

Oops, probably not. I'll put that in a separate one. It was included in the original **T branch (since we need it) but yeah it should be its own PR.

[deadprogram]

@aykevl any other feedback on this PR? The previous change that you suggested has been added.

[aykevl]

Did you run the test corpus on this PR? If not, that's something that I would recommend (because this is a rather invasive change).

Other than that, and the (hopefully easy to fix) issue mentioned below, this looks good to me.

[aykevl]

I think this should have a check for *****T types, which will overflow the pointer tagging scheme. Better have a compiler error than a mysterious runtime error.

You could for example use .Operand(...) on the GEP expression to check the offset and make sure it's at most 3.

[deadprogram]

What do you think @dgryski ?

[dgryski]

I'd love if I could get a position to report the error from instead of just token.NoPos. Any suggestion?

[aykevl]

I'd love if I could get a position to report the error from instead of just token.NoPos. Any suggestion?

Hmm, that's difficult. If it's a pointer to a named type, you could use the location of the named type - but that's still not very useful.

I think what is needed is to update all callers to provide a position because the position is not stored in the type.Type struct as far as I know. If that's a bit too much for this PR, it can maybe also done in a separate PR?

[deadprogram]

I think we should merge this PR and then can deal with the additional debugging info as needed in a future PR, if so required. Hence merging at last!

@dgryski and @aykevl great work on getting this last PR completed on the release!