cbor icon indicating copy to clipboard operation
cbor copied to clipboard

feature: Support for compilation via tinygo

Open dereulenspiegel opened this issue 3 years ago • 16 comments

I tried to build a project using this library via tinygo (because this project would also make sense in microcontrollers), but unfortunately tinygo doesn't support reflect (or at least certain parts of it. The error reported is:

../../../../pkg/mod/github.com/fxamacker/cbor/[email protected]/decode.go:1019:17: MakeMapWithSize not declared by package reflect
../../../../pkg/mod/github.com/fxamacker/cbor/[email protected]/tag.go:196:17: contentType.PkgPath undefined (type reflect.Type has no field or method PkgPath)

It would be awesome if we could get this library to compile via tinygo to support microcontrollers and smaller WASM modules. Unfortunately I am not deep enough into the architecture of cbor to understand how hard the dependency to these reflect methods is and how to resolve this.

Is this possible at all?

dereulenspiegel avatar Aug 20 '21 10:08 dereulenspiegel

Supporting tinygo seems like a worthwhile feature. I would need to look into it to understand how much work this would take.

fxamacker avatar Aug 23 '21 01:08 fxamacker

@fxamacker Awesome thanks. Really looking forward to using your cbor module on something like a NRF52 series microcontroller.

dereulenspiegel avatar Aug 23 '21 07:08 dereulenspiegel

Supporting tinygo seems like a worthwhile feature. I would need to look into it to understand how much work this would take.

Seems that only following in decoder.go would need some other approach, since tinygo reflect package as for now does not have reflect.MakeMapWithSize method implemented

https://github.com/fxamacker/cbor/blob/9e37184acacd7330c79de271370b26d57c228718/decode.go#L1256-L1262

so for instance following compiles with tinygo

if v.IsNil() {
  v.Set(reflect.MakeMap(tInfo.nonPtrType))
} 

mkungla avatar May 23 '22 22:05 mkungla

Hey @mkungla thanks for looking into this! I'll take a closer look this weekend.

fxamacker avatar May 26 '22 12:05 fxamacker

