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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add panic implementation docs #521

Open
wants to merge 6 commits into
base: master
from
Open

Conversation

@Aaron1011
Copy link

Aaron1011 commented Nov 22, 2019

This PR adds a descirption of the implementation of panicking in Rust, starting from the panic! macros and ending at the invocation of the platforms-specific unwind logic.

This is not fully complete - for one thing, it might be useful to have a description of how this works under Miri.

I developed this while working on the implementation of panics in Miri. Having this document available would have made things a lot easier for me 馃槃

Closes #486

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Nov 23, 2019

Thanks, this looks useful! I too am trying to better understand the panic machinery.

I was wondering if you could define more what a "landing pad" is? I assume it is some reserved section of the stack frame, that does鈥omething related to unwinding? It sounds like it has a list of destructors to run, is that all it does?

It might also be helpful to mention that there are two different panic strategies (encoded per crate), which are only loosely coupled with the panic runtimes. For example, the abort runtime can use crates built with unwind strategy, but not the other way around. The abort strategy disables landing pads, but I'd like to know what else changing the strategy does (or maybe landing pads are the only difference?).

Copy link
Member

mark-i-m left a comment

@Aaron1011 Thanks! This is very helpful info, and I have wondered about it before many times.

My biggest request is to please add links for all of the names you reference (the first time you reference them) to the source code (since there don't appear to be rustdocs for most of these definitions). This helps us check for breakage by just doing linkchecking.

Thanks!

src/SUMMARY.md Show resolved Hide resolved
src/panic-implementation.md Outdated Show resolved Hide resolved
src/panic-implementation.md Outdated Show resolved Hide resolved
src/panic-implementation.md Outdated Show resolved Hide resolved
[cfg(not(test))]
[panic_handler]
[unwind(allowed)]
Comment on lines +38 to +41

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Nov 23, 2019

Member

It looks like you are missing # before these lines?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Dec 2, 2019

Author

I can't seem to figure out how to stop the # from being interpreted as the start of a comment.

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Dec 6, 2019

Member

I don't think md has comments? Can you elaborate on what the concern is?

@mark-i-m

This comment has been minimized.

Copy link
Member

mark-i-m commented Nov 23, 2019

I was wondering if you could define more what a "landing pad" is?

@Aaron1011 This would be a great addition to the glossary appendix if you have time.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 25, 2019

Not knowing about this PR, I wrote a blog post about this very topic. Sorry for the duplication of work!

We can likely copy some things from that post into this PR.

EDIT: You can find the markdown sources here, for easier copying.

src/panic-implementation.md Outdated Show resolved Hide resolved
src/panic-implementation.md Outdated Show resolved Hide resolved
src/panic-implementation.md Outdated Show resolved Hide resolved
@Aaron1011 Aaron1011 force-pushed the Aaron1011:panic branch from 91b8465 to fce7efc Dec 2, 2019
@Aaron1011

This comment has been minimized.

Copy link
Author

Aaron1011 commented Dec 2, 2019

The CI failure seems unrelated

@mark-i-m

This comment has been minimized.

Copy link
Member

mark-i-m commented Dec 6, 2019

@Aaron1011 Could you rebase on top of the latest master please? Hopefully that will get rid of the CI failures.

Also, it would be awesome if you could add as many links as possible, as this helps us to detect changes.

Aaron1011 and others added 6 commits Oct 29, 2019
Co-Authored-By: Who? Me?! <mark-i-m@users.noreply.github.com>
@Aaron1011 Aaron1011 force-pushed the Aaron1011:panic branch from f620c53 to 9f25a02 Dec 7, 2019
@Aaron1011

This comment has been minimized.

Copy link
Author

Aaron1011 commented Dec 7, 2019

@mark-i-m: The CI failures didn't go away after rebasing

@JohnTitor

This comment has been minimized.

Copy link
Member

JohnTitor commented Dec 7, 2019

@Aaron1011 Could you add a line break not to exceed 100 characters of each line?

@mark-i-m

This comment has been minimized.

Copy link
Member

mark-i-m commented Dec 7, 2019

@JohnTitor Something odd is going on here... these files are not touched by the PR, and they are working just fine on master..

@mark-i-m mark-i-m closed this Dec 7, 2019
@mark-i-m mark-i-m reopened this Dec 7, 2019
@JohnTitor

This comment has been minimized.

Copy link
Member

JohnTitor commented Dec 8, 2019

Uhm, it's odd. The checker here doesn't ignore lines inside a code block...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.