Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
MaartenS11 opened this issue Nov 5, 2023 · 1 comment
Labels
feature Minor new feature or request

Comments

@MaartenS11
Copy link
Member

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);
}
@MaartenS11 MaartenS11 added the invalid This doesn't seem right label Nov 5, 2023
@tolauwae
Copy link
Member

tolauwae commented Nov 9, 2023

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

@tolauwae tolauwae added feature Minor new feature or request and removed invalid This doesn't seem right labels Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Minor new feature or request
Projects
Status: No status
Development

No branches or pull requests

2 participants