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

debuginfo: Stabilize -Z debug-macros, -Z collapse-macro-debuginfo and #[collapse_debuginfo] #120845

Merged
merged 2 commits into from Apr 26, 2024

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Feb 9, 2024

-Z debug-macros is "stabilized" by enabling it by default and removing.

-Z collapse-macro-debuginfo is stabilized as -C collapse-macro-debuginfo.
It now supports all typical boolean values (parse_opt_bool) in addition to just yes/no.

Default value of collapse_debuginfo was changed from false to external (i.e. collapsed if external, not collapsed if local) - #100758 (comment) describes some debugging scenarios that motivate this default as reasonable.
#[collapse_debuginfo] attribute without a value is no longer supported to avoid guessing the default.

Stabilization report: #120845 (comment)

Closes #100758
Closes #41743
Closes #39153

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

@petrochenkov
Copy link
Contributor Author

cc @davidtwco @azhogin

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Feb 9, 2024
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2024
@davidtwco
Copy link
Member

I haven't thoroughly reviewed the changes here but this seems sensible to me at first glance.

@petrochenkov
Copy link
Contributor Author

I removed one fast path optimization for simplicity, so let's also run benchmarks.
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 12, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2024
debuginfo: Stabilize `-Z debug-macros`, `-Z collapse-macro-debuginfo` and `#[collapse_debuginfo]`

`-Z debug-macros` is "stabilized" by enabling it by default and removing.

`-Z collapse-macro-debuginfo` is stabilized as `-C collapse-macro-debuginfo`.
It now supports all typical boolean values (`parse_opt_bool`) in addition to just yes/no.

Default value of `collapse_debuginfo` was changed from `false` to `external` (i.e. collapsed if external, not collapsed if local) - rust-lang#100758 (comment) describes some debugging scenarios that motivate this default as reasonable.
`#[collapse_debuginfo]` attribute without a value is no longer supported to avoid guessing the default.

Closes rust-lang#100758
Closes rust-lang#41743
Closes rust-lang#39153

TODO: Stabilization report
@bors
Copy link
Contributor

bors commented Feb 12, 2024

⌛ Trying commit 78ee1bc with merge 575a7f3...

@bors
Copy link
Contributor

bors commented Feb 12, 2024

