reflect: the docs of StructOf should mention embedding types with too many method will panic.

[dotaheor]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.2 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

package main

import "fmt"
import "reflect"
import "time"

func main() {
	type T = struct {
		time.Time
	}
	fmt.Println(reflect.TypeOf(T{})) // struct { x int }
	
	var n time.Time
	tn := reflect.TypeOf(n)
	// panic: reflect.StructOf: too many methods
	tt := reflect.StructOf([]reflect.StructField{
		{Name: "Time", Type: tn, Anonymous: true},
	})
	fmt.Println(tt)
}

What did you expect to see?

not panic, or document it.

What did you see instead?

panic

[dotaheor]

btw, gccgo also panics for this example, but with a different reason

panic: reflect.StructOf: embedded field with methods not implemented

[dotaheor]

Why do gccgo and gc output different results? Isn't this a standard package specified problem?

[ianlancetaylor]

I haven't looked at this case, but the reflect package is closely tied to the compiler, since the compiler generates the data structures that it uses, and there are significant differences in the implementation of the reflect package for gc and gccgo.

[jamdagni86]

@ianlancetaylor - the number of Exported methods in time.Time is 38 and it looks from the code

go/src/reflect/type.go

Lines 2589 to 2595 in 1174ad3

	case len(methods) <= 32:
	t := new(structTypeFixed32)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	default:
	panic("reflect.StructOf: too many methods")

that only 32 methods are supported; that's the reason for the panic.

Let me know if this issue needs to be fixed; happy to help!.

[ianlancetaylor]

Yes, this should be fixed, possibly in the documentation, but it would be better to fix the code.

[jamdagni86]

The code inside the individual case statements seems outdated (because of subsequent code changes). This is the original implementation and this the next change to the code. Because of the latter, the variable t declared inside each of the case statements has become redundant. Statements at 2590 & 2593 have no meaning anymore and

I was wrong; the

copy(t.m[:], methods)

copies the methods' details on to the memory after structTypeFixedXX.u. This memory will be later referenced to get the methods from the struct object using uncommonType.moff.

go/src/reflect/type.go

Lines 642 to 647 in 1174ad3

	func (t *uncommonType) exportedMethods() []method {
	if t.xcount == 0 {
	return nil
	}
	return (*[1 << 16]method)(add(unsafe.Pointer(t), uintptr(t.moff), "t.xcount > 0"))[:t.xcount:t.xcount]
	}

The code

go/src/reflect/type.go

Lines 2566 to 2596 in 1174ad3

	var typ *structType
	var ut *uncommonType
	switch {
	case len(methods) == 0:
	t := new(structTypeUncommon)
	typ = &t.structType
	ut = &t.u
	case len(methods) <= 4:
	t := new(structTypeFixed4)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	case len(methods) <= 8:
	t := new(structTypeFixed8)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	case len(methods) <= 16:
	t := new(structTypeFixed16)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	case len(methods) <= 32:
	t := new(structTypeFixed32)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	default:
	panic("reflect.StructOf: too many methods")
	}

should be replaced with a dynamic version of structTypeFixedXX struct in order to support arbitrary number of methods.

can potentially be replaced by

typ := &structType{}
ut := &uncommonType{}

I made a quick test by doing the change, and it seemed to work, haven't tested it completely though.

If the changes are OK, I will do further testing on the same and send a PR for the same.

I will post an update soon on the fix for this.

[jamdagni86]

There are two places from where the []methods field gets populated -

1. src/cmd/compile/internal/gc/reflect.go:dextratypeData - compile time. This writes the methods of the given object as an array
2. src/reflect/type.go:StructOf (this bug is from here)

My first solution was to have a struct structTypeDynamic like

type structTypeDynamic struct {
	structType
	u uncommonType
	m []method
}

and do

default:
	t := new(structTypeDynamic)
	typ = &t.structType
	ut = &t.u
	t.m = make([]method, len(methods))
	copy(t.m, methods)
}

This would also require a change in uncommonType.exportedMethods as

return (*(*[]method)(add(unsafe.Pointer(t), uintptr(t.moff), "t.xcount > 0")))[:t.xcount:t.xcount]

