Use Ceescape's sigsetjmp() wrapper
#1621
Conversation
|
What I worry about here is cee-scape’s correctness. I don’t know if our CI passing all tests is a robust enough exercise. I also don’t have helpful suggestions to really ensure correctness. Don’t let this stand in the way of approval. Just wanted to voice a concern. |
Yeah, that's reasonable. I have similar worries. Unfortunately, I'm not trained in the theory and practice of formal verification, so I can't guarantee this is correct. Would it be possible to introduce tests that would more thoroughly check the validity of our sigsetjmp usage? |
|
in practice all of our "try/catch" suite does that. |
Aren’t we also assuming this prevents possible compiler optimizations? |
|
On the flip side, I’m super jazzed this isn’t an invasive change! A++ |
Yes? The fact that rustc cannot elide an asm block that control flow passes through, and must execute it, without reasoning about its contents except where annotated ( This is heavily relied on for allowing the Rust abstract machine to ignore machine-specific details in many cases and instead implementing those in assembly. |
|
...looks like the macOS machines are using rustc 1.77 and the Linux ones are using rustc 1.76... |
|
Okay. That closes the primary concern I had and it’s good to have that linked here. Then, I guess the only concern is just “what if cee-scape is bugged?” But that’s not really any different than any other dependency we have. And the upstream folks seem receptive to patches. ship it! |
|
@NotGyro can you update the PR and/or commit message with that link and adjusted appropriately? i.e. that |
|
The tests should pass after rebase. |
| let mut result: std::mem::MaybeUninit<T> = MaybeUninit::uninit(); | ||
| let jump_value = cee_scape::call_with_sigsetjmp(false, |jump_buffer| { | ||
| // Make Postgres' error-handling system aware of our new | ||
| // setjmp/longjmp restore point. | ||
| pg_sys::PG_exception_stack = std::mem::transmute(jump_buffer as *const SigJmpBufFields); | ||
|
|
||
| // execute the closure, which will be a wrapped internal Postgres function | ||
| let result = f(); | ||
| result.write(f()); | ||
| 0 | ||
| }); | ||
|
|
||
| // restore Postgres' understanding of where its next longjmp should go | ||
| if jump_value == 0 { |
There was a problem hiding this comment.
I wonder if we'll need to tweak the scope this includes... maybe. This should be good for now, though.
|
This seems to have broken builds on ppc64el which is an architecture supported by Postgres and Rust. Is there a mitigation? Related #1897 |
The cee-scape crate uses a hand-rolled assembly wrapper around libc's
sigsetjmp(). This has better correctness guarantees than callingsigsetjmp()directly, because LLVM is not allowed to reason about / attempt to optimize inside hand-written assembly.sigsetjmp()is used forpg_guard_ffi_boundary_impl(), which is our wrapper around Postgres' exception handling, since Postgres usessigsetjmp()andsiglongjmp()to handle multiple return points. Rust's paradigm doesn't understand this - in Rust, a a function call can only return to its caller at one point in time: when it has finished executing. Whereas, sigsetjmp can return to two places chronologically - and so this needs to be handled carefully.Fixes #1144