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

Fix line numbers for MIR inlined code #103071

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

wesleywiser
Copy link
Member

should_collapse_debuginfo detects if the specified span is part of a
macro expansion however it does this by checking if the span is anything
other than a normal (non-expanded) kind, then the span sequence is
walked backwards to the root span.

This doesn't work when the MIR inliner inlines code as it creates spans
with expansion information set to ExprKind::Inlined and results in the
line number being attributed to the inline callsite rather than the
normal line number of the inlined code.

Fixes #103068

`should_collapse_debuginfo` detects if the specified span is part of a
macro expansion however it does this by checking if the span is anything
other than a normal (non-expanded) kind, then the span sequence is
walked backwards to the root span.

This doesn't work when the MIR inliner inlines code as it creates spans
with expansion information set to `ExprKind::Inlined` and results in the
line number being attributed to the inline callsite rather than the
normal line number of the inlined code.
@wesleywiser wesleywiser added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-mir-opt-inlining Area: MIR inlining labels Oct 14, 2022
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2022
@wesleywiser
Copy link
Member Author

Comparison of panics from the test case before & after this change:

Before

> .\mir-inlined-line-numbers.exe
thread 'main' panicked at 'explicit panic', .\src\test\codegen\mir-inlined-line-numbers.rs:8:5
stack backtrace:
   0: std::panicking::begin_panic<str>
             at /rustc/6b3ede3f7bc502eba7bbd202b4b9312d812adcd7\library\std\src\panicking.rs:607
   1: mir_inlined_line_numbers::bar
             at .\src\test\codegen\mir-inlined-line-numbers.rs:8
   2: mir_inlined_line_numbers::foo
             at .\src\test\codegen\mir-inlined-line-numbers.rs:12
   3: mir_inlined_line_numbers::main
             at .\src\test\codegen\mir-inlined-line-numbers.rs:12
   4: core::ops::function::FnOnce::call_once
             at /rustc/6b3ede3f7bc502eba7bbd202b4b9312d812adcd7\library\core\src\ops\function.rs:251
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

After

> .\mir-inlined-line-numbers.exe
thread 'main' panicked at 'explicit panic', .\src\test\codegen\mir-inlined-line-numbers.rs:8:5
stack backtrace:
   0: std::panicking::begin_panic<str>
             at .\library\std\src\panicking.rs:607
   1: mir_inlined_line_numbers::bar
             at .\src\test\codegen\mir-inlined-line-numbers.rs:8
   2: mir_inlined_line_numbers::foo
             at .\src\test\codegen\mir-inlined-line-numbers.rs:3
   3: mir_inlined_line_numbers::main
             at .\src\test\codegen\mir-inlined-line-numbers.rs:12
   4: core::ops::function::FnOnce::call_once
             at .\library\core\src\ops\function.rs:251
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@wesleywiser
Copy link
Member Author

r? @davidtwco

As I think you might be familiar with this code 🙂

@rust-highfive rust-highfive assigned davidtwco and unassigned jackh726 Oct 19, 2022
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2022

📌 Commit 34d90a4 has been approved by davidtwco

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 Oct 26, 2022
@pnkfelix
Copy link
Member

discussed with Wesley. Unilaterally backport approving to go into 1.65 beta branch (which will be stable next week).

@pnkfelix pnkfelix added beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Oct 27, 2022
@wesleywiser
Copy link
Member Author

@bors p=1

We're backporting this to ride the train alongside the MIR inliner being enabled by default.

@bors
Copy link
Contributor

bors commented Oct 28, 2022

⌛ Testing commit 34d90a4 with merge 9db8c2939a620326d47daadbc8d36abd1cdebb1a...

@bors
Copy link
Contributor

bors commented Oct 28, 2022

💔 Test failed - checks-actions

@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 Oct 28, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@wesleywiser
Copy link
Member Author

2022-10-28T12:51:18.7606620Z sort: write failed: 'standard output': Broken pipe
2022-10-28T12:51:18.7607419Z 257932	.
2022-10-28T12:51:18.7608062Z sort: write error

Seems spurious?

@bors retry

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

bors commented Oct 28, 2022

⌛ Testing commit 34d90a4 with merge 77e7b74...

@bors
Copy link
Contributor

bors commented Oct 28, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 77e7b74 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 28, 2022
@bors bors merged commit 77e7b74 into rust-lang:master Oct 28, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 28, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (77e7b74): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)
1.0% [0.6%, 2.2%] 8
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.0% [0.6%, 2.2%] 8

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)
3.5% [2.7%, 4.4%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) 3.5% [2.7%, 4.4%] 2

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)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

@rustbot rustbot added the perf-regression Performance regression. label Oct 28, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.66.0, 1.65.0 Oct 29, 2022
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 29, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 29, 2022
…mulacrum

[beta] backport rollup

* poll_fn and Unpin: fix pinning rust-lang#102737
* Support raw-dylib functions being used inside inlined functions rust-lang#102988
* Fix line numbers for MIR inlined code rust-lang#103071
* Add architectures to fn create_object_file rust-lang#103240
* Add eval hack in super_relate_consts back rust-lang#103279
* Mark std::os::wasi::io::AsFd etc. as stable. rust-lang#103308
* Truncate thread names on Linux and Apple targets rust-lang#103379
* Do not consider repeated lifetime params for elision. rust-lang#103450
* rustdoc: add missing URL redirect rust-lang#103588
* Remove commit_if_ok probe from NLL type relation rust-lang#103601

Also includes a copy of the release notes.

r? `@ghost`
@nnethercote
Copy link
Contributor

These perf regressions look real.

@wesleywiser
Copy link
Member Author

I'll spend some time investigating this week.

@wesleywiser
Copy link
Member Author

The regressions are real but expected. Both regex and webrender enable debuginfo generation in release mode. Prior to this change, any inlined code was being incorrectly placed at the line number of the calling function which caused a reduction in the amount of debuginfo generated by LLVM (and given to LLVM by rustc). Fixing that bug causes the debuginfo to be generated with the appropriate line numbers and results in more debuginfo. cachegrind results back this up and show the biggest increases in debuginfo related LLVM functions.

Both of these benchmarks improved considerably when MIR inlining was enabled and even with the regression caused by this fix, we're still showing net wins on these benchmarks.

I couldn't find any other actionable items that might improve these results at this time.

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…, r=davidtwco

Fix line numbers for MIR inlined code

`should_collapse_debuginfo` detects if the specified span is part of a
macro expansion however it does this by checking if the span is anything
other than a normal (non-expanded) kind, then the span sequence is
walked backwards to the root span.

This doesn't work when the MIR inliner inlines code as it creates spans
with expansion information set to `ExprKind::Inlined` and results in the
line number being attributed to the inline callsite rather than the
normal line number of the inlined code.

Fixes rust-lang#103068
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-mir-opt-inlining Area: MIR inlining beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIR inlining causes incorrect line numbers for inlined code