Regression in rlua crate tests between rustc 1.23.0 and 1.24.0, only on windows.

[kyren]

[The following explanation of the problem contains a lot of assumptions about the problem's nature, I could be extremely wrong! Take this explanation with a grain of salt]

Wild guess, it has something to do with #46833.

So, I'm not sure this is a rust bug exactly, but I wanted to make you aware of a crate regression that's causing me quite a lot of problems. Ultimately, I believe this comes from me doing some things that are not.. exactly definitely kosher, but that I don't have a convenient workaround for.

Ultimately, the problem stems from the Lua C API having error handling fundamentally based around setjmp / longjmp. In order to write a usable wrapper in Rust, without having to write inconvenient, slow C shims, Rust must, at times, call into Lua C API functions that in turn trigger a longjmp, and that longjmp must necessarily pass over Rust frames.

I realize how unsafe this is! However, there are some limitations here that I think make it at least somewhat reasonable to be able to do:

1. There are only Copy types on the rust stack frame being jumped over.
2. Rust is never calling setjmp, only triggering a longjmp through a Lua C API call (such as lua_error).
3. Making this unsafe to do means that it is impossible to wrap a C API such as Lua's without resorting to writing more C.

This last point is unfortunate, and causes me a lot of headaches, and at least one strange practical problem. We at Chucklefish build rlua for consoles, but for consoles the gcc crate does not work, and we have to build Lua out of tree (also to include Lua console specific patches). Needing to include C shims in rlua means that we would also have to copy paste those shims into the console projects, and this is a less than ideal situation.

The reason I think this issue has to do with #46833 is that I can actually fix the regression if I simply add an #[unwind] attribute to each rust function that calls lua_error. However, this is only necessary for some reason on windows, and the attribute is currently unstable!

I think it's completely reasonable to need to mark functions as #[unwind] if they will trigger a longjmp, but I'm not exactly sure what to do in the meantime until unwind_attributes becomes stable. Maybe this behavior on windows IS simply a bug? Maybe it's a bug on linux / macos that this doesn't abort? I would especially not like to have to write C shims if ultimately I can get rid of them when unwind_attributes becomes stable.

(Side Note: Chucklefish actually uses the stable rust compiler in production now, so this is especially annoying. We were very excited about the 1.24 release, because it includes incremental compilation and rustfmt-preview while also being stable, and we noticed this bug in trying to switch to the stable compiler on windows.)

Edit: If people have to dig into the rlua source, I'm sorry, it is a tangled nest of "unsafe, unsafe everywhere". The best example to look at is the test_error test, and the method Lua::create_callback_function. Adding the #[unwind] attribute to: callback_call_impl, safe_pcall, and safe_xpcall fixes that test on windows, on unstable.

Edit 2: Edited for clarity, but also I need to point out that I need to do something, because this bug currently makes rlua just basically broken on windows. It's not "just" a test failure, error handling in general will just abort.

[sfackler]

One possibility as to why that it only happens on windows is that the implementation of longjmp on that platform uses the same exception framework that Rust uses for panics.

[kyren]

One possibility as to why that it only happens on windows is that the implementation of longjmp on that platform uses the same exception framework that Rust uses for panics.

That is interesting! That WOULD explain the behavior I'm seeing.

[petrochenkov]

I'm not exactly sure what to do in the meantime until unwind_attributes becomes stable.

It's possible to temporarily unlock unstable features on stable channel with env var (RUSTC_BOOTSTRAP=1), seems acceptable if you have a critical situation like this.

[kyren]

It's possible to temporarily unlock unstable features on stable channel with env var (RUSTC_BOOTSTRAP=1), seems acceptable if you have a critical situation like this.

That's incredibly helpful, thank you!

[kyren]

Here is a summary of the current situation as I understand it: mlua-rs/rlua#71 (comment)

I may have gotten some details wrong, if I have feel free to yell at me point that out.

[SoniEx2]

I guess when they say it's a "C API" they are not kidding. :p

Would it make sense to make an standalone wrapper, called "Universal API for Lua, with C calling conventions"?

[kyren]

I guess when they say it's a "C API" they are not kidding. :p

Lua's API is not my favorite :(

[nikomatsakis]

Hmm. I think there's a good case to be made for disabling #46833 -- or at least making it opt-in -- until there are stable attributes to opt-out. @wycats points out that this likely to affect Helix as well. I would definitely prefer not to have public packages relying on RUSTC_BOOTSTRAP=1 -- it's a recipe for undermining the whole Rust stability system.

[nikomatsakis]

Re-reading the comments on that PR, I see that I wrote this:

Note though that this is a change in behavior -- albeit only quasi-defined behavior -- and it feels like it ought to go through the RFC process. Still, it'd be good to have a working implementation so that we can do a crater run and assess possible impact.

But this got overlooked.

[kyren]

I would definitely prefer not to have public packages relying on RUSTC_BOOTSTRAP=1 -- it's a recipe for undermining the whole Rust stability system.

I knew that was wrong when I did it, I'll forget this hack exists :P

[kyren]

Note though that this is a change in behavior -- albeit only quasi-defined behavior -- and it feels like it ought to go through the RFC process. Still, it'd be good to have a working implementation so that we can do a crater run and assess possible impact.

But this got overlooked.

Something I wanted to point out, is that even if a crater run was done, it wouldn't have caught my issue at least because as I understand it they're only done on linux?

If there was some kind of donation box for running crater on windows I would contribute to it!