Experiment: Change Pony errors to use return flags (internally) rather than exceptions

[jemc]

We are looking at maybe changing the way that Pony errors work internally. Specifically, we are looking at using return flags (internally) rather than exceptions and LLVM invokes.

These changes would be almost entirely invisible to Pony users except in the following ways:

• The pony_error C function would be removed, and any C code which currently uses it must be refactored use another approach to convey failures to the caller.

  • implied by this: FFI functions would no longer be allowed to be partial, because the only supported kind of partial function is one which uses pony_error to raise a Pony-specific exceptions (C++ exceptions are already not supported today)
  • this is technically a breaking change (though only for C code written for use with Pony), so it must go through the RFC process.

• Performance may be impacted (for better and/or worse, depending on the application.

  • using a return flag for errors, puts the "did raise an error" case and "didn't raise an error" case on equal footing performance-wise, and it is up to CPU branch prediction (and/or likely/unlikely annotations) to decide to favor one case over the other
  • this means that the error-related overhead for a calling a function which may raise an error, and is not inlined during compilation, may be:
    • slightly slower in the "didn't raise an error" case
    • much faster in the "did raise an error" case
  • implied by this: the current advice in the performance cheat sheet about avoiding raising error in performance-critical code would likely be removed (and possibly replaced by new advice)
  • performance testing should/will be done to give more details on the impact.

[jemc]

Note that I have already done this experiment in the Savi compiler, so I am familiar with the areas of code that need to change in the Pony compiler to make this work.

Here are my raw notes that I wrtoe up in a call with @SeanTAllen today where we went over what needs to be done:

• get rid of pony_error
• get rid of the ability for an FFI call to be partial
  • the invoke_fun branch of gen_ffi function goes away
• refactor the FFI stuff that is currently using pony_error
  • in socket.c, we can probably just replace the pony_error calls with return -1
  • in time.c, just don't said the Windows invalid param handler (says Sean, after reading the Windows docs)
• replace gencall_error with a either a jump to the nearest try block's else, or an "error return" if there is no catching try block in the local scope
  • an "error return" is basically a tuple return where the first element is the "real" return type, and the second element is an int1 (a.k.a. boolean) which represents "error or no error"
• rename invoke_target in compile_frame_t type since we're no longer using LLVM invoke
• in the gen_call function, the logic around invoke_fun needs to change:
  • if ast_canerror is false, we do codegen_call as normal
  • if ast_canerror is true, we need to destructure the return value to get the error flag
    • in the "error flag says it did not error" branch that we generate, we just get the other tuple value as the true return, and that's the expression result
    • in the "error flag says it did error" branch that we generate, we check if there's an invoke_target in the current frame
      • if there is an invoke_target, jump to that target block (with LLVMBuildBr or similar)
      • if there is no invoke_target, do an "error return"
• a few places that currently do LLVMBuildRet/LLVMBuildRetVoid directly need to call gen_return (or a similar function) so that they can do separate handling of partial function return structure vs non-partial
  • genfun_fun function in genfun.c
  • genfun_new function in genfun.c
• gen_return (or a similar function) needs to get revamped to be able to do an error-flag-including return type for partial functions
  • you can use LLVM's undef for the undefined return value in an "error return"
• make_signature in genfun.c needs to have a special case for error-flag-including return type for partial functions
• probably a lot of stuff breaks indirectly in codegen for code that is relying on getting the return type of a function and using that to do something - all such cases need to account for the possibility that they need to dig into the error-flag-including tuple to get the "real" return type when necessary

[SeanTAllen]

When getting rid of pony_error(). There are a few runtime functions that need to change. I will be updating this comment with notes on them.

time.c: ponyint_formattime

The windows version uses format_invalid_parameter_handler to set up an invalid format handler which will do a pony_error. Instead of doing pony_error, we can consider returning an empty string. We already return empty string for "potentially problematic format codes":

  // Bail out on strftime formats that can produce a zero-length string.
  if((fmt[0] == '\0') || !strcmp(fmt, "%p") || !strcmp(fmt, "%P"))
  {
    buffer = (char*)pony_alloc(ctx, 1);
    buffer[0] = '\0';
    return buffer;
  }

On Windows, instead of using the handler function that has a void return type, we can (on windows only) check if len is 0 after calling strftime and if it is AND errno is EINVAL then we bail because we weren't able to validate all the parameters to strftime.

This would make ponyint_formattime not partial and we should update its FFI definition in posix_date.pony.

When can consider leaving format on PosixDate partial and having it error if the returned string is empty.

socket.c: multiple functions

We have two options.

1. Use -1 to indicate "unrecoverable error"
2. Return a struct that includes "len" etc value that we are currently conveying. And an error value that we set to the error code if we get an error. In the case of an error, we make sure "len" is 0 and set the correct error code. For success, we make sure error code is 0. It is then the responsibility of calling code to handle each of the error types.

Although it is more work, I am leaning towards 2 being a better option.

[IgorDeepakM]

I suggest an intermediary step. Add a TLS variable in order to indicate an error, basically an errno like approach. It may not be the most elegant solution but this enables that pony_error can still be used and everything remains the same.

Using this you don't have to change the current FFI interface and also switch back and forth between exceptions/error variable in order to test things and compare.

[SeanTAllen]

We were discussing background of this during sync and I brought up a point that wasn't recorded here. When we had this conversation, Sylvan had come around to being in favor this general approach for handling errors.

I am adding now as Joe asked me to add so he could remember that point going forward.

[SeanTAllen]

We should put this through the RFC process. The key point would be "getting rid of allowing FFI functions to be partial". We need to make sure we are ok with that change.

[jemc]

Regarding the comment about using a thread-local variable, please note that this wouldn't be enough to preserve the current behavior of pony_error because the current behavior unwinds the stack, and any code using it would be relying on that. If it only sets a thread-local variable, that would be different behavior and the code using it would need to find a way to return something (which it wasn't before).

I'm in favor of just removing pony_error - which we discussed would need to go through the RFC process.

I briefly did a GitHub search and found no usage of this feature outside the standard library.