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 print unsupported split-debuginfo modes with -Zunstable-options #112109

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

Alexendoo
Copy link
Member

@Alexendoo Alexendoo commented May 30, 2023

Currently unsupported split-debuginfo options are enabled by -Zunstable-options, for projects that have -Zunstable-options for other reasons this can be an unexpected interaction

This PR makes it so that --print split-debuginfo -Zunstable-options doesn't print unsupported modes, so that a cargo config of e.g.

[profile.dev]
split-debuginfo = "unpacked"

Would not cause an unsupported mode to be enabled on x86_64-pc-windows-msvc

@rustbot
Copy link
Collaborator

rustbot commented May 30, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 30, 2023
@rust-log-analyzer

This comment has been minimized.

@Alexendoo Alexendoo force-pushed the unsupported-split-debuginfo branch from 791d4c7 to 1ae38dc Compare May 31, 2023 22:06
@klensy
Copy link
Contributor

klensy commented Jun 1, 2023

Looks related rust-lang/cargo@64a1f20

@b-naber
Copy link
Contributor

b-naber commented Jun 12, 2023

Sorry for the late reply.

I don't really know what this is about (should probably re-assign this), but I'm wondering why would anybody ever use this option, i.e. why would it make sense to enable an unsupported mode? Also would anybody ever rely on unsupported modes through -Zunstable-options?

@Alexendoo
Copy link
Member Author

I don't really know what this is about (should probably re-assign this), but I'm wondering why would anybody ever use this option, i.e. why would it make sense to enable an unsupported mode? Also would anybody ever rely on unsupported modes through -Zunstable-options?

I thought the same initially and went to just remove the behaviour, but there are some tests that explicitly use the unsupported modes so I was no longer sure

Looks related rust-lang/cargo@64a1f20

Should be unrelated to that change, cargo is doing the right thing as it asks rustc what modes are supported with --print=split-debuginfo, but -Zunstable-options --print=split-debuginfo prints all the modes even if they aren't supported

We could change just the --print part and keep the other behaviour, that would mean cargo configs with profile.dev.split-debuginfo = "unpacked" with -Zunstable-options would no longer use an unsupported mode on some platforms, but it could still be opted into explicitly through rustflags. I'll go ahead and implement that since it would mean no new flag is needed

@Alexendoo Alexendoo force-pushed the unsupported-split-debuginfo branch from 1ae38dc to fda3c9f Compare June 13, 2023 12:00
@Alexendoo Alexendoo changed the title Add -Z unsupported-split-debuginfo Don't print unsupported split-debuginfo modes with -Zunstable-options Jun 13, 2023
@b-naber
Copy link
Contributor

b-naber commented Jun 13, 2023

I don't really know what this is about (should probably re-assign this), but I'm wondering why would anybody ever use this option, i.e. why would it make sense to enable an unsupported mode? Also would anybody ever rely on unsupported modes through -Zunstable-options?

I thought the same initially and went to just remove the behaviour, but there are some tests that explicitly use the unsupported modes so I was no longer sure

Can you say which tests those were?

@Alexendoo
Copy link
Member Author

This was the main one, there was also this arm in compiletest but I don't remember if that was hit in CI or not

@b-naber
Copy link
Contributor

b-naber commented Jun 13, 2023

The changes look reasonable to me, but I'm not sure I fully grasp the implications of this change. So the change only prevents unsupported split-debuginfo options being printed in print_crate_info. How does cargo or any other crate that directly relies on rustc use this information?

@Alexendoo
Copy link
Member Author

Cargo calls rustc --print=split-debuginfo to determine if a mode should be enabled, on x86_64-pc-windows-msvc that'll result in

$ rustc --print split-debuginfo
packed

So with the config

[profile.dev]
split-debuginfo = "unpacked"

It will determine that unpacked isn't supported and so won't pass -C split-debuginfo=unpacked to the rustc commands used for compiling/checking

When you have -Zunstable-options in RUSTFLAGS/a cargo config like we do in Clippy though the result is different

$ rustc --print split-debuginfo -Zunstable-options
off
packed
unpacked

Cargo then thinks rustc supports unpacked and will pass -C split-debuginfo=unpacked, even though msvc does not properly support it. After this change it would only print packed as above

@b-naber
Copy link
Contributor

b-naber commented Jun 13, 2023

Thanks for the explanation. So it only uses those options for validation? And it doesn't currently error when an option provided in the toml file isn't supported on that machine?

If it doesn't error, does it just generate debugging information that is "unusable" on that machine?

@Alexendoo
Copy link
Member Author

If you have an unsupported (as determined through rustc --print split-debuginfo) mode in the config cargo will ignore it, you get the default mode for the platform

There's no error/warning from cargo

Copy link
Contributor

@b-naber b-naber left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying this. This seems like the correct fix then. Though I do wonder why this was introduced in the first place then.

Could you maybe open a PR to remove the following line from the split-debuginfo section on this page: https://doc.rust-lang.org/rustc/codegen-options/index.html#split-debuginfo

Attempting to use an unsupported option requires using the nightly channel with the -Z unstable-options flag.

@b-naber
Copy link
Contributor

b-naber commented Jun 14, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 14, 2023

📌 Commit fda3c9f has been approved by b-naber

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 Jun 14, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#98202 (Implement `TryFrom<&OsStr>` for `&str`)
 - rust-lang#107619 (Specify behavior of HashSet::insert)
 - rust-lang#109814 (Stabilize String::leak)
 - rust-lang#111974 (Update runtime guarantee for `select_nth_unstable`)
 - rust-lang#112109 (Don't print unsupported split-debuginfo modes with `-Zunstable-options`)
 - rust-lang#112506 (Properly check associated consts for infer placeholders)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 38ed4e5 into rust-lang:master Jun 14, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 14, 2023
@Alexendoo Alexendoo deleted the unsupported-split-debuginfo branch September 2, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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