std: Partial fix for #33368

[lhecker]

This partially fixes a problem with std::panic::catch_unwind not being compatible with coroutines due to holding onto a thread-local reference across a user providden callback.

Thanks @arielb1!

Note: Unfortunately I was incapable of testing this PR because compilation of librustc failed for some reason and I couldn't figure out why.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[lhecker]

r? @alexcrichton

[alexcrichton]

As I mentioned before, this is not sufficient for actually getting coroutines to work with the standard library. Any usage of TLS can end up causing segfaults in a program with coroutines due to LLVM's optimizations today. The hack of #[inline(never)] can get this specific case working, but for example calling thread::panicking is enough to cause a segfault.

[lhecker]

Ah damn... You're right. Right about everything. I'm really sorry about that.

So what should I do now? Mark std::thread::LocalKey::with() as inline(never), Or maybe a compile-time option to control inlining of TLS methods in the current crate and it's dependencies? Or give up on Rust, because it uses TLS that much? That would be horrible since it would make any kind of low latency or high scalability networking systems impossible. But I guess that's it. Back to C++ I guess, huh? 😞

But I do have to say that even if we would make TLS access inline(never) like it was back then with the green threads it wouldn't work with the current way the inner_try method is written. I guess you might still merge this in the case that there is ever some system in Rust that allows Coroutines.

---

Original comment(s):

I know @alexcrichton. I know! As I said multiple times before!

Can you wrap thread::panicking inside a #[inline(never)] wrapper? Yeah you can.
Can you do something about panic::catch_unwind? No.

Can't you merge this please? Really... It really saddens me to hear from you that just because we can't fix it a 100% it's not worthy to fix it all. There will never be a working TLS solution for coroutines, you know?

This change would be very welcome for the coio as well as the mioco project and I'm pretty sure for all future coroutine implementation that will come.

I mean... we might as well also declare thread::panicking as #[inline(never)] or are coroutines "just" incompatible with Rust? That would be quite... bad. And sad. So this option is better than nothing without causing any regressions.

Really... this PR is such an essential thing that without it coroutines are de facto impossible to implement with Rust even if you wanted to. And that's frankly quite horrible. 😕

[aturon]

cc @rust-lang/libs

This thread is highlighting a policy issue we should discuss. Do others on the libs team have thoughts?

[aturon]

cc @rust-lang/lang as well.

[zonyitoo]

It seems that it is impossible to use Rust to develop any Coroutine libraries now, because it is impossible to bypass the libstd!!! With the current libstd design, we cannot switch contexts because it will crash everything.

Or maybe we can fork a libstd to build our own version of that?

[lhecker]

• I think it's a option to fork Rust to provide a Go like environment, but it would probably be hardly feasible to maintain that.
• Making TLS access inline(never) really does degrade performance of repeated access in the same function (after optimization) severely (I think it's at least halfed). But is TLS even used that often in hot-paths in Rust? I did not find any docs on how to do perf. regression tests, but if I did them the result would probably show a insignificant change.
• Adding a option akin to the exchangeable allocators or panic runtimes, would be feasible too, but probably hard to implement. Is that the only option?
• Not supporting coroutines at all or even not supporting them in the near future would be a bad idea though, because they are just so quintessential for many high-performance systems, which is something Rust wants to excel in, right?

[lhecker]

Oh and I'd like to highlight that a matching issue already exists: #33368

As I said: There are a couple solutions for this and implementing one of them would be quite advantageous for Rust to be adopted for server software. This PR might still be useful depending on the outcome though.

[bors]

☔ The latest upstream changes (presumably #32900) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

The libs team discussed this PR during triage yesterday, and the conclusion was that we probably don't want to merge this at this time. We decided that we're amenable to supporting coroutines in the standard library perhaps one day (so long as it comes at no cost to other users), but this is unfortunately not enough to actually fix compatibility in the standard library itself (TLS is overall a major problem). As a result we don't want to take piecemeal PRs like this for now which fix bugs as they come up, but rather fix the problem at the source (TLS in this case).

For now I'm going to close this in light of that decision, and we can continue discussion on #33368

[alexcrichton]

Ah, and one other things I forgot to mention. A great fix for this would be to actually just not use TLS at all here. That's somewhat costly in terms of the overhead of catch_panic, so getting rid of that would be great entirely! (may be tricky though)

[lhecker]

@alexcrichton Thanks for taking your time for this (now) useless PR. I'm sorry for that. 😅
I've updated the initial post with the issue description in #33368 to better reflect the actual issue.
It also contains 4 possible solutions - I'd be cool if the libs teams could take a look at it in the near future, so we (@zonyitoo and me from the coio project) can move on and plan ahead for whatever the result of your discussion is (like writing a RFC for instance or code for Rust). 😊