api/wasm: writeToStorage defaults to true, update wasmtic_pmem signature from i(ii) to i(iI)

[peterhellberg]

I noticed two issues in src/api/wasm.c when trying to call pmem from a small Zig ⚡ program (compiling to .wasm)

• writeToStorage was never set to true (unless I've misunderstood the code that is)
• The signature of wasmtic_pmem indicated that the arguments would be three 32 bit values: i(ii), and not two 32 bit (return value and address) + one 64 bit value (for value) as I expected: i(iI)

Related to issue #2360

[joshgoebel]

Wait, why is value a 64 bit number - pmem stores and retrieves 32 bit numbers... it seems like the C code is correct and the Zig API needs to change, no?

[peterhellberg]

Wait, why is value a 64 bit number - pmem stores and retrieves 32 bit numbers... it seems like the C code is correct and the Zig API needs to change, no?

Because a i32 can not represent all u32 numbers + -1, I would personally be fine to have

TIC-80/src/api/wasm.c

Line 578 in 5061bb2

m3ApiGetArg (int64_t, value)

be an int32_t, but I suspect others might need to be able to represent all uint32_t values.

Note

It is the C code that specifies that it should be 64 bits.. due to not having a boolean for if a write should happen or not. Which in turn naturally forces the Zig API to pass in a i64 as the value.. but since there (If I understand it correctly, is a bug in how the signature is specified in wasm.c it will not match the signature, and end up in function signature mismatch unless wasmtic_pmem signature is changed to i(iI))

Also note that it is wasmtic_pmem we are talking about here, not tic_api_pmem
(where value is a u32, so no change in that api)

The current template for C imports pmem like this:

TIC-80/templates/c/src/tic80.h

Lines 321 to 323 in 5061bb2

	WASM_IMPORT("pmem")
	// Access or update the persistent memory.
	uint32_t pmem(int32_t address, int64_t value);

which results in Segmentation fault (core dumped) when calling pmem (Both with -1 as the value and other values)

With a tic80 binary compiled with the corrected wasmtic_pmem signature (i(iI)) it no longer segfaults, and pmem(1, pmem(1,-1)+1); work as expected (as long as the other bug has been corrected as well, that writeToStorage wasn't set to true)

[peterhellberg]

@joshgoebel We can probably disregard that I used Zig when building a .wasm, the same problem can be reproduced using C

#include "tic80.h"

WASM_IMPORT("snprintf")
int snprintf(char* s, size_t n, const char* format, ...);

WASM_EXPORT("BOOT")
void BOOT() {
    pmem(1, pmem(1,-1)+1);
}

WASM_EXPORT("TIC")
void TIC() {
    cls(13);

    char buf[20];
    snprintf(buf, 20, "Started %d times", pmem(1,-1));
    print(buf, 3, 3, 15, 0, 1, 1);
}

Before this fix this results in function signature mismatch and then Segmentation fault (core dumped) in tic80 when started like this:

tic80 --fs . --cli --cmd 'load wasmdemo.wasmp & import binary build/cart.wasm & save game.tic & exit'
tic80 --fs . --cmd 'load game.tic & run & exit'

[joshgoebel]

The goal here is to always maintain backwards compatibility. If someone is using the existing API, that should continue to work (bugs and all). I think the behavior here should be documented and one of two things happen:

• Introduce a new function with the signature in the name: pmem_i_iI (or some such)... libraries can be modified to link against this without breaking backwards compatibility with pmem
• client libraries could also choose to reimplement pmem using peek and poke.

[peterhellberg]

@joshgoebel This wouldn't break any compatibility though? (since the function is currently broken in TIC-80, not sure that it has ever worked)

(We are only talking about the "pmem" that is exported in WASM, wasmtic_pmem)

This is strictly a bugfix. (Of two separate bugs: The first that it was impossible to write due to writeToStorage always being false, and the second being that the m3ApiRawFunction(wasmtic_pmem) function arguments and the signature didn't match, causing "function signature mismatch" in TIC-80 when loading a wasm cart that is calling pmem)

[peterhellberg]

Looked through the repo history and these two bugs have been present in src/api/wasm.c since aeabb55, so if I'm not mistaken there are no existing users of this functionality (calling pmem from wasm)

[joshgoebel]

since the function is currently broken in TIC-80, not sure that it has ever worked)

A valid signature itself can't be "broken". It might be buggy but if anyone has ever successfully linked against it, then changing it breaks their code.

You're saying our shipping Zig library breaks out of the box, yes?
We'd need to confirm that every other library (every WASM target) is also broken in the same way.

Even that would be a compromise since it's quite possible to write WASM by hand. And also possible that individual users have patched their libraries to work with the existing buggy API - and just never reported back here.

[joshgoebel]

Correct me if I'm wrong but if linked correctly the existing binary API works, just limits you to 32 bit signed numbers other than -1.

[peterhellberg]

Disregard Zig for now, it is broken from C as well (as I noted above)

Due to

TIC-80/templates/c/src/tic80.h

Line 323 in 5061bb2

uint32_t pmem(int32_t address, int64_t value);

not matching what is exported in wasm3

(And changing value to a int32_t in tic80.h does not solve the issue of src/api/wasm.c specifying the value argument being int64_t and then exporting it as i32)

[peterhellberg]

A valid signature itself can't be "broken". It might be buggy but if anyone has ever successfully linked against it, then changing it breaks their code.

I believe my point is that it isn't valid. (to have a function with 128 bits of arguments be exported as only having 96) But maybe I've missed something fundamental in why this isn't working.

[peterhellberg]

Correct me if I'm wrong but if linked correctly the existing binary API works, just limits you to 32 bit signed numbers other than -1.

No, I believe it to never have worked to call the exported pmem function from a wasm binary, regardless of language the binary was compiled from.

Do you think otherwise?

[peterhellberg]

Another way to "fix" the problem of not being able to call the exported pmem function from wasm in TIC-80 without changing the signature would be to reduce the size of the value argument (but I think that would be a worse solution than to correct the signature to match the arguments to wasmtic_pmem)

diff --git a/src/api/wasm.c b/src/api/wasm.c
index 5dfd20f3..f6142536 100644
--- a/src/api/wasm.c
+++ b/src/api/wasm.c
@@ -575,8 +575,8 @@ m3ApiRawFunction(wasmtic_pmem)
     m3ApiReturnType  (uint32_t)
 
     m3ApiGetArg      (int32_t, address)
-    m3ApiGetArg      (int64_t, value)
-    bool writeToStorage;
+    m3ApiGetArg      (int32_t, value)
+    bool writeToStorage = true;
 
     tic_mem* tic = (tic_mem*)getWasmCore(runtime);

[peterhellberg]

The same issues can be seen with code written in D:

import tic80;

extern(C):

int  snprintf(scope char* s, size_t n, scope const char* format, scope const ...);

void BOOT() {
  pmem(1,cast(int)pmem(1,-1)+1);
}

void TIC() {
  cls(13);

  char[20] buf;
  char* bufptr = cast(char*)buf;
  snprintf(bufptr, 20, "Started %d times", pmem(1,-1));
  print(bufptr, 3, 3, 15, 0, 1, 1);
}

With the fix in this PR, and this change to src/tic80.d: uint pmem(uint index, uint value); 🡆 uint pmem(int index, long value); the cart does what it is supposed to.