nesbox / TIC-80

TIC-80 is a fantasy computer for making, playing and sharing tiny games.
https://tic80.com
MIT License
5.04k stars 490 forks source link

WASM API functions should not return C `bool` #2183

Open gefjon opened 1 year ago

gefjon commented 1 year ago

I'm running TIC-80 1.0.2164 on an m1 MacBook Air, and looking at the sources for the most recent commit. I checked git blame, and it doesn't look like any relevant functions have changed between my build and the source I'm looking at.

I'm building a game in Rust WASM. I'm using the Rust template from the repository, but I cranked the optimizations up to opt-level = 3, because I don't intend to access VRAM directly.

I noticed some unexpected and unpredictable behavior when calling btnp. (I was always calling exactly btnp(7, -1, -1), but I don't think it matters.) Depending on surrounding code, debug vs release build, and the phase of the moon, branches on btnp would sometimes be always-taken, whether or not the button in question was pressed. Printing the result of the btnp with trace would always print the result I expected. That screamed undefined behavior.

Digging into rustc/llvm's codegen, I found that it was representing bool as i32, since WASM doesn't have 8-bit integers, and that it was sometimes emitting several different encodings of branches on the result of btnp, which had inconsistent handling of values other than 0 or 1. Sometimes it would & the low bit with 1, sometimes it would use i32.eqz, and sometimes it would just use the return value directly. This is very much within rustc's purview; it reserves the right to assume that all booleans are either 0 or 1.

I changed the result type of tic80::sys::btnp to u32, and printed its result, like:

debug_ln!("BTNP(7, -1, -1) returned: {:x}", unsafe { tic80::sys::btnp(7, -1, -1) });

This printed:

>BTNP(7, -1, -1) returned: 3de00 // with unpressed
>BTNP(7, -1, -1) returned: 3de01 // with pressed
>BTNP(7, -1, -1) returned: 3de00 // with unpressed

I looked into the definition of wasmtic_btnp, which is:

m3ApiRawFunction(wasmtic_btnp)
{
    m3ApiReturnType  (bool)

    // Bunch of stuff that isn't relevant.

    m3ApiReturn(tic_api_btnp((tic_mem *)core, index, hold, period));

    // I think this call is superfluous, but whatever.
    m3ApiSuccess();
}

That got me suspicious, since WASM doesn't have bool, so how can WASM3 have them?

The relevant macros are defined:

# define m3ApiReturnType(TYPE)      TYPE* raw_return = ((TYPE*) (_sp++));
# define m3ApiReturn(VALUE)         { *raw_return = (VALUE); return m3Err_none; }

Note that _sp is a uint64_t *.

So, as far as I can tell, what's happening is that wasmtic_btnp:

  1. Reserves a uint64_t on the stack to hold its return value.
  2. Does some stuff to compute its return value.
  3. Writes one byte of its return value into the reserved space, leaving the high seven bytes untouched.
  4. Tries to return an i32, so it reads 4 bytes out of the reserved space, one of which is the proper boolean return value, and three of which are (locally) uninitialized.

TL;DR:

WASM API functions which want to return a boolean should use int32_t as their return type, not bool. Care must also be taken that the returned value is exactly either 0 or 1, assuming consumer libraries want to treat the functions as returning bool the way the Rust template defines btnp.

Dreagonmon commented 8 months ago

Using clang without wasi-sdk, compiling C to wasm, I got the same problems.

After changing the return type to int32_t, and & 0b1, it works temporarily as expected.

WASM_IMPORT("btnp")
int32_t btnp(int32_t index, int32_t hold, int32_t period);

// somewhere else
if (btnp(BUTTON_CODE_P1_LEFT, TIC80_PARAM_IGNORE, TIC80_PARAM_IGNORE) & 0b1)
{
   //  ...
}

(maybe I should read the GAMEPAD memory directly, it is more stable)