Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unwrap_panicked test fails (unwind support) #1

Open
cynecx opened this issue Oct 16, 2020 · 6 comments
Open

unwrap_panicked test fails (unwind support) #1

cynecx opened this issue Oct 16, 2020 · 6 comments

Comments

@cynecx
Copy link
Owner

cynecx commented Oct 16, 2020

The generator::unwrap_panicked test fails. It seems like both catch_unwinds (inside unwind_wrapper and inside the test) are not catching the panic. Seems like the unwinder somehow fails to properly unwind the stack.

😔

Ref PR: edef1c#70

@cynecx
Copy link
Owner Author

cynecx commented Oct 16, 2020

It seems like some bug sneaked into my branch as I can reproduce the test failure in debug mode. However the original unwind branch doesn't seem to have this bug. But the original unwind branch experiences release mode test failures. It looks like a compiler "regression" because I was able to bisect the issue to the follwing PR: rust-lang/rust#67502.

@cynecx
Copy link
Owner Author

cynecx commented Oct 18, 2020

Okay, the issue seems to be that no landingpads exist when the context switch, which is done through the inline assembly, is (swap_link/swap) fully inlined. Without inlining, a landingpad would exist because without swap being inlined, swap would've been called with invoke <target> to label <dst> unwind <catch> , hence having a landingpad. Inlining basically removes this because swap contains inline assembly which doesn't have a landingpad (llvm doesn't support that rn), so llvm doesn't generate the required exception (lsda) tables.

There are two solutions:

  • Annotate swap and swap_link with #[inline(never)] (Not sure if its guaranteed by the compiler though).
  • Introduce inline asm unwinding support on llvm's side and patch rustc.

In its current state unwinding support is basically unsound and broken.

@cynecx cynecx pinned this issue Oct 18, 2020
@cynecx
Copy link
Owner Author

cynecx commented Oct 18, 2020

Re: #[inline(never)], according to https://doc.rust-lang.org/reference/attributes/codegen.html#the-inline-attribute, it seems that the attribute is merely a suggestion. That leaves the first solution unsound.

@cynecx
Copy link
Owner Author

cynecx commented Jan 24, 2021

LLVM patch which should allow unwinding from inline asm: https://github.com/cynecx/llvm-project/tree/asm-landingpads (The patch only contains support for SelectionDAG. Adding support for GlobalIsel should be straightforward too).

@cynecx cynecx changed the title unwrap_panicked test fails unwrap_panicked test fails (unwind support) Jan 24, 2021
@cynecx
Copy link
Owner Author

cynecx commented Jan 30, 2021

Let's see where this goes: https://reviews.llvm.org/D95745. Support for unwinding from inline-asm has been upstreamed! 🎉

Next step: Wait for rust to upgrade to LLVM 13.

@cynecx
Copy link
Owner Author

cynecx commented Aug 29, 2021

@Amanieu JFYI: I've been reviving your old "unwinding support" for libfringe branch here: https://github.com/cynecx/libfringe/tree/asm as a test case for rust-lang/rust#88439.
The test suite passes with a "small rustc hack" (adding back rbx clobber for the x86_64 backend), alternatively, I thought about just "push/pop"-ing rbx. However, I haven't figured out the correct cfi directives yet, so the unwinder actually restores rbx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant