ydnar / wasm-tools-go

WebAssembly + Component Model tools for Go
Apache License 2.0
38 stars 5 forks source link

weird value printed when using function returning #95

Closed rajatjindal closed 6 days ago

rajatjindal commented 1 month ago

Hello again,

I am trying to use wasm-tools-go to generate bindings for the following wit:

interface variables {
    /// Get an application variable value for the current component.
    ///
    /// The name must match one defined in in the component manifest.
    get: func(name: string) -> result<string, error>;

    /// The set of errors which may be raised by functions in this interface.
    variant error {
        /// The provided variable name is invalid.
        invalid-name(string),
        /// The provided variable is undefined.
        undefined(string),
        /// A variables provider specific error has occurred.
        provider(string),
        /// Some implementation-specific error has occurred.
        other(string),
    }
}

when using the bindings generated, I see weird value printed:

key: password, val is "\x00\x00\x00\x00\x00\x00"

pseudo code:

func Get(key string) (string, error) {
    result := variables.Get(key)
    if result.IsErr() {
        return "", errorVariantToError(*result.Err())
    }

    stdout.GetStdout().Write(cm.ToList([]byte(fmt.Sprintf("key: %s, val is %q \n", key, *result.OK()))))

    return *result.OK(), nil
}

I printed the value on host component implementation side, and it seems to be printing and returning the correct value.

any pointers what could be wrong here? thank you so much for all the help so far.

ydnar commented 2 weeks ago

Hi there! Can you try with the latest version of wit-bindgen-go?

rajatjindal commented 2 weeks ago

Hi @ydnar, I tried this again today, and still running into same prob. I am adding some more details incase if that helps narrow down the problem:

wit file:

interface variables {
    /// Get an application variable value for the current component.
    ///
    /// The name must match one defined in in the component manifest.
    get: func(name: string) -> result<string, error>;

    /// The set of errors which may be raised by functions in this interface.
    variant error {
        /// The provided variable name is invalid.
        invalid-name(string),
        /// The provided variable is undefined.
        undefined(string),
        /// A variables provider specific error has occurred.
        provider(string),
        /// Some implementation-specific error has occurred.
        other(string),
    }
}

generated code (ommitting error functions):

// Get represents the imported function "get".
//
// Get an application variable value for the current component.
//
// The name must match one defined in in the component manifest.
//
//  get: func(name: string) -> result<string, error>
//
//go:nosplit
func Get(name string) (result cm.ErrResult[string, Error]) {
    name0, name1 := cm.LowerString(name)
    wasmimport_Get((*uint8)(name0), (uint32)(name1), &result)
    return
}

//go:wasmimport fermyon:spin/variables@2.0.0 get
//go:noescape
func wasmimport_Get(name0 *uint8, name1 uint32, result *cm.ErrResult[string, Error])

When I print this inside host component:

tracing::trace!("variables get is {:?}", val);

## above prints:
# variables get is Ok("works2")

When I print this inside my guest code function:

func Get(key string) (string, error) {
    result := variables.Get(key)
    if result.IsErr() {
        return "", errorVariantToError(*result.Err())
    }

    val := result.OK()
    println(fmt.Sprintf("variables get inside sdk is %#v", *val))
    return *result.OK(), nil
}

func println(msg string) {
    stdout.GetStdout().Write(cm.ToList([]byte(msg)))
    stdout.GetStdout().Write(cm.ToList([]byte("\n")))
}

## above prints
# variables get inside sdk is \"\\x00\\x00\\x00\\x00\\x00\\x00\"

Thank you

rajatjindal commented 2 weeks ago

I realize my last comment is almost same as the original description. I will try to narrow down this to a small example, but if in the meantime you have any ideas that I could try, kindly let me know. thank you again.

rajatjindal commented 2 weeks ago

(kindly ignore me if this is incorrect)

do we need to lower/lift the string in the result here?

ydnar commented 2 weeks ago

Call OK() on the result to extract the string.

rajatjindal commented 2 weeks ago

I did call .OK() on the result (kindly refer to the partial code below), but still get same value

val := result.OK()
println(fmt.Sprintf("variables get inside sdk is %#v", *val))
ydnar commented 2 weeks ago

Can you use %s? Val should be *string.

rajatjindal commented 2 weeks ago

Hi Randy, thank you for your continued feedback on this. I tried this as well, and see following:

variables get inside sdk is \0\0\0\0\0\0\0

I should mention that the number of \0 here (or \x00 that I mentioned earlier) is exactly the same as number of characters that I am expecting as output. It seems like its just the content which is not correct.

