-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
stabilize #[panic_handler] #51366
stabilize #[panic_handler] #51366
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
At the very least, an issue needs to be opened on the reference. This isn't right for the book, and maybe for Rust By Example. I'm imagining that the embedded docs your WG is working on may want to include it too? |
☔ The latest upstream changes (presumably #51140) made this pull request unmergeable. Please resolve the merge conflicts. |
@steveklabnik it isn't or is? If not then maybe at least the nomicon? This change basically turns implementing |
I would think docs for this should live in the Rust Reference. I'd like to see using that as, well, the reference for Rust. =) |
It isn’t. The book’s objective is to teach you Rust; you don’t need to know any of this to learn how to program in Rust.
It *is* worth documenting, but in our current set of resources, the reference and/or nomicon is the right place. As we develop more books for the shelf, it might fit in there too.
… On Jun 5, 2018, at 4:09 PM, Amit Levy ***@***.***> wrote:
This isn't right for the book
@steveklabnik it isn't or is? If not then maybe at least the nomicon? This change basically turns implementing panic from dark magic to fairly accessible and would be really good to have an easy to find reference for it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Unwinding works on no_std for me for a very long time, we use it in production. Is there anything specific you want to know? I use unwinding with a custom personality function for the language I designed, but the Rust personality function would work in a completely identical way as it does not depend on the OS at all (the libc dependency in panic_unwind crate is just for the types) and only barely depends on the platform (it needs to know the register numbers that the landing pad uses and that's it). |
r? @nagisa |
The documentation for this should be guide level in the nomicon and reference level in well, the reference. |
Ping from triage @nagisa! This PR needs your review. |
Is this really ready for stabilisation? The tracking issue still has references to a few issues that are, honestly, quite perplexing (for example why in the world Furthermore, I’m not entirely sure what are the steps before a stabilisation PR can be merged? I mildly recall having to go through a week of FCP? That would be for both T-libs (for approach, also initial issue is marked T-libs) and T-compiler (for implementation). Lastly, if I remember well, documentation has to be ready at least in form of PRs for stabilisation? Comments indicate it is not quite ready yet. |
@nagisa Yes, before merging this PR a summary comment must be written on the tracking issue nominating the feature for FCP to stabilize and the stabilization must complete without new blocking objections. |
We'll certainly be documenting
Yes. Basically, does your unwinding implementation continue to work with #[panic_implementation]? Less on-topic: could you point me to your unwinding implementation? I'd like to write documentation on how to get unwinding working on no_std. Right now it's unclear (in the docs I've seen) what eh_personality is for, how to compiler uses it and how it interacts with panic_fmt / panic_impl (if at all).
The #[no_mangle] issue that was reported looks like #51647 to me which, AFAICT, is the oom lang item undoing #49316 and not a #[panic_implementation] issue. Can't really tell as the issue is scarce on details, but I haven't required #[no_mangle] in any of my uses of #[panic_implementation] (I'm not using #[global_allocator] or the oom lang item though).
I'll prep a PR for the reference and the nomicon. So far it sounds like there are no concerns with the feature itself or its syntax. Can we start the FCPbot thing to have the appropriate team confirm that there are no concerns? cc @nikomatsakis |
I'll fcpbot this evening.
…On Wed, Jun 20, 2018, 05:12 Jorge Aparicio ***@***.***> wrote:
@steveklabnik <https://github.com/steveklabnik>
I'm imagining that the embedded docs your WG is working on may want to
include it too?
We'll certainly be documenting #[panic_implementation] in the embedded
book, which is geared towards people that want to learn how to do embedded
Rust, as lots of people will have to deal with it. More obscure stuff, like
#[used], (that most devs won't have to directly deal with) will go in the
embedonomicon.
@whitequark <https://github.com/whitequark>
Unwinding works on no_std for me for a very long time, we use it in
production. Is there anything specific you want to know?
Yes. Basically, does your unwinding implementation continue to work with
#[panic_implementation]?
Less on-topic: could you point me to your unwinding implementation? I'd
like to write documentation on how to get unwinding working on no_std.
Right now it's unclear (in the docs I've seen) what eh_personality is for,
how to compiler uses it and how it interacts with panic_fmt / panic_impl
(if at all).
@nagisa <https://github.com/nagisa>
The tracking issue still has references to a few issues that are,
honestly, quite perplexing (for example why in the world #[no_mangle] is
required).
The #[no_mangle] issue that was reported looks like #51647
<#51647> to me which, AFAICT, is
the oom lang item undoing #49316
<#49316> and not a
#[panic_implementation] issue. Can't really tell as the issue is scarce on
details, but I haven't required #[no_mangle] in any of my uses of
#[panic_implementation] (I'm not using #[global_allocator] or the oom lang
item though).
Lastly, if I remember well, documentation has to be ready at least in form
of PRs for stabilisation?
I'll prep a PR for the reference and the nomicon.
------------------------------
So far it sounds like there are no concerns with the feature itself or its
syntax. Can we start the FCPbot thing to have the appropriate team confirm
that there are no concerns? cc @nikomatsakis
<https://github.com/nikomatsakis>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51366 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AApc0uLg6HIZN-vxDx2ERUTmMK_33CSAks5t-a-JgaJpZM4Uad8d>
.
|
Documentation PRs: rust-lang/reference#362 and rust-lang/nomicon#75 |
Ping from triage @nagisa! You need to fcpbot this. |
⌛ Testing commit 358fc5b with merge 51bf99f99dc727866536b453b71006e9df903c2c... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry
|
…imulacrum stabilize #[panic_handler] closes rust-lang#44489 ### Update(2018-09-07) This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment) Documentation PRs: - Reference. rust-lang/reference#362 - Nomicon. rust-lang/nomicon#75 --- `#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team. (\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054 Some unresolved questions from rust-lang#44489: > Should the Display of PanicInfo format the panic information as "panicked at 'reason', > src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason". The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable. > Is this design compatible, or can it be extended to work, with unwinding implementations for > no-std environments? I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update? --- Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation. cc @rust-lang/lang cc @jackpot51 @alevy @phil-opp
Rollup of 11 pull requests Successful merges: - #51366 (stabilize #[panic_handler]) - #53162 (rustdoc: collect trait impls as an early pass) - #53932 ([NLL] Remove base_place) - #53942 (Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.) - #53973 (Have rust-lldb look for the rust-enabled lldb) - #53981 (Implement initializer() for FileDesc) - #53987 (rustbuild: allow configuring llvm version suffix) - #53993 (rustc_resolve: don't record uniform_paths canaries as reexports.) - #54007 (crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint) - #54040 (update books for next release) - #54050 (Update `petgraph` dependency to 0.4.13 to fix build with nightly) Failed merges: r? @ghost
stabilize #[panic_handler] closes #44489 ### Update(2018-09-07) This was proposed for stabilization in #44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in #44489 (comment) Documentation PRs: - Reference. rust-lang/reference#362 - Nomicon. rust-lang/nomicon#75 --- `#[panic_implementation]` was implemented recently in #50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. #44489) but this PR is meant to start a discussion about those issues / questions with the language team. (\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in #43054 Some unresolved questions from #44489: > Should the Display of PanicInfo format the panic information as "panicked at 'reason', > src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason". The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable. > Is this design compatible, or can it be extended to work, with unwinding implementations for > no-std environments? I believe @whitequark made more progress with unwinding in no-std since their last comment in #44489. Perhaps they can give us an update? --- Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation. cc @rust-lang/lang cc @jackpot51 @alevy @phil-opp
☀️ Test successful - status-appveyor, status-travis |
panic_implemenation was renamed to panic_handler: rust-lang/rust#44489 (comment) panic_handler was stablized: rust-lang/rust#51366 `cargo check` now succeeds without warnings
panic_implemenation was renamed to panic_handler: rust-lang/rust#44489 (comment) panic_handler was stablized: rust-lang/rust#51366 `cargo check` now succeeds without warnings
panic_implemenation was renamed to panic_handler: rust-lang/rust#44489 (comment) panic_handler was stablized: rust-lang/rust#51366 `cargo check` now succeeds without warnings
panic_implemenation was renamed to panic_handler: rust-lang/rust#44489 (comment) panic_handler was stablized: rust-lang/rust#51366 `cargo check` now succeeds without warnings
1950: remove all uses of lang_items unstable feature r=ppannuto a=hudson-ayers ### Pull Request Overview This pull request removes all uses of the lang_items unstable feature, to get us closer to building Tock on stable rust. Originally, lang_items was required to declare the global panic handler, but that was changed by rust-lang/rust#51366 . After that, we seem to have just kept it around for declaring a dummy `eh_personality()` implementation, which I understand is not needed if you build with panic=abort (which we do for both release and debug builds). see: https://os.phil-opp.com/freestanding-rust-binary/ ### Testing Strategy This pull request was tested by compiling, running a normal app on Imix, and by panicing intentionally and observing the output looks normal. ### TODO or Help Wanted Confirmation that I am correct in my understanding that we do not need eh_personality because we always use panic=abort. ### Documentation Updated - [x] No updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Hudson Ayers <hayers@stanford.edu>
closes #44489
Update(2018-09-07)
This was proposed for stabilization in #44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in #44489 (comment)
Documentation PRs:
#[panic_implementation]
was implemented recently in #50338.#[panic_implementation]
is basically the oldpanic_fmt
language item but in a less error prone (*) shape. There are still some issues and questions to sort out around this feature (cf. #44489) but this PR is meant to start a discussion about those issues / questions with the language team.(*)
panic_fmt
was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in #43054Some unresolved questions from #44489:
The current implementation formats
PanicInfo
as the first alternative, which is how panic messages are formatted by thestd
panic handler. TheDisplay
implementation is more than a convenience:PanicInfo.message
is unstable so it's not possible to replicate theDisplay
implementation on stable.I believe @whitequark made more progress with unwinding in no-std since their last comment in #44489. Perhaps they can give us an update?
Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.
cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp