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

Don't leave debug locations for constants sitting on the builder indefinitely #130052

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

khuey
Copy link
Contributor

@khuey khuey commented Sep 6, 2024

Because constants are currently emitted before the prologue, leaving the debug location on the IRBuilder spills onto other instructions in the prologue and messes up both line numbers as well as the point LLVM chooses to be the prologue end.

Example LLVM IR (irrelevant IR elided):
Before:

define internal { i64, i64 } @_ZN3tmp3Foo18var_return_opt_try17he02116165b0fc08cE(ptr align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8, !dbg !357
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)

After:

define internal { i64, i64 } @_ZN3tmp3Foo18var_return_opt_try17h00b17d08874ddd90E(ptr align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)

Note in particular how !357 from %residual.dbg.spill's dbg_declare no longer falls through onto the store to %self.dbg.spill. This fixes argument values at entry when the constant is a ZST (e.g. <Option as Try>::Residual). This fixes #130003 (but note that it does not fix issues with argument values and non-ZST constants, which emit their own stores that have debug info on them, like #128945).

r? @michaelwoerister

…finitely.

Because constants are currently emitted *before* the prologue, leaving the
debug location on the IRBuilder spills onto other instructions in the prologue
and messes up both line numbers as well as the point LLVM chooses to be the
prologue end.

Example LLVM IR (irrelevant IR elided):
Before:

define internal { i64, i64 } @_ZN3tmp3Foo18var_return_opt_try17he02116165b0fc08cE(ptr align 8 %self) !dbg !347 {
start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8, !dbg !357
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)

After:

define internal { i64, i64 } @_ZN3tmp3Foo18var_return_opt_try17h00b17d08874ddd90E(ptr align 8 %self) !dbg !347 {
start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)

Note in particular how !357 from %residual.dbg.spill's dbg_declare no longer
falls through onto the store to %self.dbg.spill. This fixes argument values
at entry when the constant is a ZST (e.g. <Option as Try>::Residual). This
fixes rust-lang#130003 (but note that it does *not* fix issues with argument values and
non-ZST constants, which emit their own stores that have debug info on them,
like rust-lang#128945).
@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 Sep 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@michaelwoerister
Copy link
Member

Thanks, @khuey! Looks good to me.
@bors r+

@bors
Copy link
Contributor

bors commented Sep 11, 2024

📌 Commit 7ed9f94 has been approved by michaelwoerister

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 Sep 11, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
…-emission, r=michaelwoerister

Don't leave debug locations for constants sitting on the builder indefinitely

Because constants are currently emitted *before* the prologue, leaving the debug location on the IRBuilder spills onto other instructions in the prologue and messes up both line numbers as well as the point LLVM chooses to be the prologue end.

Example LLVM IR (irrelevant IR elided):
Before:
```
define internal { i64, i64 } `@_ZN3tmp3Foo18var_return_opt_try17he02116165b0fc08cE(ptr` align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8, !dbg !357
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)
```
After:
```
define internal { i64, i64 } `@_ZN3tmp3Foo18var_return_opt_try17h00b17d08874ddd90E(ptr` align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)
```
Note in particular how !357 from %residual.dbg.spill's dbg_declare no longer falls through onto the store to %self.dbg.spill. This fixes argument values at entry when the constant is a ZST (e.g. `<Option as Try>::Residual`). This fixes rust-lang#130003 (but note that it does *not* fix issues with argument values and non-ZST constants, which emit their own stores that have debug info on them, like rust-lang#128945).

r? `@michaelwoerister`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…kingjubilee

Rollup of 14 pull requests

Successful merges:

 - rust-lang#129260 (Don't suggest adding return type for closures with default return type)
 - rust-lang#129520 (Suggest the correct pattern syntax on usage of unit variant pattern for a struct variant)
 - rust-lang#129696 (update stdarch)
 - rust-lang#129759 (Stabilize `const_refs_to_static`)
 - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86)
 - rust-lang#129866 (Clarify documentation labelling and definitions for std::collections)
 - rust-lang#130052 (Don't leave debug locations for constants sitting on the builder indefinitely)
 - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS)
 - rust-lang#130123 (Report the `note` when specified in `diagnostic::on_unimplemented`)
 - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - rust-lang#130206 (Map `WSAEDQUOT` to `ErrorKind::FilesystemQuotaExceeded`)
 - rust-lang#130207 (Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`)
 - rust-lang#130219 (Fix false positive with `missing_docs` and `#[test]`)
 - rust-lang#130221 (Make SearchPath::new public)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
…-emission, r=michaelwoerister

Don't leave debug locations for constants sitting on the builder indefinitely

Because constants are currently emitted *before* the prologue, leaving the debug location on the IRBuilder spills onto other instructions in the prologue and messes up both line numbers as well as the point LLVM chooses to be the prologue end.

Example LLVM IR (irrelevant IR elided):
Before:
```
define internal { i64, i64 } ``@_ZN3tmp3Foo18var_return_opt_try17he02116165b0fc08cE(ptr`` align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8, !dbg !357
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)
```
After:
```
define internal { i64, i64 } ``@_ZN3tmp3Foo18var_return_opt_try17h00b17d08874ddd90E(ptr`` align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)
```
Note in particular how !357 from %residual.dbg.spill's dbg_declare no longer falls through onto the store to %self.dbg.spill. This fixes argument values at entry when the constant is a ZST (e.g. `<Option as Try>::Residual`). This fixes rust-lang#130003 (but note that it does *not* fix issues with argument values and non-ZST constants, which emit their own stores that have debug info on them, like rust-lang#128945).

r? ``@michaelwoerister``
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#119286 (show linker output even if the linker succeeds)
 - rust-lang#129103 (Don't warn empty branches unreachable for now)
 - rust-lang#129696 (update stdarch)
 - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86)
 - rust-lang#129992 (Update compiler-builtins to 0.1.125)
 - rust-lang#130052 (Don't leave debug locations for constants sitting on the builder indefinitely)
 - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS)
 - rust-lang#130114 (Remove needless returns detected by clippy in the compiler)
 - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - rust-lang#130168 (maint: update docs for change_time ext and doc links)
 - rust-lang#130239 (miri: fix overflow detection for unsigned pointer offset)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2024
…-emission, r=michaelwoerister

Don't leave debug locations for constants sitting on the builder indefinitely

Because constants are currently emitted *before* the prologue, leaving the debug location on the IRBuilder spills onto other instructions in the prologue and messes up both line numbers as well as the point LLVM chooses to be the prologue end.

Example LLVM IR (irrelevant IR elided):
Before:
```
define internal { i64, i64 } ```@_ZN3tmp3Foo18var_return_opt_try17he02116165b0fc08cE(ptr``` align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8, !dbg !357
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)
```
After:
```
define internal { i64, i64 } ```@_ZN3tmp3Foo18var_return_opt_try17h00b17d08874ddd90E(ptr``` align 8 %self) !dbg !347 { start:
  %self.dbg.spill = alloca [8 x i8], align 8
  %_0 = alloca [16 x i8], align 8
  %residual.dbg.spill = alloca [0 x i8], align 1
    #dbg_declare(ptr %residual.dbg.spill, !353, !DIExpression(), !357)
  store ptr %self, ptr %self.dbg.spill, align 8
    #dbg_declare(ptr %self.dbg.spill, !350, !DIExpression(), !358)
```
Note in particular how !357 from %residual.dbg.spill's dbg_declare no longer falls through onto the store to %self.dbg.spill. This fixes argument values at entry when the constant is a ZST (e.g. `<Option as Try>::Residual`). This fixes rust-lang#130003 (but note that it does *not* fix issues with argument values and non-ZST constants, which emit their own stores that have debug info on them, like rust-lang#128945).

r? ```@michaelwoerister```
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#129367 (Fix default/minimum deployment target for Aarch64 simulator targets)
 - rust-lang#129992 (Update compiler-builtins to 0.1.125)
 - rust-lang#130052 (Don't leave debug locations for constants sitting on the builder indefinitely)
 - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - rust-lang#130160 (Fix `slice::first_mut` docs)
 - rust-lang#130250 (Fix `clippy::useless_conversion`)
 - rust-lang#130252 (Properly report error on `const gen fn`)
 - rust-lang#130256 (Re-run coverage tests if `coverage-dump` was modified)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Sep 13, 2024

⌛ Testing commit 7ed9f94 with merge 5e84295...

@bors
Copy link
Contributor

bors commented Sep 13, 2024

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 5e84295 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 13, 2024
@bors bors merged commit 5e84295 into rust-lang:master Sep 13, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 13, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5e84295): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -3.2%, secondary -2.1%)

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)
-3.2% [-3.5%, -2.9%] 2
Improvements ✅
(secondary)
-2.1% [-3.6%, -1.2%] 4
All ❌✅ (primary) -3.2% [-3.5%, -2.9%] 2

Cycles

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

Binary size

Results (primary -0.2%, secondary -0.4%)

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)
-0.2% [-0.6%, -0.0%] 50
Improvements ✅
(secondary)
-0.4% [-1.1%, -0.1%] 15
All ❌✅ (primary) -0.2% [-0.6%, -0.0%] 50

Bootstrap: 756.26s -> 757.063s (0.11%)
Artifact size: 341.29 MiB -> 341.17 MiB (-0.03%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

1.71: Bad DW_TAG_lexical_block; gdb breaks on method before self is initialized
5 participants