ydnar commented 1 week ago
  1. Have you successfully been able to have an imported function return a string?
  2. Is the length of the returned string correct (7 bytes)? It’s just returning blank memory?
  3. Can you check the value of the unsafe.StringData pointer of the returned string?
  4. What implements the get() function?
rajatjindal commented 1 week ago

Hi Randy,

I just tried this with a host-component that implements following wit:

    /// Return a list of all the keys in kv store
    get-keys: func() -> result<list<string>, error>;

and the result I receive back is (the number of items are same as the unique keys available in the kv store):

[]string{"", ""}

These host components that I am trying out are implemented as part of fermyon/spin

e.g. variables

and

kv store

rajatjindal commented 1 week ago

I will be trying out the other suggestions and will report back the result 👍

rajatjindal commented 1 week ago

on trying to print stringdata, I get following:

stringdata is (*uint8)(0x20)

^^ is the output of following code:

// Get an application variable value for the current component.
//
// The name must match one defined in in the component manifest.
func Get(key string) (string, error) {
    result := variables.Get(key)
    if result.IsErr() {
        return "", errorVariantToError(*result.Err())
    }

    val := result.OK()

    //check the value of the unsafe.StringData pointer of the returned string
    stringdata := unsafe.StringData(*val)

    stdout.GetStdout().Write(cm.ToList([]byte(fmt.Sprintf("stringdata is %#v \n", stringdata))))
    return *result.OK(), nil
}

variables.Get is the generated code:

//go:nosplit
func Get(name string) (result cm.ErrResult[string, Error]) {
    name0, name1 := cm.LowerString(name)
    wasmimport_Get((*uint8)(name0), (uint32)(name1), &result)
    return
}

//go:wasmimport fermyon:spin/variables@2.0.0 get
//go:noescape
func wasmimport_Get(name0 *uint8, name1 uint32, result *cm.ErrResult[string, Error])
rajatjindal commented 1 week ago

I have also created a small repro repo for this: https://github.com/rajatjindal/wasip2-string

it needs to run using spin (I can try to get a reproducible scenario with wasmtime if you think that would be better).

ydnar commented 1 week ago

Thanks. I assume this is run with the wasip2 fork of TinyGo?

rajatjindal commented 1 week ago

yes, that is right. I updated the repo in last day or so.

ydnar commented 1 week ago

I have also created a small repro repo for this: https://github.com/rajatjindal/wasip2-string

it needs to run using spin (I can try to get a reproducible scenario with wasmtime if you think that would be better).

Possible to do one that works with wasmtime? Just to rule out the additional software?

rajatjindal commented 1 week ago

Hi Randy, I am trying to reproduce the issue with wasmtime, but unable to do so far.

The repo is here: https://github.com/rajatjindal/wasip2-string-wasmtime

this uses environment host component, but this returns the string correctly as expected.

I am trying to narrow down to a testcase that we can verify with both.

ydnar commented 1 week ago

Does environment work in spin?

rajatjindal commented 1 week ago

i was able to reproduce this issue with wasmtime. I've updated the repo with the scenario: https://github.com/rajatjindal/wasip2-string-wasmtime

need to use a fork that implements additional functions in environment host to demonstrate the problem.

ydnar commented 1 week ago

Do you have a working client for this WIT file in any language?

rajatjindal commented 1 week ago

this is a repo I created for running similar code using rust:

https://github.com/rajatjindal/wasip2-string-rust

I hope this helps

ydnar commented 1 week ago

Thanks for the reproduction repos, they’ve been a ton of help.

I’ve verified that the host successfully passes the string in the result<string, error>. The issue seems to be when the generated function returns ErrResult[string, Error] by value on the stack.

It seems to be mangled. If I make it store the data field as a byte slice, it works.

More work tomorrow.

rajatjindal commented 1 week ago

if its of any help, the bindings in the rust repo I shared has useful info on how that lifting is done there. (I found it useful as I am not very familiar with canonical abi spec as such)

ydnar commented 1 week ago

No lifting occurs in this case. The component calls the imported function with an outparam pointer to the result.

It works fine to that point (you can edit and log the OK value inside the Get function.

I believe I may have made an incorrect assumption about how Go passes structs on the stack, and may need to revise the shape type for results and variants.

ydnar commented 1 week ago

Give https://github.com/ydnar/wasm-tools-go/pull/121 a shot!

rajatjindal commented 1 week ago

YES IT WORKS :)

I will try out a few other things, and will report back how that goes

ydnar commented 1 week ago

One thing I noticed debugging this: Spin doesn't support blocking write and flush, so panic, print, and println do not work. Nor does fmt.Print*.

This seems like an obvious addition to the Spin API surface, particularly for debugging.