rlua error handling is completely broken on windows with rustc 1.24+

[kyren]

See rust-lang/rust#48251 for details.

[kyren]

So, this can be worked around right now by enabling the "unwind" feature, which requires nightly.

If this doesn't get fixed soon, it might be wise to make this a hard cfg!(target_os = "windows") or something, which would just mean rlua is COMPLETELY unusable on windows with stable.

[jonas-schievink]

Dang, I didn't know Windows used SEH to implement setjmp/longjmp. Of course that would unleash #21.

[kyren]

Dang, I didn't know Windows used SEH to implement setjmp/longjmp.

Yeah, me neither. I'm hoping this is an excuse to get an official word from the rust devs on whether this is meant to be supported or not. I suppose it's leaning towards yes because they even went through the effort of making #[unwind], but I would accept it if the answer was a definite no. Mostly I just want to know for sure so I can plan where to go from here.

[kyren]

From the README:

(NOTE: I believe it is still an open question whether technically Rust allows longjmp over Rust stack frames at all, even if there are only Copy types on the stack. Currently rlua uses this to avoid having to write a lot of messy C shims. It currently works fine, and it is difficult to imagine how it would ever NOT work, but what is and isn't UB in unsafe Rust is not precisely specified.)

I apparently have a pretty bad imagination :P

[kyren]

I wrote.. possibly the worst hack: dec360f

I know this isn't a solution, I just wanted to know if it was even possible. I'm kind of surprised build.rs even lets you get away with this? I mean.. I guess rlua now works again on stable with windows? My god, BUT AT WHAT COST??

[Timidger]

I'm (obviously) not targeting windows with my usage of rlua. But that's probably not a good solution, if only because it's still a nightly feature and you shouldn't depend upon nightly features since they could change

[kyren]

I don't really have another good solution other than waiting on the rust devs to respond. I mean, I could start writing a lot of C, but it's not JUST that I don't want to write a lot of C shims, it's also that writing C shims in the direct way where you simply write versions of a lot of functions that could longjmp but instead return error codes just ends up being really terrible. There are quite a lot of places where I "bunch up" several Lua C API calls together that could all longjmp to avoid the overhead of repeatedly calling lua_pcall (I don't do it everywhere, I need actually to do it in more places). I actually think that I would not have even chosen Lua at all if I thought Rust wouldn't be able to call half of its API safely.

[kyren]

I guess I wish that crater would have caught this problem sooner, I guess currently the rust devs don't do crater runs on windows?

What if I added rustc version checking with the rustc_version crate?

This is really starting to bother me because it's looking like a casual rust stable update is causing me problems to the level of like.. rewriting rlua.

I really wish the #[unwind] attribute had been stabilized at the same time as this change...

[jonas-schievink]

I guess currently the rust devs don't do crater runs on windows?

I think that's correct, but also there never was a crater run for rust-lang/rust#46833

[kyren]

So I've been trying to educate myself, reading the source to libpanic_unwind/[seh.rs, seh64_gnu.rs, windows.rs], and trying to understand better how windows SEH works. I didn't really learn anything dramatically new, but it helped cement my understanding of the issue and of the implementation of the rust runtime. Let me summarize the situation at this point, as I understand it (If I have anything wrong PLEASE correct me!):

The Lua C API is pretty fundamentally based around setjmp / lonjmp for error handling. It should be clear from the implementation of this crate that I am not exactly a FAN of this, but I don't have much of a choice but to deal with it. On windows, setjmp and longjmp are implemented using SEH, so this means that Lua errors are as well.

(I cannot find many specifics about HOW setjmp and longjmp are implemented with SEH, and my understanding of things like _CxxThrowException and __CxxFrameHandler3 are really shaky. If I had a better understanding of these I might be able to make a suggestion to how the rust compiler / runtime could be changed to ignore lonjmp SEH, but I'm not sure that such a change would even be welcome.)

It's definitely clear from reading libpanic_unwind that anything invoking SEH is incompatible with rust panic handling, both in principle and in practice. The reason this has worked up to this point is because prior to this, at no point was there a lonjmp in Lua without the corresponding setjmp being "above" it. There are plenty of calls to catch_unwind in rlua, but they always either catch rust panics, or first call lua_pcall (setjmp), which then in turn calls some Lua function that might error (longjmp).

The breaking change is the insertion of what is effectively the following code around all extern "C" calls:

match catch_unwind(extern_c_fn) {
    Ok(r) => r,
    Err(_) => ::std::process::abort(),
}

which breaks the rules above. Now, any time we call lua_pcall (setjmp) to in turn call some Lua functions that might error (longjmp), in between is inserted an extra call to catch_unwind (effectively) which catches the lonjmp instead of lua_pcall, which then simply aborts. Adding #[unwind] removes this wrapper, which restores the previous functionality. (Edit: I know this is an over-simplification, but I'm assuming what the compiler is inserting is whatever the try intrinsic does. Is this a bad simplification?)

This means that the following Lua C API functions that rlua uses are IMPOSSIBLE to call from rust in 1.24.0, unless you can be sure that they won't error (and in practice, it is often impossible to be sure they won't error):

lua_call / lua_callk
lua_pushstring / lua_pushlstring
lua_pushcclosure
lua_gettable
lua_geti
lua_rawseti
lua_createable
lua_newuserdata
lua_newthread
lua_settable
lua_rawset
lua_len
lua_next
lua_error
lua_gc (only some functions of lua_gc)
luaopen_xxxx (only technically?)
luaL_ref
luaL_checkstack
luaL_traceback
luaL_len

This is not an exhaustive list of all 'm' or 'e' functions in the Lua C API, only the ones that are in the rlua ffi module. Also, functions marked 'm' can be guaranteed not to error if garbage collection is not currently enabled and allocations never error, and rlua takes advantage of this in a few places.

If it turns out that rust does not intend to support calls to longjmp by rust (even indirectly), these functions will need to be wrapped in lua_pcall inside C code, either individually or as part of a set of other functionality. Doing so also means that the gcc crate now becomes a mandatory dependency, because there will need to be custom C code that is built along with the rlua library. Requiring the gcc crate may or may not be a problem for me on consoles, I need to actually check on this again.

If it turns out that the requiring the gcc crate is not a problem, I will just start implementing wrappers around these functions in C and moving a lot of rlua internal code to C. Maybe it's less bad than I'm imagining?

[kyren]

This is going to be awful, if I have to write all the callback entry points in C, I effectively cannot use lua_pushcfunction at all, only lua_pushcclosure with a rust fn pointer as one of the upvalues.

[kyren]

Okay, it turns out I was COMPLETELY wrong, and not only does the gcc crate work for me in console builds, it has been working this entire time! I even made sure that it worked correctly when I set this up (using CC, AR, other env vars etc), because otherwise ogg-sys and vorbis-sys would have never built correctly.

I don't know whether I'm smart because I was able to get all of this working, or really dumb for forgetting that I did.

Edit: Also, apparently it's now called the cc crate, not gcc.

[kyren]

The worst part of this change is actually the fact that, in order to trigger an error, there MUST be a C callback in between Lua and any Rust callback, to call lua_error on behalf of the Rust callback in case of an error. I have had to make my own "Rust callback protocol" that wraps the standard Lua C callback protocol, requiring use of another upvalue and more pointer chasing :(

I'm not done yet, and it's not exactly looking very clean so far :(

[SoniEx2]

Result<LuaOk, LuaErr>?