The above code assumes the underlying array is a slice. However, dextratypeData writes the backing []methods as array and the uncommonType.exportedMethods method would fail.

The simplest fix for this bug is to introduce structTypeFixed64 (and may be structTypeFixed128, like funcTypeFixedX) and add another case statement.

case len(methods) <= 64:
	t := new(structTypeFixed64)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)

Only problem with this approach is, we might still end up with a panic in case the number of methods is more than 64 (or 128). But I guess, that's still OK considering 64 (or 128) methods is already way too many methods to have on a type.

@ianlancetaylor let me know if the solution looks ok. I will send a PR for the same.

[ianlancetaylor]

If you're willing to do a little work, I think the correct approach is for StructOf to determine the right number of methods, and then call itself recursively to create a struct type with space for the right number of methods. We know that that struct type won't itself require any methods.

[jamdagni86]

@ianlancetaylor - I can allocate space for the right number of methods using this approach or even by using reflect.ArrayOf. However, I need to allocate it right after the uncommonType object as the existing logic of retrieving the methods is by adding uncommonType.moff to the base address of the uncommonType object.

[jamdagni86]

@ianlancetaylor one other thing I can do to fix this.

Add a flag argument to uncommonType called methodStorageSlice and modify the the following lines

go/src/reflect/type.go

Lines 2569 to 2596 in 1174ad3

	switch {
	case len(methods) == 0:
	t := new(structTypeUncommon)
	typ = &t.structType
	ut = &t.u
	case len(methods) <= 4:
	t := new(structTypeFixed4)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	case len(methods) <= 8:
	t := new(structTypeFixed8)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	case len(methods) <= 16:
	t := new(structTypeFixed16)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	case len(methods) <= 32:
	t := new(structTypeFixed32)
	typ = &t.structType
	ut = &t.u
	copy(t.m[:], methods)
	default:
	panic("reflect.StructOf: too many methods")
	}

to

	switch {
	case len(methods) == 0:
		t := new(structTypeUncommon)
		typ = &t.structType
		ut = &t.u
	default:
		t := new(structTypeDynamic)
		typ = &t.structType
		ut = &t.u
		t.m = make([]method, len(methods))
		copy(t.m, methods)
	}

and set methodStorageSlice to true.

In the methods uncommonType.exportedMethods and uncommonType.methods, we'll have to look for this flag and cast the unsigned.Pointer to either an array or a slice.

It would look like:

        methodPtr := add(unsafe.Pointer(t), uintptr(t.moff), "t.xcount > 0")
	if t.methodStorageSlice == true {
		methods = (*[1 << 16]method)(methodPtr)[:t.xcount:t.xcount]
	} else {
		methods = (*(*[]method)(methodPtr))[:t.xcount:t.xcount]
	}
	return methods

The same has to be replicated in runtime package as well.

I would have liked a cleaner approach than this to fix the issue. But this is the only thing I could think of.

Please let me know!

[ianlancetaylor]

@jamdagni86 Sorry, I didn't notice your comment of 22 days ago, and when I read it now I don't understand it. I'm suggesting that instead of using the existing structTypeFixedNN types, which already come with the method arrays where you want them, that if you need space for 100 methods, you have StructOf call itself recursively to construct the equivalent of structTypeFixed100.

[jamdagni86]

@ianlancetaylor - I think I figured out a way to fix this bug.
Essentially, the current design expects a struct to be represented by a structType field and an uncommonType field. If it has methods, an array of method will need to follow the uncommonType field. This is what the structs structTypeUncommon and structTypeFixedNN are for. (Comment by @crawshaw also describes the same thing)

go/src/reflect/type.go

Lines 2278 to 2299 in 1174ad3

	type structTypeUncommon struct {
	structType
	u uncommonType
	}
	// A *rtype representing a struct is followed directly in memory by an
	// array of method objects representing the methods attached to the
	// struct. To get the same layout for a run time generated type, we
	// need an array directly following the uncommonType memory. The types
	// structTypeFixed4, ...structTypeFixedN are used to do this.
	//
	// A similar strategy is used for funcTypeFixed4, ...funcTypeFixedN.
	// TODO(crawshaw): as these structTypeFixedN and funcTypeFixedN structs
	// have no methods, they could be defined at runtime using the StructOf
	// function.
	type structTypeFixed4 struct {
	structType
	u uncommonType
	m [4]method
	}

