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

src/runtime: implement runtime.Version() function #2676

Merged
merged 3 commits into from
Mar 19, 2022

Conversation

ZauberNerd
Copy link
Contributor

@ZauberNerd ZauberNerd commented Mar 4, 2022

This adds the Version() function of the runtime package which embeds
the go version that was used to build tinygo.

For programs that are compiled with tinygo the version can be overriden
via the:
tinygo build -ldflags="-X 'runtime.buildVersion=abc'" flag.
Otherwise it will continue to use the go version with which tinygo was
compiled.

// It is either the commit hash and date at the time of the build or,
// when possible, a release tag like "go1.3".
func Version() string {
return "TODO: not implemented"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually something that's not too difficult to support. The compiler already supports the -X flag and we could add the version relatively easily. See these lines for a starting point:

tinygo/builder/build.go

Lines 846 to 850 in 3883550

// Insert values from -ldflags="-X ..." into the IR.
err = setGlobalValues(mod, config.Options.GlobalValues)
if err != nil {
return err
}

We could for example add runtime.buildVersion to GlobalValues earlier in the build. The version itself can be obtained using goenv.GorootVersionString.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR to your suggestion.
Now the tinygo runtime.Version() is always the version of golang that was used to build tinygo, but not the version of tinygo itself, which I think would be more useful?

Also, for some reason I wasn't able to set the version via go build -tags=llvm11 -ldflags="-X 'runtime.buildVersion=abc'" -o build/tinygo - shouldn't that work too, so that tinygo could set its own version during build?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the tinygo runtime.Version() is always the version of golang that was used to build tinygo, but not the version of tinygo itself, which I think would be more useful?

I agree that we should return the version of TinyGo used to build the binary here, which is what the main Go seems to do with respect to the version of Go used to build binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved and exported main.gitSha1 to goenv.GitSha1 to be able to access it from both the main and builder package.
Now the runtime.Version() function returns a version string similar to what tinygo version returns (0.23.0 - or, if it ends in -dev it will be the concatenation of the version and git sha 0.23.0-dev-f5170c0b).

https://github.com/tinygo-org/tinygo/pull/2676/files#diff-d871f892c3b7d415467578ac1742cccab676ec4cc21022b0b9b34f5741237eb7R208-R217

@ZauberNerd ZauberNerd force-pushed the runtime-version branch 2 times, most recently from a040e74 to c094a2d Compare March 10, 2022 17:20
@ZauberNerd ZauberNerd force-pushed the runtime-version branch 2 times, most recently from 0337f0d to f5170c0 Compare March 14, 2022 21:34
@ZauberNerd
Copy link
Contributor Author

ZauberNerd commented Mar 14, 2022

I get a few errors here, most are related to WASI, but one is:

tinygo:ld.lld: error: undefined symbol: notARealFunction
>>> referenced by builderr.go:9 (/home/ubuntu/tinygo/tests/testing/builderr/builderr.go:9)
>>>               /home/ubuntu/.cache/tinygo/thinlto/llvmcache-0A6BB726F2D94B251D948A1883CD488484B0EB09:(github.com/tinygo-org/tinygo/tests/testing/builderr_test.TestThing)
failed to run tool: ld.lld

I found: #2708 and the linked issues, but I have rebased and the error still persists.

Edit: I found --- PASS: TestTest/Host/Fail (14.24s) in the logs, so it seems the above error is just printed, but the test seems to pass.

The other ones seem to come from wasmtime:

And they look like:

--- FAIL: TestTest (0.00s)
    --- FAIL: TestTest/WASI (0.00s)
        --- FAIL: TestTest/WASI/Pass (32.81s)
            main_test.go:611: Error: failed to run main module `/tmp/tinygo2920845472/main`
            main_test.go:611: 
            main_test.go:611: Caused by:
            main_test.go:611:     0: failed to instantiate "/tmp/tinygo2920845472/main"
            main_test.go:611:     1: command export 'runtime.buildVersion' is not a function
            main_test.go:611: FAIL      github.com/tinygo-org/tinygo/tests/testing/pass 4.192s
            main_test.go:523: test failed
FAIL

Apparently there's a switch to allow unknown exports: bytecodealliance/wasmtime#2879, but I want to first understand why and how this error came to be before enabling any further options.
Or maybe one of you has seen this error already / has an idea what is happening?

@ZauberNerd
Copy link
Contributor Author

Adding the --allow-unknown-exports to the args here:

tinygo/main.go

Lines 276 to 305 in 3883550

if emulator[0] == "wasmtime" {
// create a new temp directory just for this run, announce it to os.TempDir() via TMPDIR
tmpdir, err := ioutil.TempDir("", "tinygotmp")
if err != nil {
return false, &commandError{"failed to create temporary directory", "tinygotmp", err}
}
args = append(args, "--dir="+tmpdir, "--env=TMPDIR="+tmpdir)
// TODO: add option to not delete temp dir for debugging?
defer os.RemoveAll(tmpdir)
// allow reading from directories up to module root
for _, d := range dirsToModuleRoot(result.MainDir, result.ModuleRoot) {
args = append(args, "--dir="+d)
}
// mark end of wasmtime arguments and start of program ones: --
args = append(args, "--")
if testVerbose {
args = append(args, "-test.v")
}
if testShort {
args = append(args, "-test.short")
}
if testRunRegexp != "" {
args = append(args, "-test.run="+testRunRegexp)
}
if testBenchRegexp != "" {
args = append(args, "-test.bench="+testBenchRegexp)
}
}

does fix the issue, but I don't understand why it is necessary.

And when I just build the test file tinygo test -target wasi -c os, it will generate os.test which, when converted to WAT, shows that runtime.buildVersion is mentioned in the exports:

  (table $T0 58 58 funcref)
  (memory $memory 3)
  (global $g0 (mut i32) (i32.const 65536))
  (global $runtime.buildVersion i32 (i32.const 92376))
  (global $g2 (mut i32) (i32.const 0))
  (global $g3 (mut i32) (i32.const 0))
  (export "memory" (memory 0))
  (export "malloc" (func $malloc))
  (export "free" (func $free))
  (export "calloc" (func $calloc))
  (export "realloc" (func $realloc))
  (export "_start" (func $_start))
  (export "runtime.buildVersion" (global 1))
  (export "asyncify_start_unwind" (func $asyncify_start_unwind))
  (export "asyncify_stop_unwind" (func $asyncify_stop_unwind))
  (export "asyncify_start_rewind" (func $asyncify_start_rewind))
  (export "asyncify_stop_rewind" (func $asyncify_stop_rewind))
  (export "asyncify_get_state" (func $asyncify_get_state))

Which I assume has somehow to do with the variable being set by the linker?
Ideally, I wouldn't want the export showing up here instead of setting the --allow-unknown-exports flag.

@ZauberNerd
Copy link
Contributor Author

Ok, so in the linked issue the proposed fix is to remove --export-dynamic linker flag from Rust when targeting wasm32-wasi: rust-lang/rust#81255
We can do the same by removing --export-dynamic in the targets/wasi.json file:

"--export-dynamic",

@ZauberNerd
Copy link
Contributor Author

Compiling a minimal program:

package main

import "runtime"

func main() {
    println(runtime.Version())
}

to WASI tinygo build -o main.wasm -target wasi ./main.go and running it with wasmtime: wasmtime ./main.wasm works fine when removing the --export-dynamic linker flag.
With the --export-dynamic flag set, the program crashes when running it with wasmtime wasmtime ./main.wasm but works when running it with: wasmtime --allow-unknown-exports ./main.wasm.

And looking at the resulting WAT files in both cases it seems the only change is whether the runtime.buildVersion exists in the exports or not.

Moving and exporting this variable from the main to the goenv package
allows us to use it from both the main and the builder package.
This is done in preparation to include the value in `tinygo build`
linker flags, so that we can embed the version and git sha into binaries
built with tinygo.
@ZauberNerd
Copy link
Contributor Author

ZauberNerd commented Mar 18, 2022

Now there's only the env.go tests failing:
It seems like these tests embed the os.Environ() and os.Args in the binary via the linker

Edit: Nevermind, seems like I overwrote config.Options.GlobalValues["runtime"] before.

This adds the `Version()` function of the `runtime` package which embeds
the go version that was used to build tinygo.

For programs that are compiled with tinygo the version can be overriden
via the:
`tinygo build -ldflags="-X 'runtime.buildVersion=abc'"` flag.
Otherwise it will continue to use the go version with which tinygo was
compiled.
Exporting symbols seems to embed them in the WASM exports section which
causes wasmtime to fail: bytecodealliance/wasmtime#2587
As a workaround, it is possible to specify the `--allow-unknown-exports`
flag on wasmtime.
But as discussed in the above linked issue, this seems to only be a
workaround. For the Rust compiler the fix was to remove the
`--export-dynamic` linker flag when targeting `wasm32-wasi`:
rust-lang/rust#81255
Which is waht this commit does for Tinygo too.
@ZauberNerd ZauberNerd changed the title src/runtime: add stub for runtime.Version() function src/runtime: implement runtime.Version() function Mar 18, 2022
@aykevl aykevl merged commit 0b5d300 into tinygo-org:dev Mar 19, 2022
@ZauberNerd ZauberNerd deleted the runtime-version branch March 19, 2022 14:37
@aykevl
Copy link
Member

aykevl commented Mar 19, 2022

Thank you! This looks good to me.

I hope that --export-dynamic doesn't change anything important.

@aykevl
Copy link
Member

aykevl commented Mar 19, 2022

Note that programs using runtime.Version() might expect the Go version string (from the currently used GOROOT), instead of the TinyGo version. That's hard to know, so let's keep it as-is.

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

Successfully merging this pull request may close these issues.

3 participants