Define types and functions in the proposal's template

[loganek]

This for now just defines types and has enigmatic documentation. The purpose of this PR is to reach agreement on type definitions:

• I used result as a return type for the function - that feels more idiomatic, although I presume that changes the C interface, so I'm curious to know what others think
• I defined errno enum, which regardless of the decision about the point above, we might still keep to define types of errors being returned by the function. For now there's only eagain, but consider it as a placeholder. The point of this PR is not to define error codes, but to decide whether we want them as part of the public API or should we rather stick to binary response. IMO it feels reasonable to have them and we can define (better) set of errors than the ones in POSIX
• because the return type of the function is result, thread-id can be an unsigned integer now, but the documentation should mention the reserved bits (or min/max value for thread id).

[abrown]

Yeah, I think this is good! We can further document and modify the WIT as we go along but I think this is a start as it helps make the current structure of the API visible.

[abrown]

Suggested change

	It's goal is to provide functions that allow implementation of a subset of `pthreads` API, but it doesn't aim to be 100% compatible with POSIX threads standard.
	Its goal is to provide functions that allow implementation of a subset of `pthreads` API, but it doesn't aim to be 100% compatible with POSIX threads standard.

[yamt]

can wit represent what we have README.md?
my impression is it can't.
iirc, wit is a bit more abstracted/higher level than what eg. wasi snapshot preview1 uses.

[yamt]

isn't wasi errno 16-bit?

[loganek]

This isn't the same as wasi's errno. Here we only define a subset of errors that can be returned by the function

[yamt]

IMO it's confusing to have a wit definition which is incompatible with README.md

i'd suggest to use a separate PRs for api/abi changes and introduction of a wit definition.

[loganek]

i'd suggest to use a separate PRs for api/abi changes and introduction of a wit definition.

This PR only changes WIT definitions. I don't think it's a problem for now to keep it like that; interfaces defined in README are somehow conceptual anyways, but I'm aiming to update them as well as soon as I confirm a couple of things related to WIT.

[sunfishcode]

Since this proposal is currently using the instance-per-thread model, Wit isn't able to correctly describe it. thread-spawn creates a new thread that implicitly captures the memory and imports of the calling thread, which is not something that Wit APIs can do. I suggest at least adding a comment about this.

[abrown]

I'm still fine with this, especially now that start-arg is no longer a resource. It describes what WIT can describe and, as @sunfishcode notes, there are things that WIT cannot describe. I do think it would be helpful to duplicate the "wasi-threads requires a wasi_thread_start export" documentation here but it could be a small note and a link to the README.

[yamt]

which version of wit-bindgen (or other tools) can process this definition?
i want to know the exact core wasm level abi corresponding to this definition.

[yamt]

i played a bit with wit-bindgen.

with this wit definition, do you mean to switch to the following core wasm abi?

// Unique thread identifier.
typedef uint32_t wasi_thread_id_t;

// A reference to data passed to the start function (`wasi_thread_start()`) called by the newly spawned thread.
typedef uint32_t wasi_start_arg_t;

// Error codes returned by the `thread-spawn` function.
typedef uint8_t wasi_errno_t;

#define WASI_ERRNO_EAGAIN 0

__attribute__((import_module("wasi"), import_name("thread-spawn")))
void __wasm_import_wasi_thread_spawn(int32_t, int32_t);

bool wasi_thread_spawn(wasi_start_arg_t start_arg, wasi_thread_id_t *ret, wasi_errno_t *err) {
  __attribute__((aligned(4)))
  uint8_t ret_area[8];
  int32_t ptr = (int32_t) &ret_area;
  __wasm_import_wasi_thread_spawn((int32_t) (start_arg), ptr);
  wasi_result_thread_id_errno_t result;
  switch ((int32_t) (*((uint8_t*) (ptr + 0)))) {
    case 0: {
      result.is_err = false;
      result.val.ok = (uint32_t) (*((int32_t*) (ptr + 4)));
      break;
    }
    case 1: {
      result.is_err = true;
      result.val.err = (int32_t) (*((uint8_t*) (ptr + 4)));
      break;
    }
  }
  if (!result.is_err) {
    *ret = result.val.ok;
    return 1;
  } else {
    *err = result.val.err;
    return 0;
  }
}

[yamt]

i prefer to stick with a simple i32->i32 type at least for now.

[yamt]

i prefer to stick with a simple i32->i32 type at least for now.

i mean something like this:

/// Unique thread identifier or negative WASI errno.
type thread-id-or-negative-wasi-errno = s32

/// Creates a new thread.
thread-spawn: func(
    /// A value being passed to a start function (`wasi_thread_start()`).
    start-arg: start-arg,
) -> thread-id-or-negative-wasi-errno

btw, do you intend to use thread-spawn (with hyphen) for core wasm import?

[abrown]

Ah, good catch. I think we do just mean s32 here (at least that is how I implemented things in Wasmtime). I don't know if we really need the new type but a doc comment indicating that negative return values indicate an error would be helpful.

As for what wit-bindgen version to use, it seems like we should target the latest published version available as a CI action.

[loganek]

Yes, I was aware of this change, that's why I highlighted it in the description. I know that wasi-libc, WAMR, and (I guess) wasmtime implement this method with thread-id-or-negative-wasi-errno. But I proposed using result instead as this is exactly what we want to achieve, and using negative value to indicate error looks like a workaround.

I'm ok with switching it back to i32 here, but I'd rather prefer going with a long-term solution (which I believe is using result) and update all the places that already implement this function.

[yamt]

@loganek
is this the right understanding of the new abi you are proposing here?
WebAssembly/wasi-libc#385

[loganek]

Yes, that seems to match the generated output in https://bytecodealliance.github.io/wit-bindgen/ when passing the definition:

interface host-funcs {
  type start-arg = u32
  type thread-id = u32
  enum errno {
    /// TBD
    eagain,
  }
  thread-spawn: func(
    /// A value being passed to a start function (`wasi_thread_start()`).
    start-arg: start-arg,
  ) -> result<thread-id, errno>
}

world demo {
  import host: host-funcs 
}

The only question though is how stable the ABI is, hopefully I'll have answer for that soon.

[abrown]

@loganek, re: your comment up above:

I'm ok with switching it back to i32 here, but I'd rather prefer going with a long-term solution (which I believe is using result) and update all the places that already implement this function.

I don't care too much either way: sure, result may be more semantically clear. I do lean towards s32 since it is a bit simpler ABI-wise, but that is not too important. What I do think is important is that we just merge this to not get bogged down with that detail. I propose we merge this PR with s32 and some doc note about "negative numbers are error codes" and then open an issue to switch to result in which we list out all the places we will need to update things: wasi-libc, Wasmtime, WAMR, etc.

What do you think of that?

[loganek]

Yep, I don't want to keep this PR open forever so let's have a version that's compatible with current proposal; I'll open an issue and a PR for the follow-up discussion.

[loganek]

Create a follow-up issue: #29