TL;DR master branch requires extensive changes to support tinygo but features/stream-mode branch (superset of master branch) would require fewer changes to allow tinygo to use StreamEncoder and StreamDecoder (which don't exist in master yet).

@dereulenspiegel @mkungla I've been looking into tinygo and it's a really cool project! I experimented with this version of tinygo:

tinygo version 0.23.0 linux/amd64 (using go version go1.18.2 and LLVM version 14.0.0)

tinygo's support for reflect is still very much a work in progress and it uses stubs that allow compiling which would panic at runtime.

For example, tinygo has many stubs like these:

image

In tinygo's value.go, there are 30 matches for panic("unimplemented: which allows us to compile but they panic at runtime:

fa@tinygo-amd64:~/go/src/github.com/fxamacker/cbor_main$ ./cbor_main
panic: unimplemented: (reflect.Type).FieldByName()
Aborted (core dumped)

Based on these findings, adding support for tinygo 0.23 would require more extensive changes than anticipated.

I'm going to keep this issue open because tinygo is making progress incrementally adding more support for reflect. In the meantime, I will:

  • look into updating features/stream-mode branch so tinygo can use StreamEncoder and StreamDecoder
  • look into adding support for 32-bit arch and revisit tinygo when they tag new releases

Thanks again for suggesting tinygo!

fxamacker avatar May 28 '22 22:05 fxamacker

@fxamacker in brief look of your code I see that reflect package is primarily used to get underlying builtin type of value and value it self to be encoded or decoded?

If so, it might be possible to get rid of the `reflect' package as a dependency altogether and problem solved 😄. For example, you can create an "internal" package to get types and values that contains something as follows. The following is just a bit of experimental code I have, which of course is not a out of box solution.

package internal

import "unsafe"

// interface for the header of builtin value
type typeiface struct {
	kind *kindinfo
	ptr  unsafe.Pointer
}

// minimal builtin kind info struct needed to get that info
type typeinfo struct {
	size       uintptr
	ptrdata    uintptr // number of bytes in the kinde that can contain pointers
	hash       uint32  // hash of type; avoids computation in hash tables
	tflag      uint8   // extra type information flags
	align      uint8   // alignment of variable with this type
	fieldAlign uint8   // alignment of struct field with this type
	kind       uint8   // enumeration for C
}

type Kind uint

const (
	KindInvalid Kind = iota
	KindBool
	KindInt
	KindInt8
	KindInt16
	KindInt32
	KindInt64
	KindUint
	KindUint8
	KindUint16
	KindUint32
	KindUint64
	KindUintptr
	KindFloat32
	KindFloat64
	KindComplex64
	KindComplex128
	KindArray
	KindChan
	KindFunc
	KindInterface
	KindMap
	KindPointer
	KindSlice
	KindString
	KindStruct
	KindUnsafePointer
)

var kindNames = []string{
	KindInvalid:       "invalid",
	KindBool:          "bool",
	KindInt:           "int",
	KindInt8:          "int8",
	KindInt16:         "int16",
	KindInt32:         "int32",
	KindInt64:         "int64",
	KindUint:          "uint",
	KindUint8:         "uint8",
	KindUint16:        "uint16",
	KindUint32:        "uint32",
	KindUint64:        "uint64",
	KindUintptr:       "uintptr",
	KindFloat32:       "float32",
	KindFloat64:       "float64",
	KindComplex64:     "complex64",
	KindComplex128:    "complex128",
	KindArray:         "array",
	KindChan:          "chan",
	KindFunc:          "func",
	KindInterface:     "interface",
	KindMap:           "map",
	KindPointer:       "ptr",
	KindSlice:         "slice",
	KindString:        "string",
	KindStruct:        "struct",
	KindUnsafePointer: "unsafe.Pointer",
}

func (t Kind) String() (str string) {
	if uint(t) < uint(len(kindNames)) {
		str = kindNames[uint(t)]
	}
	return
}

// Kill "reflect"
func GetUnderlyingValueAndKind(in any, withvalue bool) (value any, kind Kind) {
	e := (*typeiface)(unsafe.Pointer(&in))

	// check whether it is really a pointer or not.
	t := e.kind
	if in == nil || t == nil {
		return nil, KindInvalid
	}

	// there are 27 kinds.
	// check whether t is stored indirectly in an interface value.
	f := uintptr(Kind(t.kind & ((1 << 5) - 1)))
	if t.kind&(1<<5) == 0 {
		f |= uintptr(1 << 7)
		kind = Kind(f & (1<<5 - 1))
	} else {
		kind = Kind(t.kind & ((1 << 5) - 1))
	}
        
        // return early if you only need to know underlying kind
	if !withvalue {
		return nil, kind
	}
	switch kind {
	case KindBool:
		value = *(*bool)(e.ptr)
	case KindInt:
		value = *(*int)(e.ptr)
	case KindInt8:
		value = *(*int8)(e.ptr)
	case KindInt16:
		value = *(*int16)(e.ptr)
	case KindInt32:
		value = *(*int32)(e.ptr)
	case KindInt64:
		value = *(*int64)(e.ptr)
	case KindUint:
		value = *(*uint)(e.ptr)
	case KindUint8:
		value = *(*uint8)(e.ptr)
	case KindUint16:
		value = *(*uint16)(e.ptr)
	case KindUint32:
		value = *(*uint32)(e.ptr)
	case KindUint64:
		value = *(*uint64)(e.ptr)
	case KindUintptr, KindPointer, KindUnsafePointer:
		value = *(*uintptr)(e.ptr)
	case KindFloat32:
		value = *(*float32)(e.ptr)
	case KindFloat64:
		value = *(*float64)(e.ptr)
	case KindComplex64:
		value = *(*complex64)(e.ptr)
	case KindComplex128:
		value = *(*complex128)(e.ptr)
	case KindString:
		value = *(*string)(e.ptr)
        // ... other supported kinds
	}
	return value, kind
}

mkungla avatar Aug 19 '22 23:08 mkungla

@fxamacker has that issue become obsolete

mkungla avatar Dec 29 '22 15:12 mkungla

hi @mkungla reflect is used for setting value during decoding, in addition to getting underlying value and type during encoding and decoding. So it is quite a task to replace it.

As of today, both dev and release branches in TinyGo don't yet have all the reflect features needed by this library, such as support for map, etc. TinyGo is refactoring reflect in PR https://github.com/tinygo-org/tinygo/pull/2640 and it got scheduled for TinyGo v0.28 milestone. Not sure yet which reflect features TinyGo will add first after that refactoring.

Ideally, I would love to support TinyGo without adding special workaround for it or providing new API, but that may be necessary (if TinyGo decides not to implement some missing features) so I would like to keep this issue open.

fxamacker avatar Dec 29 '22 21:12 fxamacker

Quick update: TinyGo released 0.28 today (June 11, 2023) and it includes 24 improvements related to reflect.

I need to work this weekend but will take a look after wrapping up a couple urgent projects.

At a glance, this looks promising. When time allows, it would be useful to see what reflect support are still missing (if any) in 0.28 that is currently used in this codec.

fxamacker avatar Jun 11 '23 15:06 fxamacker

TinyGo 0.28.1 allowed fxamacker/cbor to pass more tests, so that's the good news. :sweat_smile:

  • First issue is MapIter.Key() returning key with wrong type for maps with any or interface{} keys. I opened issue tinygo-org/tinygo#3794 and dgryski opened PR tinygo-org/tinygo#3795 to fix it.

  • After recompiling with PR tinygo-org/tinygo#3795, remaining issue is TinyGo's Type.AssignableTo in reflect not yet implementing support for interface. Since AssignableTo isn't directly called by fxamacker/cbor, modifying the codec might not be as clean as TinyGo implementing more of the AssignableTo function.

The above two items appear to be the main blockers and other remaining issues appear to be limited to tests.

fxamacker avatar Jun 18 '23 20:06 fxamacker

I created branch https://github.com/fxamacker/cbor/tree/fxamacker/cbor-tinygo based on fxamacker/cbor v2.5.0-beta4.

It passes tests when compiled with TinyGo 0.28.1 patched with PR https://github.com/tinygo-org/tinygo/pull/3795.

A tradeoff is the removal of one feature: codec cannot decode CBOR tag to Go interface when compiled with TinyGo. Instead, it will return UnmarshalError. This tradeoff is caused by TinyGo v0.28.1 not fully implementing Type.AssignableTo.

I'll update this issue as we make more progress.

fxamacker avatar Jun 18 '23 23:06 fxamacker

dgryski's https://github.com/tinygo-org/tinygo/pull/3795 got merged :tada:

fxamacker avatar Jul 02 '23 15:07 fxamacker

@fxamacker Sorry, I didn't see this issue and opened #499 to add build tag for tinygo to move it into main branch.

x448 avatar Feb 27 '24 19:02 x448