All the 3 fields need to be allocated sequentially because of the way they are accessed in certain places, like:

rtype.MethodByName calls rtype.uncommon which casts the receiver rtype as structTypeUncommon

go/src/reflect/type.go

Lines 704 to 710 in 1174ad3

	func (t *rtype) uncommon() *uncommonType {
	if t.tflag&tflagUncommon == 0 {
	return nil
	}
	switch t.Kind() {
	case Struct:
	return &(*structTypeUncommon)(unsafe.Pointer(t)).u

Also, uncommonType.methods and uncommonType.exportedMethods get the method backing array by adding offset to the base address of itself.

go/src/reflect/type.go

Lines 642 to 647 in 1174ad3

	func (t *uncommonType) exportedMethods() []method {
	if t.xcount == 0 {
	return nil
	}
	return (*[1 << 16]method)(add(unsafe.Pointer(t), uintptr(t.moff), "t.xcount > 0"))[:t.xcount:t.xcount]
	}

I cannot use StructOf to call itself to create enough space for structTypeFixedNN because it will need to have structType and uncommonType fields (which both have methods).

However, we could allocate enough space for the entire struct by using reflect.ArrayOf and cast a pointer to the beginning of this array as structType and the location after structType as uncommonType. Later, we can copy the methods after the uncommonType ends. Here's the fix:

Let me know your thoughts!

switch {
case len(methods) == 0:
	t := new(structTypeUncommon)
	typ = &t.structType
	ut = &t.u
default:
	szTyp := int(unsafe.Sizeof(structType{}))
	szUt := int(unsafe.Sizeof(uncommonType{}))
	szMethods := int(unsafe.Sizeof(methods[0]))

	szTotal := szTyp + szUt + int(len(methods)*szMethods)
	structArr := New(ArrayOf(szTotal, TypeOf(byte(0))))

	typ = (*structType)(unsafe.Pointer(structArr.Pointer()))
	ut = (*uncommonType)(unsafe.Pointer(structArr.Pointer() + uintptr(szTyp)))

	mb := structArr.Elem().Slice(szTyp+szUt, szTotal).Interface().([]byte)
	for i, m := range methods {
		copy(mb[i*szMethods:], (*(*[1 << 16]byte)(unsafe.Pointer(&m)))[:szMethods])
	}
}

[ianlancetaylor]

Before discussing your suggestion, can you explain why you don't want to use my suggestion? The advantage of the approach that I suggested above is that we allocate memory with the correct type, which simplifies the interface to the garbage collector.

[jamdagni86]

As I mentioned in my earlier comment, I can not call StructOf recursively since structType and uncommonType structs have methods. This would result in an infinite recursion.

Please let me know if I'm missing something :-)

[ianlancetaylor]

They have a fixed number of methods. Can we predefine that specific type?

[jamdagni86]

Yeah. I was wrong about infinite recursion though. This worked without infinite recursion, not sure why.
I will verify it before sending the PR.

switch {
	case len(methods) == 0:
	t := new(structTypeUncommon)
	typ = &t.structType
	ut = &t.u
default:
	tt := New(StructOf([]StructField{
		{Name: "S", Type: TypeOf(structType{})},
		{Name: "U", Type: TypeOf(uncommonType{})},
		{Name: "M", Type: ArrayOf(len(methods), TypeOf(methods[0]))},
	}))

	typ = (*structType)(unsafe.Pointer(tt.Pointer()))
	ut = (*uncommonType)(unsafe.Pointer(tt.Pointer() + unsafe.Sizeof(*typ)))

	copy(tt.Elem().FieldByName("M").Slice(0, len(methods)).Interface().([]method), methods)
}

[gopherbot]

Change https://golang.org/cl/128479 mentions this issue: reflect: fix StructOf panics when an embedded field is present