coder-mike / microvium

A compact, embeddable scripting engine for applications and microcontrollers for executing programs written in a subset of the JavaScript language.
MIT License
569 stars 25 forks source link

MVM_VERY_EXPENSIVE_MEMORY_CHECKS yields unexpected behaviour #47

Closed TobyEalden closed 1 year ago

TobyEalden commented 1 year ago

Sorry to bombard you with this stuff - this is not a priority but I thought I'd mention it.

I've been playing with the options in the port file and when I set MVM_VERY_EXPENSIVE_MEMORY_CHECKS it breaks things.

I have a command function that has the generated wrapper code below. If I call it with command "fib" and payload "50" at the point of entry to mvm_call the args contains the correct values.

void AppMVM::command(std::string command, std::string payload) {
  // Prepare the arguments
  mvm_Value _args[2];
  _args[0] = mvm_newString(_vm, command.c_str(), command.size());
  _args[1] = mvm_newString(_vm, payload.c_str(), payload.size());
  mvm_Value _result;

  // Call the JS function
  mvm_TeError err = mvm_call(_vm, _vmExports[1], &_result, _args, 2);
  if (err != MVM_E_SUCCESS) MVM_FATAL_ERROR(_vm, err);

  // Convert/return the result
  return ;
}

However, in the implementation below, the console.log statements print got command: 50 and with payload: 50, i.e. both arguments have the same value.

function mvm_command(command, payload) {
    console.log("got command " + command);
    console.log("with payload: " + payload);

    if (command === "fib") {
        emitAck(command);
        fibonacci(command, payload);
    } else {
        emitError(command, "unknown command: " + command);
    }
}

With MVM_VERY_EXPENSIVE_MEMORY_CHECKS not set, I get the expected results.

coder-mike commented 1 year ago

Sorry to bombard you with this stuff - this is not a priority but I thought I'd mention it.

Nah that's cool. Everything you're raising is helping to harden Microvium, which is great!

This is an interesting one. I'll take a look.

Btw I like your idea of having the native implementations inside a class derived from the App. It's a clean way to get access to the app without it being an explicit parameter.

coder-mike commented 1 year ago

Hi Toby. Is this example representative of the program where you're seeing the problem? When I run this on my side, I don't see any issue. I did change here the payload to be Any because fib takes a number rather than a string. I'm not sure if you're doing a conversion somewhere.

TobyEalden commented 1 year ago

Thanks for this! It does indeed look similar to what I'm trying to do. I'll take a look and get back to you.

boogie commented 1 year ago

I have a similar issue. When MVM_VERY_EXPENSIVE_MEMORY_CHECKS is enabled, then mvm_newString returns with the same integer.

mvm_Value argValue = mvm_newString(vm, value, strlen(value));
mvm_Value argType = mvm_newString(vm, type, strlen(type));
mvm_Value argSource = mvm_newString(vm, source, strlen(source));

In this case argValue, argType and argSource have the same value (12 for me). They are becoming different (12, 20, 28) when I turn off MVM_VERY_EXPENSIVE_MEMORY_CHECKS.

coder-mike commented 1 year ago

Hi @boogie. What's happening in your case is that your variables argValue, argType, and argSource are not reachable by the Microvium garbage collector. And since there are no JavaScript references to those strings in the VM, and no handles (mvm_Handle) referencing them, they are elligable for garbage collection. The MVM_VERY_EXPENSIVE_MEMORY_CHECKS flag exposes this usage mistake by forcing a garbage collection cycle before each allocation, so the argValue string is garbage collected before the argType string is allocated, thus allowing the same memory address (in this case address 12) to be re-used each time.

The solution in a case like this is to use handles, which is a way of telling Microvium about references that C has to VM heap allocations, as in the following example:

  mvm_Handle argValue;
  mvm_Handle argType;
  mvm_Handle argSource;

  mvm_initializeHandle(vm, &argValue);
  mvm_initializeHandle(vm, &argType);
  mvm_initializeHandle(vm, &argSource);

  mvm_handleSet(&argValue, mvm_newString(vm, value, strlen(value)));
  mvm_handleSet(&argType, mvm_newString(vm, type, strlen(type)));
  mvm_handleSet(&argSource, mvm_newString(vm, source, strlen(source)));

  // ... use the values (with mvm_handleGet) ...

  // When finished, release the handles to allow those strings to be collected
  mvm_releaseHandle(vm, &argSource);
  mvm_releaseHandle(vm, &argType);
  mvm_releaseHandle(vm, &argValue);

I'll add a warning in the documentation of mvm_newString and similar functions to say that handles should be used.

coder-mike commented 1 year ago

I've now added some docs around the use of handles, and warnings on all the API functions that return values that may need to be wrapped in a handle. https://github.com/coder-mike/microvium/blob/main/doc/handles-and-garbage-collection.md