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

Reenable debuginfo tests #95696

Closed

Conversation

cbiffle
Copy link
Contributor

@cbiffle cbiffle commented Apr 5, 2022

These tests were disabled four years ago; it appears that it was
intended to be temporary.

In that time their assertions have rotted. I've gone through and fixed
the assertions where necessary and rehabilitated them. In several cases
I've had to leave lldb disabled, because the lldb output has changed in
ways that I don't understand. I figured gdb coverage was better than no
coverage.

It's worth noting that these tests being enabled would probably have
caught the regression observed in #90357.

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 rust-lang#90357 for more info
and a repro case.

This commit is a mechanical revert of
d5de680 from PR rust-lang#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 rust-lang#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.
- Re-enabled basic-types-globals which has been disabled since 2018
- Updated its now-rotted assertions about GDB's output to pass
- Rewrote header comment describing some previous state of GDB behavior
  that didn't match either the checked-in assertions _or_ the current
  behavior, and so I assume has just been wrong for some time.
- Copy-pasta'd the test into a version that uses LTO to check for
  regression on rust-lang#90357, because I don't see a way to matrix the same
  test into several build configurations.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2022
@rust-highfive
Copy link
Collaborator

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2022
@cbiffle
Copy link
Contributor Author

cbiffle commented Apr 5, 2022

(More context available in #95685, fwiw.)

@cbiffle
Copy link
Contributor Author

cbiffle commented Apr 5, 2022

Welp. One of these tests appears to segfault rustc under certain circumstances. Guess I'll turn it back off.

Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 136 tests
i........i..i..i.......ii.i........F......i.............iii....ii.......i.......i................... 100/136
Some tests failed in compiletest suite=debuginfo mode=debuginfo host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
..i.................................
failures:

---- [debuginfo-gdb] debuginfo/drop-locations.rs stdout ----
NOTE: compiletest thinks it is using GDB with native rust support

error: compilation failed!
status: signal: 11 (core dumped)
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/debuginfo/drop-locations.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/drop-locations.gdb/a" "-Crpath" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-g" "-O" "-C" "no-prepopulate-passes" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/drop-locations.gdb/auxiliary"
stdout: none
stderr: none



failures:
    [debuginfo-gdb] debuginfo/drop-locations.rs

test result: FAILED. 119 passed; 1 failed; 16 ignored; 0 measured; 0 filtered out; finished in 4.09s

These tests were disabled four years ago; it appears that it was
intended to be temporary.

In that time their assertions have rotted. I've gone through and fixed
the assertions where necessary and rehabilitated them. In several cases
I've had to leave lldb disabled, because the lldb output has changed in
ways that I don't understand. I figured gdb coverage was better than no
coverage.

It's worth noting that these tests being enabled would probably have
caught the regression observed in rust-lang#90357.
@lcnr
Copy link
Contributor

lcnr commented Apr 12, 2022

hey 👋 is there anything to review right now? If so please comment r? rust-lang/compiler to reassign this PR as I don't know much about the state of debug info in Rust.

@lcnr lcnr 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 Apr 12, 2022
@cbiffle
Copy link
Contributor Author

cbiffle commented Apr 12, 2022

Sorry, this is based on another PR (#95685) that I thought was headed for review. Without it, the tests don't pass. I'll update when that's in.

@wesleywiser
Copy link
Member

Oh, excellent! I started working on re-enabling many of these as well in #94489 but I haven't had much time lately to work on that. I'm happy to review & approve your changes instead!

r? @wesleywiser

@rust-highfive rust-highfive assigned wesleywiser and unassigned lcnr Apr 14, 2022
@cbiffle
Copy link
Contributor Author

cbiffle commented Apr 14, 2022

Oh, excellent! I started working on re-enabling many of these as well in #94489 but I haven't had much time lately to work on that. I'm happy to review & approve your changes instead!

r? @wesleywiser

Thanks! The PR to fix the debuginfo generation has been approved, once that's rolled up I'll rebase this and set it to ready (and CC you).

@camelid
Copy link
Member

camelid commented Aug 2, 2022

triage: Hi @cbiffle, it looks like the fix PR you mentioned has been merged. Is this just in need of a rebase and review?

@cbiffle
Copy link
Contributor Author

cbiffle commented Aug 2, 2022

triage: Hi @cbiffle, it looks like the fix PR you mentioned has been merged. Is this just in need of a rebase and review?

Oh hi! Yes, I believe that is correct. Lemme set the ready bit.

@cbiffle cbiffle marked this pull request as ready for review August 2, 2022 23:02
@camelid
Copy link
Member

camelid commented Aug 3, 2022

Hi, I think this might need a rebase first! (Unless you feel that a review first would be helpful.)

Either way, please then post a comment with @rustbot ready to mark your PR as ready for a review. This changes the label, which is what reviewers usually look for.

@JohnCSimon
Copy link
Member

Triage - doesn't seem to be in ready state? Not sure what's causing this

@cbiffle cbiffle marked this pull request as ready for review last month

@rustbot ready - setting it to ready anyway

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2022

Error: The "Ready" shortcut only works on pull requests.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@JohnCSimon
Copy link
Member

JohnCSimon commented Sep 11, 2022

???
@rustbot ready

ok, setting it manually and I'll follow up with #t-infra

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2022

Error: The "Ready" shortcut only works on pull requests.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 11, 2022
@wesleywiser
Copy link
Member

@cbiffle do you want rebase out 98190b7 since that was already merged in #95685?

@apiraino
Copy link
Contributor

apiraino commented Oct 26, 2022

Switching to waiting on PR author if I read this comment correctly. @cbiffle Feel free to request a review with @rustbot ready, thanks!

@rustbot author

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

@cbiffle
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jan 1, 2023
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

8 participants