feat(gnovm): improved native bindings

[thehowl]

This PR provides an initial transition and groundwork to move from native bindings as "package injections", whereby an injector function is called after package parsing and loading to provide built-in natives, to the new design described in #814.

Status

The PR is currently ready for review.

• Fix all tests and ensure precompiling has no issues
• Find out if the errors that arise when running the tests with DEBUG=1 are bugs introduced by this PR or not
• Create temporary exceptions for linked types (like std.{Coins,Address,Realm}), so that all normal native bindings run exclusively on the new system, awaiting //gno:link (see [META] improved native bindings #814).
• Finding a permanent place for ExecContext (see comment on gnovm/stdlibs/context.go); suggestions welcome (tm2/vm?)
• Include code generation in Makefiles and add a CI check to make sure PRs do generate code when necessary
• And some unit tests for the new functionality / potential edge cases.
• make sure all injected functions have // injected as a comment

Next steps, after merging this PR, include:

• Supporting precompilation of native functions by linking directly to the Go source in stdlibs/, removing the necessity of stdshim.
• Supporting the test-only tests/stdlibs/ directory in gno doc; as well as documenting native functions explicitly by adding the // injected comment after their declaration.
• Making all of the native-only packages defined in tests/imports.go native bindings; so that the special packages and native values are restricted only to ImportModeNativePreferred, while everything else uses documentable native bindings also for tests.

Implementation Summary

A new API has been added to gnolang.Store: NativeStore. This is a simple function which resolves a package and a name of a function to a native function.¹ This is used to resolve native bindings first and foremost in preprocess.go, which also adds information about the native binding in NativePkg and NativeName, two new fields in *gnolang.FuncValue.

The purpose of NativePkg and NativeName is to 1. enable assigning native functions to other function types, including function parameters or func type variables and 2. being able to easily and effectively persist the FuncValue during realm de/serialization. This way, op_call.go can recover the value of nativeBody if it is nil.

On the other side, examples of the new stdlibs can be seen in the gnovm/stdlibs directory, where everything except a few functions in package std have been ported to the new system. The code generator in misc/genstd parses the AST of both the Go and Gno files, and ultimately generates native.go, which contains the porting code from the Gno function call to Go and back.

To support the test contexts, which needs to have different function implementation or entirely new functions, an additional stdlibs directory was added to the tests directory. This is still code generated with the same generator and follows the same structure. When stdlibs are loaded for tests, the code in tests/stdlibs/, if it exists, is loaded with "higher priority" compared to normal stdlibs code.²

Suggested order for reviewing (and some notes)

1. pkg/gnolang for the core of the changes, starting from values.go, preprocess.go, op_call.go, store.go, realm.go which show how NativePkg, NativeName and nativeBody interact

