Skip to content

Port backtrace's cpp_smoke_test to rustc repo #136182

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

Closed
wants to merge 1 commit into from

Conversation

Pyr0de
Copy link
Contributor

@Pyr0de Pyr0de commented Jan 28, 2025

Ported backtrace-rs test crates cpp_smoke_test to rust repo.

issue #122899

@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fmease (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Jan 28, 2025
@jieyouxu
Copy link
Member

Maybe r? @workingjubilee (or @ChrisDenton)

@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@jieyouxu
Copy link
Member

Right. r? @ChrisDenton (or idk someone else who's more familiar with backtrace)

@rustbot rustbot assigned ChrisDenton and unassigned fmease Jan 29, 2025
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is intentionally testing C++ so it will need to be a cpp file. Also the backtrace feature cpp_demangle will need to be enabled for the test.

Though looking at this further, it seems like it's been "temporarily" ignored in backtrace-rs for eight years? https://github.com/rust-lang/backtrace-rs/blob/e33eaac8caf46d0d3cc57f8d152529e8b7ae1b78/crates/cpp_smoke_test/tests/smoke.rs#L11
Ah it seems that the reason it was ignored has long since been fixed: gimli-rs/cpp_demangle#73

@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 Jan 30, 2025
@Pyr0de
Copy link
Contributor Author

Pyr0de commented Jan 30, 2025

Hey,
Can I add this test to tests/run-make?

and how can I use the feature cpp_demangle?
I doesn't seem to be present in std::backtrace

@Pyr0de
Copy link
Contributor Author

Pyr0de commented Jan 31, 2025

Hey @ChrisDenton,
I do not see any way to use the feature cpp_demangle.
There is also no function exposed by std::backtrace that can be used to get the demangled function name from the frame. BacktraceFrames

If there is some work around to get the demangled name, please do let me know

@ChrisDenton
Copy link
Member

@Pyr0de I mean you can just use run_make_support::cargo() with a --manifest-path argument set to the cpp_smoke_test/Cargo.toml. That should take care of setting features, etc.

@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 1, 2025

Oh, thanks for the suggestion
I'll give it a shot

@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Feb 1, 2025
@Pyr0de Pyr0de marked this pull request as ready for review February 1, 2025 16:15
@rustbot
Copy link
Collaborator

rustbot commented Feb 1, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 1, 2025

The test is mostly the same from cpp_smoke_test, just made some changes for it to successfully run and added the rmake.rs file

@rustbot review

@rustbot rustbot 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 Feb 1, 2025
@Pyr0de Pyr0de requested a review from ChrisDenton February 3, 2025 15:12
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only comment on the rmake.rs test file itself, not on the actual test logic.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me if @jieyouxu is happy with the rmake test

@jieyouxu
Copy link
Member

jieyouxu commented Feb 5, 2025

I'll read this test again a bit later

@jieyouxu jieyouxu self-assigned this Feb 5, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Feb 5, 2025

Oh this is slightly problematic because now the run-make test is calling out to cargo test which is its own test runnner wrapper on top of libtest, just like compiletest, which is very strange. This should be able to be a plain rmake.rs without cargo being involved. I'll take a stab tmrw at slightly rewriting this test.

Generally speaking, using cargo inside rmake.rs to build something (like a nested crate in a run-make test) is fine, but using cargo test inside rmake.rs is undesirable because it's really really opaque and another thicc layer of indirection in testing. (Notably, we're already run by libtest by the time the test recipe binary is executed!)

The current formulation of the test is also exercising the crates.io version of some backtrace, not necessarily the backtrace shipped alongside std.

@jieyouxu
Copy link
Member

jieyouxu commented Feb 7, 2025

So I had an attempt over at #136695 to not depend on cargo test. Along the way I found out this test is quite problematic. It's super fragile and sensitive to opt levels and debuginfo level of the C++ file in conjunction with the rust file, and the test (or at least the assertion) does not seem to work on msvc at all.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
Port `rust-lang/backtrace`'s `cpp_smoke_test` to `rust-lang/rust` test suite

Ports https://github.com/rust-lang/backtrace-rs/tree/e33eaac8caf46d0d3cc57f8d152529e8b7ae1b78/crates/cpp_smoke_test to the main repo.

I can't say I'm 100% convinced by the value of this test living in the main repo for several reasons:

1. This test (or at least the actual `cpp_smoke_test.rs` is **extremely** fragile. It's sensitive to debuginfo levels AND optimizations. On `x86_64-unknown-linux-gnu`, the C++ file **must** be compiled with **exactly** `-O1`. No more, no less, or else the backtrace symbol names do not line up in symbol count or name.
2. I could not get this test to work on Windows MSVC, no matter which combination of debuginfo levels and optimization levels I tried.

Note that I have not tried other targets, e.g. Apple friends, which may fail for yet other reasons.

Possibly supersedes rust-lang#136182.

r? `@ChrisDenton` (or `@workingjubilee)`

try-job: x86_64-apple-1
try-job: aarch64-apple
@workingjubilee
Copy link
Member

Given its difficulties, I have struck the test from the list of tests we want to port.

@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 8, 2025

Will this test also fail like how @jieyouxu's test failed?

@jieyouxu
Copy link
Member

jieyouxu commented Feb 8, 2025

Will this test also fail like how @jieyouxu's test failed?

Yes, provided how sensitive this test is to debuginfo-level + opt-level + platform/env for the combination of the C++ lib + Rust binary. It didn't work on msvc no matter what configuration I tried, nor does it work on apple targets where I ran a try-job.

@Pyr0de I'm going to close this and the other attempt over at #136695 based on how fragile this test is + its difficulty to implement versus the value it may or may not provide. Thank you for taking a shot at this though, and while this didn't work out, I find discovering what doesn't work here to be equally valuable.

@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 8, 2025

Even though it didn't work, thanks for all the help with this PR

@Pyr0de Pyr0de deleted the cpp_smoke_test branch February 22, 2025 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants