-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Fix debuginfo for generators #84752
Fix debuginfo for generators #84752
Conversation
All fields except the discriminant (including `outer_fields`) should be put into structures inside the variant part, which gives an equivalent layout but offers us much better integration with debuggers.
(rust-highfive has picked a reviewer for you, use r? to override) |
There is actually another commit (lrh2000@e65c460) that names the upvars for closures and generators in debuginfo. Finally we'll be able to see something like
But I am not sure whether what I did is right. Could I add the above commit into this PR or should I open a separate PR for it later? Any comments are welcome. And thanks in advance. |
1995cd9
to
5bf989e
Compare
- Literally, variants are not artificial. We have `yield` statements, upvars and inner variables in the source code. - Functionally, we don't want debuggers to suppress the variants. It contains the state of the generator, which is useful when debugging. So they shouldn't be marked artificial. - Debuggers may use artificial flags to find the active variant. In this case, marking variants artificial will make debuggers not work properly. Fixes rust-lang#79009.
cc @tmandry |
I'd remove |
📌 Commit 5bf989e has been approved by |
Fix debuginfo for generators First, all fields except the discriminant (including `outer_fields`) should be put into structures inside the variant part, which gives an equivalent layout but offers us much better integration with debuggers. Second, artificial flags in generator variants should be removed. - Literally, variants are not artificial. We have `yield` statements, upvars and inner variables in the source code. - Functionally, we don't want debuggers to suppress the variants. It contains the state of the generator, which is useful when debugging. So they shouldn't be marked artificial. - Debuggers may use artificial flags to find the active variant. In this case, marking variants artificial will make debuggers not work properly. Fixes rust-lang#79009. And refer https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Debuginfo.20for.20generators.
Thanks a lot for your feedback. I'll take your advice. The trickiest problem is that if we simply remove fn main() {
let a = 3;
let mut c = move || {
let a = a;
yield;
let b = a;
};
Pin::new(&mut c).resume(());
_zzz();
} We could get something like
But if we simply remove |
I think what you said is reasonable. How about just replacing
I agree. I added |
Fix debuginfo for generators First, all fields except the discriminant (including `outer_fields`) should be put into structures inside the variant part, which gives an equivalent layout but offers us much better integration with debuggers. Second, artificial flags in generator variants should be removed. - Literally, variants are not artificial. We have `yield` statements, upvars and inner variables in the source code. - Functionally, we don't want debuggers to suppress the variants. It contains the state of the generator, which is useful when debugging. So they shouldn't be marked artificial. - Debuggers may use artificial flags to find the active variant. In this case, marking variants artificial will make debuggers not work properly. Fixes rust-lang#62572. Fixes rust-lang#79009. And refer https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Debuginfo.20for.20generators.
Rollup of 5 pull requests Successful merges: - rust-lang#84358 (Update closure capture error logging for disjoint captures for disjoint captures) - rust-lang#84392 (Make AssertKind::fmt_assert_args public) - rust-lang#84752 (Fix debuginfo for generators) - rust-lang#84763 (shrink doctree::Module) - rust-lang#84821 (Fix nit in rustc_session::options) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
I'd be fine with it. This should also affect closures. We should also support printing the original name of the variable while stepping through the closure/generator (I think we already do).
My sense is that could be confusing, since the user never writes |
Name the captured upvars for closures/generators in debuginfo Previously, debuggers print closures as something like ``` y::main::closure-0 (0x7fffffffdd34) ``` The pointer actually references to an upvar. It is not very obvious, especially for beginners. It's because upvars don't have names before, as they are packed into a tuple. This PR names the upvars, so we can expect to see something like ``` y::main::closure-0 {_captured_ref__b: 0x[...]} ``` r? `@tmandry` Discussed at rust-lang#84752 (comment) .
First, all fields except the discriminant (including
outer_fields
) should be put into structures inside the variant part, which gives an equivalent layout but offers us much better integration with debuggers.Second, artificial flags in generator variants should be removed.
yield
statements, upvars and inner variables in the source code.Fixes #62572.
Fixes #79009.
And refer https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Debuginfo.20for.20generators.