• gnovm/pkg/gnolang/op_eval.go adds the type of the evaluated expression, as I think it's useful to understand in what terms something is being evaluated; specifically I added it to better distinguish between DEBUG: EVAL: (*gnolang.CallExpr) std<VPBlock(3,0)>.TestCurrentRealm() and DEBUG: EVAL: (*gnolang.SelectorExpr) std<VPBlock(3,0)>.TestCurrentRealm. I think it's useful, but we can remove it if we prefer to keep it simpler.
• gnovm/pkg/gnolang/op_expressions.go adds a new debug print pretending we're popping and pushing the value stack. This is to make clear what's happening in the debug logs (we're really swapping the value at the pointer), and I've made it v[S] so it's clear that the log is not coming from PushValue/PopValue.
• I removed a goto-loop in gnovm/pkg/gnolang/values.go because I don't see it as beneficial when it's essentially just doing a for loop; if the concern was inlining, even with goto the function is too "costly" for the go compiler to inline.

2. misc/genstd contains the code generator and the template it uses to generate the files, ie. gnovm/stdlibs/native.go and gnovm/tests/stdlibs/native.go.
3. stdlibs and tests/stdlibs changes show the new stdlib directories, which genstd uses to generate code. The tests/stdlibs is available only in test environments, and there are also some overriding definitions to those in normal stdlibs.

• gnovm/stdlibs/encoding/base64/base64.gno removes a check on strconv.IntSize. This is because strconv.IntSize is dependent on the system and thus is non-deterministic behaviour on the Gnovm. I don't know if int in Gno is == Go's int or it is precisely defined to be 64-bits; in any case I don't think it should be dependent on the system as this wouldn't work in a blockchain scenario.

4. Changes to tm2 and gnovm/tests/imports.go mostly relate to wiring up the new NativeStore to the gnovm where it's used.
5. Changes to examples and tests/files (except for float5) are all updates to golden tests.

Checklists...

Contributors Checklist

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests

Maintainers Checklist

• Checked that the author followed the guidelines in CONTRIBUTING.md
• Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
• Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

Footnotes

1. Currently, the only native bindings that are supported with the new system are top-level functions. I think we can keep it this way in the spirit of KISS; maybe think of extending it if we find appropriate usage. ↩

2. See gnolang.RunMemPackageWithOverrides ↩

[tbruyelle]

About adding gno.Machine to go functions, I naively changed this function :

// gnovm/stdlibs/math/native.go
- func Float64bits(f float64) uint64     { return math.Float64bits(f) }
+ func Float64bits(m *gno.Machine, f float64) uint64 { return math.Float64bits(f) }

As a result, code generator updated the related binding like that :

// gnovm/stdlibs/native.go
-			r0 := lib1.Float64bits(p0)
+			r0 := lib1.Float64bits(
+				m,
+				p0)

Then I ran the related test and it was OK :

$ go test ./gnovm/tests/ -v -run 'TestFiles/float5' -count 1
=== RUN   TestFilesNative
=== RUN   TestFilesNative/float5_native.gno
--- PASS: TestFilesNative (0.01s)
    --- PASS: TestFilesNative/float5_native.gno (0.00s)
=== RUN   TestFiles
=== RUN   TestFiles/float5_stdlibs.gno
--- PASS: TestFiles (0.02s)
    --- PASS: TestFiles/float5_stdlibs.gno (0.01s)
PASS
ok  	github.com/gnolang/gno/gnovm/tests	0.035s

Did I miss something ?

[ajnavarro]

I'm not a big fan of code gen :). This solution is definitely better and cleaner than using pn.DefineNative, but do you think that might be another way to implement this? maybe having two ways of adding external code that won't be executed by the VM, one for the standard library and the other for specific std calls (using this method).

[thehowl]

@tbruyelle yes, it may work on some functions. However it doesn't seem to work particularly well for instance with this diff:

diff --git a/gnovm/stdlibs/time/time.gno b/gnovm/stdlibs/time/time.gno
index 7b7e45ba..dfae44b0 100644
--- a/gnovm/stdlibs/time/time.gno
+++ b/gnovm/stdlibs/time/time.gno
@@ -80,8 +80,6 @@ package time
 
 import (
 	"errors"
-
-	ios "internal/os" // XXX to access time.
 	// XXX _ "unsafe" // for go:linkname
 )
 
@@ -1071,17 +1069,14 @@ func daysSinceEpoch(year int) uint64 {
 	return d
 }
 
-/* XXX replaced with ios.Now()
-// Provided by package runtime.
 func now() (sec int64, nsec int32, mono int64)
-*/
 
 // XXX SHIM
 // runtimeNano returns the current value of the runtime clock in nanoseconds.
 //
 //go:linkname runtimeNano runtime.nanotime
 func runtimeNano() int64 {
-	_, _, mono := ios.Now() // XXX now()
+	_, _, mono := now()
 	return mono
 }
 
@@ -1095,7 +1090,7 @@ var startNano int64 = runtimeNano() - 1
 
 // Now returns the current local time.
 func Now() Time {
-	sec, nsec, mono := ios.Now() // XXX now()
+	sec, nsec, mono := now()
 	mono -= startNano
 	sec += unixToInternal - minWall
 	if uint64(sec)>>33 != 0 {

(note: now() is implemented as X_now in native.go)

This generates the following panic:

   --- FAIL: TestFiles/addr0b_stdlibs.gno (0.28s)
panic: fail on files/addr0b_stdlibs.gno: got unexpected error: --= Error =--
Data: runtime.boundsError{x:-1, y:2, signed:true, code:0x3}
Msg Traces:
    0  /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:2851 - github.com/gnolang/gno/_test/net/http/http.gno:1
    1  /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:2851 - main/files/addr0b_stdlibs.gno:1
Stack Trace:
    0  /home/howl/oc/gno/tm2/pkg/errors/errors.go:20
    1  /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:2851
    2  /usr/lib/go/src/runtime/panic.go:884
    3  /usr/lib/go/src/runtime/panic.go:153
    4  /home/howl/oc/gno/gnovm/pkg/gnolang/machine.go:1501
    5  /home/howl/oc/gno/gnovm/pkg/gnolang/op_assign.go:9
[...]

It definitely should work on some functions; the panic issue may be more related to calling native functions within the same package (which, by the way, is something that I think we should support, instead of continuing with what we're doing now whereby if we need to use native functions within a package, they need to be in a different package in order for the injecting function to work.)

[thehowl]

@ajnavarro on the other hand, I'm not a fan of reflection for "hot" code :) (partly for performance, but also because I think that not using reflection generates a lot of dumb code which actually makes it easier to track down what it's doing compared to reflection; but I digress).

It is not entirely clear to me what you're saying here:

maybe having two ways of adding external code that won't be executed by the VM, one for the standard library and the other for specific std calls (using this method).

So you're proposing that there should be code-gen for functions which rely on *gno.Machine, but not for the rest of standard library functions?

But aside from personal preference, I'll try stating my case as to why I think code generation is a better fit for what we're trying to do here:

I'm starting off from the premise that the primary goal is that of having native functions be declared in Gno code (but not defined); this serves the purpose of improving documentation for end users, as native methods don't have to be tracked down in Gno internals but can be seen clearly in the source and exposed by tools such as gno doc.

Then, a secondary goal is making it easier to define native functions and to allow for a (likely) increase of their use as we add more standard libraries; by making it so that some functions can be simply defined with their matching gno signature, without having to do all the boilerplate that is needed in stdlibs.go right now to even get a parameter and return a result, converting from the go type system to Gno. Note that this is not to say that standard libraries should just be bindings to go's standard libraries; discussing with Ray I think one thing that has not been made very clear is that I still think native bindings have very limited scope in the standard libraries to where they make sense (you need *gno.Machine, and/or the function would be very slow in pure gno)

So, with these premises, I think the arguments in favour are:

• Codegen allows us to make native bindings work by simply defining a function in a .go file in stdlibs; we don't need to additionally write a call to pn.DefineNative (or similar) ourselves to "register" the function. Note that reflection is not capable of "listing" the functions that exist under a package name; also in relation to how the Go linker works (ie. some functions can be trimmed out of the binary if they're not used).
• As I already mentioned at the beginning, codegen generates dumb code while also being more performant than reflection. I've written my fair share of reflection code (in a previous job, I'd made the majority of the REST API work through "automatic" handlers (crud + list), which relied on a struct's fields and two methods, Access and Write, all reflection based, to do all the business logic), and I think I find stacktraces more readable when they point to a clear line in the generated code rather than a line which is reflect.ValueOf(o).Elem().Field(0).Addr().Interface() (of course, exaggerating, but I hope you see my point)
• It works well here, because we don't need to support native functions outside of this repo as a Gno feature. So nobody other than people working on Gno stdlibs would need to interface themselves with this (=> we'll never tell people that they need to go install the codegen).

Ultimately, I think that we want native code organised in the different packages where they're applied; so a "decent" proposal for how the code would look, taking sha256 as an examples to show how the reflection approach would work, is the following:

package sha256

import (
	"crypto/sha256"
)

var Fns = map[string]interface{}{
	"Sum256": func(data []byte) [32]byte {
		return sha256.Sum256(data)
	},
}

// stdlibs/native.go or similar

import "github.com/gnolang/gno/stdlibs/crypto/sha256"

// ...
	addBindings(sha256.Fns)
// ...

Note that each additional package added or remove entails modifying a relative stdlibs/native.go or similar file. Plus, if you want to make sure that the functions match those of gno code, you need to add some code checking for it at runtime; while here you get it trivially at "gen-time" because checking that they match is part of trivial validation that is done even by the POC at this stage.

</daily-wall-of-text> :)

