lib/utils/pyexec: improve pyexec so it can be used by unix (WIP)

[dpgeorge]

Background: unix has its own custom REPL loop and parse-compile-execute function, while bare-metal ports all use the one provided by lib/utils/pyexec.c.

This set of commits aims to improve and factor the existing code in lib/utils/pyexec.c so that it can be used by the unix port. It adds a few optional features to pyexec.c and makes some functions there public, including some flags.

The final commit here then updates the unix port to use the new pyexec.c code, which is mostly just deleting things from unix's main.c.

This is still a WIP because it's not fully tested and there are some subtle changes, including to the behaviour of SystemExit to try and make the code usable by unix and bare-metal (eg on bare-metal the argument of SystemExit exceptions is now interpreted and used as the return value of the pyexec.c functions).

[stinos]

This would be good: for embedding now I use a combination of copying a bunch of functionality from unix/main.c (mostly the initialization phase, setting up heap/stack/path etc) and making some functions non-static (execute_from_lexer and handle_uncaught_exception) so would be welcome to have some of these available from the API. The only thing missing is that I use a custom exception handler: the unhandled exception object should be available elsewhere so it can be logged in a custom way and sent to another application to notify it of the reason of failure including stacktrace. Probably I once made a PR for that which wasn't accepted, but with these changes I think it would be beneficial since all or most ports could use it.

[dpgeorge]

The only thing missing is that I use a custom exception handler: the unhandled exception object should be available elsewhere so it can be logged in a custom way and sent to another application to notify it of the reason of failure including stacktrace.

Would it work for you to allow pyexec_handle_uncaught_exception to be overridden? Or does it need more than that?

[stinos]

I think that would work yes, just via the preprocessor, but I'd still have to copy a bit of logic for handling SystemExit but that's not a real problem. Although it could also be part of the API.

[stinos]

On second thought, something like this would still be more convenient because I could just use it as-is:

//In pyexec.h
typedef int(*mp_handle_exception_fun_t)(void *data, mp_obj_t ex);

typedef struct _mp_handle_exception_t {
    void *data;
    mp_handle_exception_fun_t handle;
} mp_handle_exception_t;

extern struct _mp_handle_exception_t mp_handle_exception;

//in pyexec.c
int handle_uncaught_exception(mp_obj_base_t *exc) {
    ...
    //original SystemExit handling here
    ...
    // Report all other exceptions
    if (mp_handle_exception.handle) {
        return mp_handle_exception.handle(mp_handle_exception.data, MP_OBJ_FROM_PTR(exc));
    }
    return 1;
}

//default implementation in pyexec.c or per port
STATIC int print_exception(void *data, mp_obj_t exc) {
    (void)data;
    mp_obj_print_exception(&mp_stderr_print, exc);
    return 1
}

mp_handle_exception_t mp_handle_exception = {NULL, print_exception};

[dlech]

+1 for being able to hook the unhandled exception handler. We have a use case for that as well. Could live without the extra .data field though.

[stinos]

The data field is the to allow injecting code without having to use global state

[tve]

and PYEXEC_FLAG_SOURCE_IS_RAW_CODE, PYEXEC_FLAG_SOURCE_IS_VSTR, PYEXEC_FLAG_SOURCE_IS_FILENAME, PYEXEC_FLAG_SOURCE_IS_FD, PYEXEC_FLAG_COMPILE_ONLY ?

[dpgeorge]

ok

[tve]

what does this change?

[dpgeorge]

it's needed only by the unix port to switch to/from raw console mode

[dpgeorge]

The data field is the to allow injecting code without having to use global state

But mp_handle_exception is itself a global struct, no?

Wouldn't it be simpler and still as powerful to just have the ability to provide a general function to call, eg:

int handle_uncaught_exception(mp_obj_base_t *exc) {
    ...
    // original SystemExit handling here
    ...
    // Report all other exceptions
    #ifdef MICROPY_REPORT_UNHANDLED_EXCEPTION
    return MICROPY_REPORT_UNHANDLED_EXCEPTION(MP_OBJ_FROM_PTR(exc));
    #else 
    // existing handler that printing the exception to stderr
    #endif
}

[stinos]

But mp_handle_exception is itself a global struct, no?

Yes but that offloads having to deal with it to MicroPython.

Wouldn't it be simpler and still as powerful to just have the ability to provide a general function to call

It's simpler to implement on the MicroPython side for sure, but it's a bit less easy to just use as-is (at the price of code size as usual, but could be conditional of course) since it requires the same boilerplate to make it generally useful. For example with the code like I showed I'm using something like this (not the best example:

void SomeClass::DoSomethingWhileUsingCustomLogging() {
  mp_handle_exception_t logExceptions{ &logger, [] (void* data, mp_obj_t ex) {
    (reinterpret_cast<CustomLogger*>)->Log(ExceptionToString(ex));
    return 1;
  } };
  mp_handle_exception = logExceptions;
  //...
  //For the next part we cannot do logging for some reason
  mp_handle_exception_t dontLogExceptions{ nullptr, [] (void*, mp_obj_t) {
    return 2;
  } };
  mp_handle_exception = dontLogExceptions;
  //...
  //Restore default exception handling now.
}

whereas with your example I'd first have to make a change at the build level like

make MICROPY_REPORT_UNHANDLED_EXCEPTION=MyExHandler

and then implement the rest manually:

typedef int(*mp_handle_exception_fun_t)(void *data, mp_obj_t ex);

typedef struct _mp_handle_exception_t {
    void *data;
    mp_handle_exception_fun_t handle;
} mp_handle_exception_t;

struct _mp_handle_exception_t mp_handle_exception;

int MyExHandler(mp_obj_t ex) {
  if(mp_handle_exception.handle) {
    return mp_handle_exception.handle(mp_handle_exception.data);
  }
  return 1;
}

//Now we can use DoSomethingWhileUsingCustomLogging() like above

So it's not like MICROPY_REPORT_UNHANDLED_EXCEPTION doesn't work, it just requires some extra patchwork. But I completely get it if you find that too specific and not generally useful enough.

[projectgus]

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.