Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Support for compilation via tinygo #295

Open
dereulenspiegel opened this issue Aug 20, 2021 · 15 comments
Open

feature: Support for compilation via tinygo #295

dereulenspiegel opened this issue Aug 20, 2021 · 15 comments
Labels
TinyGo TinyGo compatibility

Comments

@dereulenspiegel
Copy link

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?

@fxamacker fxamacker added enhancement New feature or request undecided Final decision hasn't been made yet labels Aug 23, 2021
@fxamacker
Copy link
Owner

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

@dereulenspiegel
Copy link
Author

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

@mkungla
Copy link

mkungla commented May 23, 2022

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

cbor/decode.go

Lines 1256 to 1262 in 9e37184

if v.IsNil() {
mapsize := count
if !hasSize {
mapsize = 0
}
v.Set(reflect.MakeMapWithSize(tInfo.nonPtrType, mapsize))
}

so for instance following compiles with tinygo

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

@fxamacker
Copy link
Owner

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

@fxamacker
Copy link
Owner

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 fxamacker removed the undecided Final decision hasn't been made yet label May 28, 2022
fxamacker added a commit that referenced this issue May 28, 2022
Replaced reflect.MakeMapWithSize() with reflect.MakeMap()
to allow compiling with tinygo 0.23 (latest version).

Only StreamEncoder and StreamDecoder can be used with
tinygo.  Other features may encounter stubs in tinygo
that panics due to unimplmented Go features.

For more details, see
#295 (comment)
@fxamacker fxamacker added TinyGo TinyGo compatibility and removed enhancement New feature or request labels May 29, 2022
@mkungla
Copy link

mkungla commented Aug 19, 2022

@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
Copy link

mkungla commented Dec 29, 2022

@fxamacker has that issue become obsolete

@fxamacker
Copy link
Owner

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 tinygo-org/tinygo#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
Copy link
Owner

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
Copy link
Owner

fxamacker commented Jun 18, 2023

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

  • 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
Copy link
Owner

fxamacker commented Jun 18, 2023

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 tinygo-org/tinygo#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
Copy link
Owner

fxamacker commented Jul 2, 2023

dgryski's tinygo-org/tinygo#3795 got merged 🎉

@x448
Copy link
Contributor

x448 commented Feb 27, 2024

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

@progrium
Copy link

Is there an issue in TinyGo to track AssignableTo?

@fxamacker
Copy link
Owner

@progrium Good catch! 👍 I just opened issue at TinyGo to track AssignableTo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TinyGo TinyGo compatibility
Projects
None yet
Development

No branches or pull requests

5 participants