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

Use fibers on Windows. Use CFI instructions on other platforms to enable unwinding, catching panics and backtraces. #4

Closed
wants to merge 12 commits into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Oct 31, 2017

No description provided.

@Zoxc Zoxc force-pushed the fiber branch 2 times, most recently from 9977f8e to bc17fe0 Compare October 31, 2017 23:19
@alexcrichton
Copy link
Member

Can you expand on the rationale for this? I'd always imagine that if we wanted to support unwinding through these stack segments we'd just use catch_unwind and a propagate?

@Zoxc
Copy link
Contributor Author

Zoxc commented Nov 1, 2017

Windows keeps a bunch of internal state related to the stack. So if we change the stack pointer we have no guarantee that Windows APIs which use this internal state still work. I know that at least .NET relies on some of this state for optimizing stack probes and changing the stack pointer would make that optimization unsound.

@alexcrichton
Copy link
Member

Ok... There's basically zero documentation in the code itself to explain any of these changes (or the PR description). Can that be added? Are the non-Windows changes necessary if we catch panics and propagate?

Do you have an example of what breaks on Windows today?

@Zoxc
Copy link
Contributor Author

Zoxc commented Nov 1, 2017

The current code would break GetCurrentThreadStackLimits in that it would return the limits of the Windows stack. It is also incorrect for code using fibers since the stack limit is stored in a thread local. I also have no idea what happens if switch fibers while using a custom stack pointer.

For the non-Windows changes, the addition of a guard page is necessary. The usage of CFI instructions is the proper solution for unwinding (and it is how compilers provide unwind information to the assembler). Catching panics and propagating them isn't equivalent since that will result in the panic and C++ exceptions being handled even if there isn't actually a real handler for them. The unwind information is also required for backtraces.

Windows also has such unwinding instructions on x86-64, unfortunately it has sanity checks which prevents it from unwinding outside the current stack. It does work if I manually eliminate those checks however. I think it should be theoretically possible to write some custom unwinding code which handles this. I have not looked at how Windows deals with unwinding through multiple stacks on x86-32.

@alexcrichton
Copy link
Member

Hm ok thanks for the info, but presumably this crate can still work on Windows for libraries/applications not using fibers? That could presumably go in the documentation?

Throwing a C++ exception into Rust code is already undefined behavior so it's fine for this to not support them, so I think that the most portable solution for handling panics (if at all) will be a catch/propagate.

@Zoxc
Copy link
Contributor Author

Zoxc commented Nov 20, 2017

Hm ok thanks for the info, but presumably this crate can still work on Windows for libraries/applications not using fibers? That could presumably go in the documentation?

It will work fine for those. The problem is programs actually using fibers, since GetFiberData and GetCurrentFiber will change if the stack expands.

Throwing a C++ exception into Rust code is already undefined behavior so it's fine for this to not support them, so I think that the most portable solution for handling panics (if at all) will be a catch/propagate.

On Windows/MSVC Rust panics are C++ exceptions. They will still work, but you won't get unhandled exception errors on the origin of the panic, but at the location it was re-raised, probably after all stacker stacks have been unwound. Rust prints backtraces before doing any unwinding, so you'll still get a backtrace, although it won't be across any stacks.

@alexcrichton
Copy link
Member

On Windows/MSVC Rust panics are C++ exceptions.

Heh no need to explain, I implemented all of this so I'm quite familiar with the implementations!

but you won't get unhandled exception errors on the origin of the panic, but at the location it was re-raised, probably after all stacker stacks have been unwound. Rust prints backtraces before doing any unwinding, so you'll still get a backtrace, although it won't be across any stacks.

Ok this sounds like some good data! Extrapolating this it sounds like we'd get benefits a long the lines of:

  • Stack traces in debuggers would likely work better
  • Stack traces via RUST_BACKTRACE would likely work better
  • Profilers and such may work better as well

I think that we'll still want to deal with panics manually eventually (relying on the precise codegen here seems sketchy). Can you be sure to add comments to the various bits and pieces about how this all builds together? If I'm going to maintain this over time I'd like to at least be able to reconstruct how it all works...

@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 1, 2017

Extrapolating this it sounds like we'd get benefits a long the lines of:

You only get these benefits on Linux/macOS though. We have fallback to fibers on Windows which has none of these benefits, but it is the only officially supported way to change stacks.

I think that we'll still want to deal with panics manually eventually (relying on the precise codegen here seems sketchy).

You do not want that on non-Windows platforms. All the above benefits go away and you gain nothing. There's no reliance on some specific codegen here either.

@alexcrichton
Copy link
Member

Hm so we can't get better stack traces in non-Windows situations? That's unfortunate...

What I mean with catching panics is that I think catching explicitly is a more robust way to implement this over time with things like new architectures or oses, whereas ensuring that the assembly is just right seems difficult to do if we change it over time.

@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 5, 2017

Hm so we can't get better stack traces in non-Windows situations? That's unfortunate...

We do get better stack traces in non-Windows (i.e. DWARF) situations, it's Windows that doesn't get full stack traces.

What I mean with catching panics is that I think catching explicitly is a more robust way to implement this over time with things like new architectures or oses, whereas ensuring that the assembly is just right seems difficult to do if we change it over time.

The assembly shouldn't really change over time though. You could perhaps catch panics on new platforms if you don't know how to use unwinding instructions, but you eventually want to use unwinding instructions if possible since it makes the segmented stacks appear as one large stack.

@alexcrichton
Copy link
Member

My point though is that the assembly may change APIs interfaces or such over time. In that we may tweak the precise ABI and function signatures. I wouldn't personally know how to update all the cfi business.

@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 7, 2017

One trick to find the correct assembly is to look at the output of C compiler with a function using alloca. You can basically copy that assembly and just change it to set the stack register to some argument.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2018

ping @Zoxc do you think you could have another go at this? We're trying to use stacker in the compiler, but I can't get windows to work at the moment.

@Zoxc
Copy link
Contributor Author

Zoxc commented Nov 22, 2018

@oli-obk Did you try this branch/PR?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 23, 2018

It works, I just assumed from the above discussion that there was still something to be done.

#8 is a rebase of this PR + an integration test


// If the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment aborted in the middle of typing ;)

@alexcrichton
Copy link
Member

I'm gonna close this in favor of https://github.com/alexcrichton/stacker/pull/8

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

Successfully merging this pull request may close these issues.

3 participants