[ajnavarro]

You sold me pretty well the code generation, I buy it :). Let's push this forward and improve the solution if needed through the implementation process.

My only worry is that we might be mixing our std package with some parts of the Go standard library, that's why I proposed different ways to add custom external methods to the VM, one auto for all the validated methods from the standard library, and another way for custom methods being executed outside the VM OPs system.

Nit: I think your example above could be refactored to:

var Fns = map[string]interface{}{
	"Sum256": sha256.Sum256
	},
}

right?

[thehowl]

Nit: I think your example above could be refactored to [...]

Yes, you're right :)

My only worry is that we might be mixing our std package with some parts of the Go standard library, that's why I proposed different ways to add custom external methods to the VM,

I think it's useful to start with one system, and we'll see how things develop from there :)

[tbruyelle]

It definitely should work on some functions; the panic issue may be more related to calling native functions within the same package (which, by the way, is something that I think we should support, instead of continuing with what we're doing now whereby if we need to use native functions within a package, they need to be in a different package in order for the injecting function to work.)

I tested and it's not related to the presence of *gno.Machine in parameters. With or without this parameter in X_now(), the same panic occurs with the bound error.
If you think it's because of identical package, did you try with a different one to confirm ? I would like to but I'm not sure what do you mean by calling native functions within the same package.

EDIT: OK I think I understand now, it looks like the DefineNative is not invoked properly when the related function package is not declared in the imports (so when it comes from the same package). That probably requires a tricky fix ^^

EDIT2: the code you commented above is probably where you should handle native local function like now.

[thehowl]

I tested and it's not related to the presence of *gno.Machine in parameters. With or without this parameter in X_now(), the same panic occurs with the bound error.
If you think it's because of identical package, did you try with a different one to confirm ? I would like to but I'm not sure what do you mean by calling native functions within the same package.

Well, I did some homework to try to track this issue further. What I found is that this is an issue of the time package as a global variable declaration depends on the now() function, which should be injected natively:

var startNano int64 = runtimeNano() - 1

(runtimeNano then calls now())

Though of course I believe that this should be a case we can and should handle correctly.

EDIT2: the code you commented above is probably where you should handle native local function like now.

That would be a good idea, the problem is that Go2Gno and ParseFile are ignorant about their context; ie. they don't know what package they're parsing. stdlib injections obviously need to know that, so I don't think it's actually the correct spot where to do it.

I've uncommented the code to add the panic() body. This makes it so that user-defined bodyless functions panic when they're called; ie. this feature is ineffective outside of the gnovm.

---

Current state

The currently-pushed code tries to inject the native function by having a new option/field in the Machine struct: Injector. Further below in the machine.go file I try to call it where it seems appropriate (after initial parsing, before declaration of global variables). With the currently pushed code, Injector works and we get the log output that it is being redefined correctly:

REDEFINE now 0xc000990900

However, we also log the point where now() is called with OpCall, and the pointer to *FuncValue is different -- maybe it is duplicated at one point between the value in pn and the value in the machine itself?

CALL NOW 0xc00125b290 &gnolang.FuncValue{Type:(*gnolang.FuncType)(0xc000673e00), IsMethod:false, Source:(*gnolang.FuncDecl)(0xc000516400), Name:"now", Closure:(*gnolang.Block)(0xc000b8c9
60), FileName:"time.gno", PkgPath:"time", body:[]gnolang.Stmt{(*gnolang.ExprStmt)(0xc000518000)}, nativeBody:(func(*gnolang.Machine))(nil)}

Note that aside from the pointer being different, crucially, nativeBody is nil, so DefineNative has managed to modify a different *FuncValue.

So I think we either:

1. Need to understand where the *FuncValue are split, and have a way for an injector function to inject into the duplicated FuncValue
2. Or alternatively, work on injecting native functions in the same way that we inject uverse functions -- and additionally, make definitions of bodyless functions fail at parse-time instead of runtime.

I think 2. is better, possibly, although I'm not sure (a) where we define uverse functions when running RunFiles and (b) how to modify definitions to fail on empty func calls effectively (this I would imagine somewhere in runDeclarationFor inside machine.go)

I'm currently running tests with this command:

go test -v -p 1 -timeout=30m tests/*.go -test.short -run 'TestFiles$/^addr0b_'

As addr0b it is the earliest failing test, as it imports the time package.

[tbruyelle]

I've uncommented the code to add the panic() body. This makes it so that user-defined bodyless functions panic when they're called; ie. this feature is ineffective outside of the gnovm.

This confuses me, since it's uncommented, the now function generates a panic call to bodyless/external function is invalid outside of standard library when I run the test. It seems like not only user-defined function are concerned.

[moul]

Need to understand where the *FuncValue are split, and have a way for an injector function to inject into the duplicated FuncValue

Give a look at preprocess.go. I'll conduct my own researches too, so feel free also to just skip the now case and focus on the rest.

Or alternatively, work on injecting native functions in the same way that we inject uverse functions -- and additionally, make definitions of bodyless functions fail at parse-time instead of runtime.

I suggest keeping a strong separation, we're considering making uverse even more independent; at the end we'll probably have something like this:

• layer0: minimal uverse
• layer1: "gnolang1.x"
• layer2: stdlibs + userdefined libs + contracts

[zivkovicmilos]

I'm severely late to the party with an official review, even though I've went through the PR months ago.

It is great. The detailed description you've provided in the PR, but also as part of the code changes, were super helpful in helping me navigate the scope of what you set out to accomplish. I'm not 100% familiar with the VM code, so it was nice having a guard rail while diving into the code. I genuinely wish all of our PRs were this detailed 🙂

The comments I've left are mostly nitpicks, and questions I've had regarding implementations. Please feel free to resolve anything that requires more than a nanosecond to fix / change; I understand this PR is on the back-burner as-is, and I want us to move it into the master codestream as soon as possible.

Please update the PR with the latest master changes, so we can go ahead and close the chapter on this effort 🚀

[jaekwon]

I have a few remaining suggestions and one correction (the GetPointerTo can be inlined). Otherwise pre-approving. Sorry this took too long. But also this is the coolest single contribution I've ever seen for Gno so far. Thank you!

[jaekwon]

where is the list of "notable contributions"? put this in dar.

[thehowl]

(merged as the failing CI was just due to codecov)

[piux2]

• How is template.tmpl file generated? It seems go generate does not work.
• Where is the source of line 55, or is it manually coded?
• Let's say if we found a bug in generated native.go. what are the steps involved to make changes and fix it?