☀️ Try build successful - checks-actions
Build commit: 575a7f3 (575a7f35a80ab18d54b08775b7bd6cc50ee4e338)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (575a7f3): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.6% [-0.7%, -0.5%] 2
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)
-2.8% [-3.2%, -2.3%] 7
All ❌✅ (primary) -2.4% [-2.4%, -2.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.0% [0.0%, 0.1%] 40
Regressions ❌
(secondary)
0.0% [0.0%, 0.1%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.0% [0.0%, 0.1%] 40

Bootstrap: 663.718s -> 664.389s (0.10%)
Artifact size: 308.26 MiB -> 308.31 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 12, 2024
@petrochenkov
Copy link
Contributor Author

Perf changes are noise.
@bors rollup=maybe

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Feb 12, 2024

Stabilization Report

Before this PR all spans coming from macro definitions were by default collapsed to a single span representing the macro's call site when generating debuginfo.
The option -Z debug-macros changed that behavior and preserved all spans in debuginfo without collapsing.

With this PR the -Z debug-macros option is removed and the defaults are partially swapped.
By default spans are preserved without collapsing, giving more detailed information during debugging, but only for macros coming from the current crate.
For macros from other crates the spans are still collapsed to a single call site span, this way we don't jump to code of println! and other macros from the standard library.

Attributes #[collapse_debuginfo(yes/no/external)] and command line options -C collapse-macro-debuginfo=yes/no/external (replace -Z debug-macros) provide more fine-grained control over collapsing debuginfo spans to call site, when the defaults are insufficient.

History

#41743 identified the problem with detailed debuginfo spans for macros not being available on stable channel.

rust-lang/rfcs#2117 and rust-lang/compiler-team#386 provided the solution for undesirable jumping to sources of println! and friends when detailed spans are enabled.
#99556 implemented that solution in July 2022.

#118903 added detailed tests for the feature and fixed some issues with nested macro calls in December 2023.
#119828 added some more fine grained options to the #[collapse_debuginfo] attribute, and added a command line flag -C collapse-macro-debuginfo for overriding both defaults and attributes as well in January 2024.

Design

rust-lang/compiler-team#386 gives motivation for not collapsing macros in debuginfo in general, but collapsing some macros in particular.

Comments in #100758 (comment) and below describe some possible debugging scenarios, and make a choice for the default collapsing behavior implemented in this PR.

Tests

  • tests/debuginfo/collapse-debuginfo*
  • tests/debuginfo/skip_second_statement*
  • tests/debuginfo/lexical-scope-with-macro.rs

Documentation

src/doc/rustc/src/codegen-options/index.md in this PR and rust-lang/reference#1468 in the reference.

@petrochenkov
Copy link
Contributor Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 12, 2024

Team member @petrochenkov has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 12, 2024
@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-compiler-nominated The issue / PR has been nominated for discussion during a compiler team meeting. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Feb 12, 2024
@petrochenkov
Copy link
Contributor Author

--pass check failed me for the first time in years.
Updated the test.
@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Apr 25, 2024

📌 Commit 98804c1 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2024
debuginfo: Stabilize `-Z debug-macros`, `-Z collapse-macro-debuginfo` and `#[collapse_debuginfo]`

`-Z debug-macros` is "stabilized" by enabling it by default and removing.

`-Z collapse-macro-debuginfo` is stabilized as `-C collapse-macro-debuginfo`.
It now supports all typical boolean values (`parse_opt_bool`) in addition to just yes/no.

Default value of `collapse_debuginfo` was changed from `false` to `external` (i.e. collapsed if external, not collapsed if local) - rust-lang#100758 (comment) describes some debugging scenarios that motivate this default as reasonable.
`#[collapse_debuginfo]` attribute without a value is no longer supported to avoid guessing the default.

Stabilization report: rust-lang#120845 (comment)

Closes rust-lang#100758
Closes rust-lang#41743
Closes rust-lang#39153
@bors
Copy link
Contributor

bors commented Apr 25, 2024

⌛ Testing commit 98804c1 with merge 78aad13...

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 25, 2024
@petrochenkov
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Apr 26, 2024

📌 Commit 683ad6b has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2024
@bors
Copy link
Contributor

bors commented Apr 26, 2024

⌛ Testing commit 683ad6b with merge 6acb9e7...

@bors
Copy link
Contributor

bors commented Apr 26, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6acb9e7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 26, 2024
@bors bors merged commit 6acb9e7 into rust-lang:master Apr 26, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 26, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Apr 26, 2024
debuginfo: Stabilize `-Z debug-macros`, `-Z collapse-macro-debuginfo` and `#[collapse_debuginfo]`

`-Z debug-macros` is "stabilized" by enabling it by default and removing.

`-Z collapse-macro-debuginfo` is stabilized as `-C collapse-macro-debuginfo`.
It now supports all typical boolean values (`parse_opt_bool`) in addition to just yes/no.

Default value of `collapse_debuginfo` was changed from `false` to `external` (i.e. collapsed if external, not collapsed if local) - rust-lang/rust#100758 (comment) describes some debugging scenarios that motivate this default as reasonable.
`#[collapse_debuginfo]` attribute without a value is no longer supported to avoid guessing the default.

Stabilization report: rust-lang/rust#120845 (comment)

Closes rust-lang/rust#100758
Closes rust-lang/rust#41743
Closes rust-lang/rust#39153
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6acb9e7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.4% [3.4%, 3.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.0% [1.0%, 1.0%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.0% [1.0%, 1.0%] 1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.1% [0.0%, 1.0%] 54
Regressions ❌
(secondary)
0.0% [0.0%, 0.1%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [0.0%, 1.0%] 54

Bootstrap: 670.868s -> 670.073s (-0.12%)
Artifact size: 316.00 MiB -> 316.00 MiB (-0.00%)

@ehuss ehuss added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 27, 2024
@philipc
Copy link
Contributor

philipc commented May 3, 2024

This has broken a test in backtrace (rust-lang/backtrace-rs#617). The collapse_debuginfo attribute can be used to fix it.

The debuginfo for line 11 is now line 11 instead of the caller, as expected. However, the result of the line! (called via pos!) is still the caller. Is it intentional that line! is unaffected? It's not clear cut to me how this should work, so I want to check.

@petrochenkov
Copy link
Contributor Author

@philipc
Test suite in this repo has the same test - tests/ui/debuginfo/backtrace-dylib-dep.rs, I updated it to preserve the previous behavior.
The test relied on specific lines being emitted into debuginfo, and since we are changing the debuginfo emitted by default behavior of such tests may also change.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 3, 2024

file!() and line!() are not defined in the current crate, so they are collapsed by default now, unlike pos!() and check!() defined in the current crate.

UPD: As for values returned by file!() and line!(), they follow their own logic independent from debuginfo (defined in compiler\rustc_builtin_macros\src\source_util.rs). They traverse the macro call stack until they encounter root expansion or include! and report the line/column/file at that position.

@workingjubilee
Copy link
Contributor

Breaking the test was fine if it can still be configured to pass. I'm glad that we moved that test into rustc's suite, so that it can be kept current with current defaults. I'm not sure that I want to remove the copy of the test from the backtrace suite yet, but I can just copy petrochenkov's changes to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet