TOPLLab / WARDuino

📟 A dynamic WebAssembly VM for embedded systems
https://topllab.github.io/WARDuino/
Mozilla Public License 2.0
72 stars 7 forks source link

Return value of run_module depends on the implementation of the module #217

Open MaartenS11 opened 8 months ago

MaartenS11 commented 8 months ago

The run_module method in WARDuino.cpp is a bit strange to use.

int WARDuino::run_module(Module *m) {
    uint32_t fidx = this->get_main_fidx(m);

    // execute main
    if (fidx != UNDEF) {
        this->invoke(m, fidx);
        return m->stack[0].value.uint32;
    }

    // wait
    m->warduino->debugger->pauseRuntime(m);
    return interpret(m, true);
}

If the module you run happens to have a main function then it appears to want to return the result of executing that function. However, if the module your run doesn't have a main function it will return the result of calling the interpret function. This function returns true if successful and false otherwise. So the return value is either a result of executing a wasm function or something that indicates if the module executed successfully which makes it hard to use as a programmer. If I want to make something that does something if the execution fails then I can't use this function reliably because if the module has a main method then it won't tell me if it failed or not. If the module doesn't have a main method then it does work.

So it basically sometimes returns success of the application (based on the exit code) and sometimes success as in it was able to fully execute everything without any issues. An application can return exit code 1 but that doesn't mean there was an issue executing the module, it means there was an error and the application safely took care of it.

On a further note m->stack[0].value.uint32 is also not really correct for all functions. If I invoke a function that has a local for example and the execution fails then it will return the value of the local instead of the value that was actually returned from the function. So m->stack[m->sp].value.uint32 would be more correct.

Proposed solution

Just return what invoke returns so this function consistently just returns a boolean that indicates success in terms of execution. If you want to get the returned value of the main method then the caller can just look what is on the stack after running run_module.

bool WARDuino::run_module(Module *m) {
    uint32_t fidx = this->get_main_fidx(m);

    // execute main
    if (fidx != UNDEF) {
        return this->invoke(m, fidx);
    }

    // wait
    m->warduino->debugger->pauseRuntime(m);
    return interpret(m, true);
}
tolauwae commented 7 months ago

This looks like a good change. This can be implemented on its own branch and merged to main.