Skip to content

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

Merged
bors merged 2 commits into
rust-lang:masterfrom
petrochenkov:debmac
Apr 26, 2024
Merged

debuginfo: Stabilize -Z debug-macros, -Z collapse-macro-debuginfo and #[collapse_debuginfo]#120845
bors merged 2 commits into
rust-lang:masterfrom
petrochenkov:debmac

Conversation

@petrochenkov

@petrochenkov petrochenkov commented Feb 9, 2024

Copy link
Copy Markdown
Contributor

-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

rustbot commented Feb 9, 2024

Copy link
Copy Markdown
Collaborator

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

rustbot commented Feb 9, 2024

Copy link
Copy Markdown
Collaborator

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

@petrochenkov

Copy link
Copy Markdown
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, or significant enough to need a team 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
Copy Markdown
Member

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

@petrochenkov

Copy link
Copy Markdown
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

bors commented Feb 12, 2024

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Feb 12, 2024

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Perf changes are noise.
@bors rollup=maybe

@petrochenkov

petrochenkov commented Feb 12, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

@rfcbot fcp merge

@rfcbot

rfcbot commented Feb 12, 2024

Copy link
Copy Markdown

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 I-compiler-nominated 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, or significant enough to need a team FCP to proceed. labels Feb 12, 2024
@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
Copy Markdown
Contributor Author

@bors r=oli-obk

@bors

bors commented Apr 26, 2024

Copy link
Copy Markdown
Collaborator

📌 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

bors commented Apr 26, 2024

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Apr 26, 2024

Copy link
Copy Markdown
Collaborator

☀️ 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
@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
Copy Markdown
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

ghost commented May 3, 2024

Copy link
Copy Markdown
Contributor

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

ghost commented May 3, 2024

Copy link
Copy Markdown
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

ghost commented May 3, 2024

Copy link
Copy Markdown
Contributor Author

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

ghost commented May 6, 2024

Copy link
Copy Markdown
Member

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

Projects

None yet