Revert "Work around invalid DWARF bugs for fat LTO"

[cbiffle]

Since September, the toolchain has not been generating reliable DWARF
information for static variables when LTO is on. This has affected
projects in the embedded space where the use of LTO is typical. In our
case, it has kept us from bumping past the 2021-09-22 nightly toolchain
lest our debugger break. This has been a pretty dramatic regression for
people using debuggers and static variables. See #90357 for more info
and a repro case.

This commit is a mechanical revert of
d5de680 from PR #89041, which caused
the issue. (Note on that PR that the commit's author has requested it be
reverted.)

I have locally verified that this fixes#90357 by restoring the
functionality of both the repro case I posted on that bug, and debugger
behavior on real programs. There do not appear to be test cases for this
in the toolchain; if I've missed them, point me at 'em and I'll update
them.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information.

[compiler-errors]

r? @pnkfelix since you self-assigned the attached issue (and I know very little about codegen)

[jyn514]

@cbiffle do you think you could add tests for this in src/test/debuginfo so it doesn't regress again?

[cbiffle]

@cbiffle do you think you could add tests for this in src/test/debuginfo so it doesn't regress again?

Happy to give it a shot! A modification to basic-types-globals.rs looks like it'd suffice -- that test is not currently being run with lto on, and if it were, I believe it'd be failing.

Is it worth attempting to matrix the debug tests across different build settings so that there aren't omissions like this?

[cbiffle]

So, there were some tests that would have caught this, except that (1) they were "temporarily" disabled in 2018 and (2) they were using the wrong build configuration. Altering basic-types-globals.rs by turning it back on and enabling LTO produces a test that fails on current master, for two reasons:

• Because its assertions about GDB's output appear to have rotted in the four years it's been disabled, and
• Because of the bug described in DWARF info for static vars in lib crates has stopped being produced reliably in LTO builds #90357

I've added a commit to this PR that re-enables the test and adds a duplicate of it using LTO. Both tests pass with this PR; the LTO test fails without it, reproducing the bug.

I admit I don't understand the circumstances that led to these tests being disabled in #47155, so I don't know for sure that simply turning them back on and fixing the decay is the right answer.

Also, if there is a way to matrix a single test case in multiple build configurations rather than copy-pasta, please let me know.

[cbiffle]

Also, for what it's worth, I've managed to update and re-enable 16 debuginfo tests, some simply by turning them back on, some by updating rotten assertions about GDB's output. It looks like there's been some regression in lldb's output in some cases, unrelated to rustc's DWARF generation, so in some cases I've left them disabled for lldb. If there's interest in this change I can create a followup PR.

[jyn514]

If there's interest in this change I can create a followup PR.

That sounds amazing!

[compiler-errors]

Also, if there is a way to matrix a single test case in multiple build configurations rather than copy-pasta, please let me know.

@cbiffle: Look at revisions, though you might need to check out other tests for best examples for how to use it.

[cbiffle]

@cbiffle: Look at revisions, though you might need to check out other tests for best examples for how to use it.

Slick. That's almost exactly what I wanted. Unfortunately, any attempt to use revisions in a gdbtest hits a panic: https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/runtest.rs#L758

[cbiffle]

If there's interest in this change I can create a followup PR.

That sounds amazing!

Posted as a draft in #95696 . It's based on top of this PR for my convenience. If it seems like this PR is going to be controversial I can rebase and separate them.

[cbiffle]

Hi! Is anybody looking at this? Can I do anything to facilitate? Thanks!

[pnkfelix]

Hi! Is anybody looking at this? Can I do anything to facilitate? Thanks!

Hi @cbiffle , I'm looking at it now. Hopefully will merge soon.

[pnkfelix]

This commit is a mechanical revert ofhttps://github.com/rust-lang/rust/commit/d5de680e20def848751cb3c11e1182408112b1d3 from PR #89041, which caused
the issue. (Note on that PR that the commit's author has requested it be
reverted.)

This is pretty easy justification for a revert here. :)

Thanks @cbiffle !

@bors r+ rollup

[bors]

📌 Commit 42af197 has been approved by pnkfelix

[GrantM11235]

What is the status of this PR? Does bors need to try again?

[jyn514]

@GrantM11235 it failed the multiplatform tests - someone (presumably @cbiffle, although other people can take it over if they don't have time) needs to figure out why and fix the tests.

[cbiffle]

@jyn514 I'd like to help however I can, but I don't understand the failure these hit.

[cbiffle]

@pnkfelix or anyone else, do you understand the build failure here? We're getting increasingly far behind the rust-embedded ecosystem because they're using features that became available after DWARF generation broke, whereas we require DWARF generation. I'd like to do what I can to move this along but my fix works on the platforms I have access to.

[nikic]

The build failure is because the newly reenabled tests fail. It looks like #95138 also tried to reenable gdb tests, but also ran into failures. Most likely, these don't work under gdb 7, but work under some newer version. You could try adding a min-gdb-version annotation.

[JohnCSimon]

Ping from triage:
@cbiffle - can you post your status on this PR?

[luqmana]

@rustbot ready

Spoke w/ @cbiffle to help get this landed.

I updated the test to include a minimum GDB version. The CI job linked above that failed was using 7.11.1-0ubuntu1~16.5. I did confirm that it also fails locally with 7.12.1 but passes starting with 8.0.

While this does mean it won't be run on some of the CI machines, we already have a number of tests that specify a minimum GDB version of anywhere from 7.7 to 10.1. This should still give us some coverage (e.g. src/test/debuginfo/rc_arc.rs is marked as min-gdb-version: 8.1 and is successfully run as part of auto (x86_64-gnu-stable, x86_64-gnu, stable, nightly, ubuntu-20.04-xl)).

[jyn514]

@bors r=pnkfelix rollup=never

Thanks!

[bors]

📌 Commit 2085d6a has been approved by pnkfelix

It is now in the queue for this repository.

[bors]

⌛ Testing commit 2085d6a with merge 1d91b6bba8269b811ffc115121616b3d0d2ef43f...

[Mark-Simulacrum]

@bors retry prioritizing #99288

[rust-log-analyzer]

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

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

[bors]

⌛ Testing commit 2085d6a with merge e6c43cf...

[bors]

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing e6c43cf to master...

[rust-timer]

Finished benchmarking commit (e6c43cf): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	3.2%	3.6%	2
Regressions 😿 (secondary)	2.9%	2.9%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.2%	3.6%	2

Cycles

Results

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: 🎉 relevant improvement found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.4%	-3.4%	1
Improvements 🎉 (secondary)	-2.7%	-2.7%	1
All 😿🎉 (primary)	-3.4%	-3.4%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change↩↩²

2. number of relevant changes↩↩²