Allow named debuginfo options in Cargo.toml #11958

jyn514 · 2023-04-11T05:15:55Z

What does this PR try to resolve?

Rustc supports these since rust-lang/rust#109808. It's technically possible to set a named debuginfo level through RUSTFLAGS, but in practice cargo always passes its own opinion of what the debuginfo level is, so allow it to be configured through cargo too so the options don't conflict.

Note that this also adds support for "none", "limited", and "full" as aliases for 0, 1, and 2 respectively. The motivation is not just to match rustc, but also to make it easier for us to change the default for 1 over an edition to mean "line-tables-only" instead of "limited". If we allow limited as an alias for 1, people will be able to use it before and after the change in defaults to mean the same thing; if we disallow it and only allow it in the new edition, it will be impossible to have the same code work with both new and old versions of cargo.

How should we test and review this PR?

All modified tests are normal tests that can be run with cargo test. You can also build cargo from source and verify that CARGO_PROFILE_DEV_DEBUG=line-tables-only cargo build on a hello world project correctly passes -C debuginfo=line-tables-only through to rustc.

I recommend viewing this with whitespace changes hidden.

Additional information

Enabling these flags in cargo is required before bootstrap can start passing them through to rustc (which I'd like to do so we can reduce the size of the generated artifacts: rust-lang/rust#104968 (comment)).

rustbot · 2023-04-11T05:16:00Z

r? @weihanglo

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

tests/testsuite/unit_graph.rs

Muscraft

Thanks for this!

src/cargo/util/toml/mod.rs

Muscraft · 2023-04-11T17:39:32Z

This is slightly more lenient than it needs to be for that use-case; it will allow things like debug = "true" and debug = "1" that were disallowed before (treating them the same as debug = true and debug = 1 respectively). I can make cargo's parsing more restrictive here if you like.

I don't know anywhere (currently) that "true" is parsed to true and I am unsure if this is something that cargo should support.

Rustc supports these since rust-lang/rust#109808. It's technically possible to set a named debuginfo level through `RUSTFLAGS`, but in practice cargo always passes its own opinion of what the debuginfo level is, so allow it to be configured through cargo too.

jyn514 · 2023-04-11T20:55:03Z

Thanks for the feedback! I ended up redoing the main data structure from an arbitrary InternedString to an enum, which hopefully is both easier to read and avoids mistakes from doing string comparisons. I also changed the parsing to be more targeted and disallow "1" and "true".

Muscraft

Can you update the docs for debug in profile?

Muscraft · 2023-04-12T15:02:57Z

src/cargo/util/toml/mod.rs

+                    "none" => TomlDebugInfo::None,
+                    "limited" => TomlDebugInfo::Limited,
+                    "full" => TomlDebugInfo::Full,


It seems like this PR is about adding "line-tables-only" and "line-directives-only". If this PR is trying to get cargo to match rustc, do you mind updating the PR description with the rational for this

I updated the PR description. The main reason is that I want to change 1 to mean line-tables-only in the next edition.

I opened an MCP for that change: https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/2024.3A.20Decrease.20debuginfo.20generated.20by.20.20.60-.E2.80.A6.20compiler-team.23613. I think allowing these aliases won't be too much of a maintenance burden for cargo even if it ends up not happening, though.

src/cargo/util/toml/mod.rs

src/cargo/core/profiles.rs

- Update the documentation and doc-comments - Improve the error message for parsing Cargo.toml

Muscraft · 2023-04-14T22:22:11Z

Just a heads up, I will be bringing this up in the next Cargo team meeting because I am unsure of how stabilization will look.

If we were to put this behind a nightly flag, it would mean this is stable in rustc and unstable for cargo. If we make this insta-stable, we would have to live with the (minimal) Toml changes. No matter what, we will be at least a version behind rustc as this probably won't make it in the 1.70.0 merge window.

Muscraft · 2023-04-19T22:26:14Z

This was discussed in the recent Cargo team meeting, and there were no particular objections to instant stabilization since it is already stabilized in rustc. That being said, we would like a few more tests for the new options (nightly only for now) and to clarify the documentation, particularly so users have the context for deciding what value they should use.

jyn514 · 2023-04-20T00:43:40Z

I've pushed some more tests.

@Muscraft can you clarify what you're looking for in the documentation? Right now this matches the documentation for rustc, is there something specific you're looking for?

If it helps, here's an example backtrace with each of the new options, compared to limited:

none (missing line numbers, missing frames)

; CARGO_PROFILE_RELEASE_DEBUG=none RUST_BACKTRACE=1 ${CARGO_TARGET_DIR}/debug/cargo run --release -q
thread 'main' panicked at 'explicit panic', src/main.rs:6:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/std/src/panicking.rs:578:5
   1: core::panicking::panic_fmt
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:67:14
   2: core::panicking::panic
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:117:5
   3: example::foo
   4: example::main

line-directives-only (same as none)

; CARGO_PROFILE_RELEASE_DEBUG=line-directives-only RUST_BACKTRACE=1 ${CARGO_TARGET_DIR}/debug/cargo run --release -q
thread 'main' panicked at 'explicit panic', src/main.rs:6:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/std/src/panicking.rs:578:5
   1: core::panicking::panic_fmt
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:67:14
   2: core::panicking::panic
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:117:5
   3: example::foo
   4: example::main

line-tables-only (missing module qualifiers for some functions)

; CARGO_PROFILE_RELEASE_DEBUG=line-tables-only RUST_BACKTRACE=1 ${CARGO_TARGET_DIR}/debug/cargo run --release -q
thread 'main' panicked at 'explicit panic', src/main.rs:6:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/std/src/panicking.rs:578:5
   1: core::panicking::panic_fmt
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:67:14
   2: core::panicking::panic
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:117:5
   3: example::foo
             at ./src/main.rs:6:5
   4: bar
             at ./src/main.rs:11:9
   5: main
             at ./src/main.rs:2:5
   6: call_once<fn(), ()>
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/ops/function.rs:250:5

limited

; CARGO_PROFILE_RELEASE_DEBUG=limited RUST_BACKTRACE=1 ${CARGO_TARGET_DIR}/debug/cargo run --release -q
thread 'main' panicked at 'explicit panic', src/main.rs:6:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/std/src/panicking.rs:578:5
   1: core::panicking::panic_fmt
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:67:14
   2: core::panicking::panic
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/panicking.rs:117:5
   3: example::foo
             at ./src/main.rs:6:5
   4: example::inner::bar
             at ./src/main.rs:11:9
   5: example::main
             at ./src/main.rs:2:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/7908a1d65496b88626e4b7c193c81d777005d6f3/library/core/src/ops/function.rs:250:5

but I'm not sure how to condense that into a short sentence. The backtraces at levels lower than limited also vary between debug and release mode: when optimizations are disabled line-tables-only behaves the same as limited and none has the same frames as limited but is missing the file/column info.

Muscraft

@jyn514 and I discussed this and concluded that Cargo's docs should be kept relatively simple and link to Rust's docs on debuginfo if more information is needed.

src/doc/src/reference/profiles.md

Muscraft

Everything looks good!

Muscraft · 2023-04-22T15:07:53Z

Thanks!

@bors r+

bors · 2023-04-22T15:07:55Z

📌 Commit dbab134 has been approved by Muscraft

It is now in the queue for this repository.

bors · 2023-04-22T15:08:11Z

⌛ Testing commit dbab134 with merge 77069b4...

ehuss · 2023-04-22T15:18:07Z

Would it be possible to add some tests that actually use the new options? Since it isn't stable on rustc, yet, they will need to be marked with #[cargo_test(nightly, reason = "debug options stabilized in 1.70")].

bors · 2023-04-22T15:51:27Z

☀️ Test successful - checks-actions
Approved by: Muscraft
Pushing 77069b4 to master...

Update cargo 17 commits in de80432f04da61d98dcbbc1572598071718ccfd2..9e586fbd8b931494067144623b76c37d213b1ab6 2023-04-21 13:18:32 +0000 to 2023-04-25 22:09:11 +0000 - Update home dependency (rust-lang/cargo#12037) - Warn instead of error in `cargo package` on empty `readme` or `license-file` in manifest (rust-lang/cargo#12036) - Clarify documentation around test target setting. (rust-lang/cargo#12032) - fix: apply `[env]` to target info discovery rustc (rust-lang/cargo#12029) - CI: ensure intra links for all members are checked (rust-lang/cargo#12025) - chore: make credential dependencies platform-specific (rust-lang/cargo#12027) - CI: use `-p` to specify workspace members instead of `--manifest-path` (rust-lang/cargo#12024) - ci: requires `test_gitoxide` and `lockfile` for both bors success and failure (rust-lang/cargo#12026) - Update windows-sys (rust-lang/cargo#12021) - Bump libc to 0.2.142 (rust-lang/cargo#12014) - Update openssl-src to 111.25.3+1.1.1t (rust-lang/cargo#12005) - Improve error message for empty dep (rust-lang/cargo#12001) - Remove wrong url in benchsuite manifest. (rust-lang/cargo#12020) - Bump versions of local crates (rust-lang/cargo#12019) - Add the Win32_System_Console feature since it is used (rust-lang/cargo#12016) - Update outdated crates.io URLs in publishing guide (rust-lang/cargo#12018) - Allow named debuginfo options in Cargo.toml (rust-lang/cargo#11958) r? `@ghost`

Test that the new `debuginfo` options match between cargo and rustc As requested in #11958 (comment). r? `@ehuss`

Update docs for artifact JSON debuginfo levels. String-based debuginfo levels were added in #11958, but the documentation wasn't updated to reflect that.

rustbot assigned weihanglo Apr 11, 2023

jyn514 commented Apr 11, 2023

View reviewed changes

tests/testsuite/unit_graph.rs Outdated Show resolved Hide resolved

xxchan mentioned this pull request Apr 11, 2023

Use debug=line-tables-only risingwavelabs/risingwave#9107

Closed

Muscraft requested changes Apr 11, 2023

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

jyn514 force-pushed the named-debuginfo branch from e353f2e to d861dcf Compare April 11, 2023 20:06

Allow none/limited/full as aliases for 0-2

3dbb474

jyn514 mentioned this pull request Apr 12, 2023

Greatly decrease the size of rustc_driver.so when debuginfo is enabled rust-lang/rust#110221

Merged

Muscraft reviewed Apr 12, 2023

View reviewed changes

rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Apr 12, 2023

jyn514 force-pushed the named-debuginfo branch from 806570f to b56d230 Compare April 12, 2023 16:40

Address review comments

8a9db7a

- Update the documentation and doc-comments - Improve the error message for parsing Cargo.toml

jyn514 force-pushed the named-debuginfo branch from b56d230 to 8a9db7a Compare April 12, 2023 17:05

Muscraft added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Apr 14, 2023

weihanglo removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Apr 18, 2023

add more debuginfo tests

a982bcc

jyn514 force-pushed the named-debuginfo branch from 4452cc3 to a982bcc Compare April 20, 2023 01:01

jyn514 mentioned this pull request Apr 20, 2023

2024: Decrease debuginfo generated by -Cdebuginfo=1 rust-lang/compiler-team#613

Closed

3 tasks

Muscraft reviewed Apr 21, 2023

View reviewed changes

src/doc/src/reference/profiles.md Outdated Show resolved Hide resolved

weihanglo reviewed Apr 21, 2023

View reviewed changes

src/doc/src/reference/profiles.md Outdated Show resolved Hide resolved

Use relative links in documentation

dbab134

jyn514 force-pushed the named-debuginfo branch from 5a2f6e5 to dbab134 Compare April 22, 2023 00:05

Muscraft approved these changes Apr 22, 2023

View reviewed changes

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 Apr 22, 2023

bors merged commit 77069b4 into rust-lang:master Apr 22, 2023

jyn514 mentioned this pull request Apr 23, 2023

Test that the new debuginfo options match between cargo and rustc #12022

Merged

jyn514 deleted the named-debuginfo branch April 23, 2023 22:18

weihanglo mentioned this pull request Apr 25, 2023

Update cargo rust-lang/rust#110834

Merged

ehuss added this to the 1.71.0 milestone May 5, 2023

weihanglo mentioned this pull request May 21, 2023

Inconsistent handling of debug = 0 vs debug = false in profiles #12163

Closed

bors added a commit that referenced this pull request May 24, 2023

Auto merge of #12022 - jyn514:debuginfo-tests, r=weihanglo

092db78

Test that the new `debuginfo` options match between cargo and rustc As requested in #11958 (comment). r? `@ehuss`

This was referenced May 30, 2023

Announcing Rust 1.70.0 rust-lang/blog.rust-lang.org#1110

Merged

Don't distinguish Debuginfo::None and Debuginfo::Explicit(None) #12205

Merged

ehuss mentioned this pull request Jul 19, 2023

Update docs for artifact JSON debuginfo levels. #12376

Merged

bors added a commit that referenced this pull request Jul 19, 2023

Auto merge of #12376 - ehuss:artifact-json-docs, r=epage

499296e

Update docs for artifact JSON debuginfo levels. String-based debuginfo levels were added in #11958, but the documentation wasn't updated to reflect that.

epage mentioned this pull request Oct 12, 2023

Enable strip in release mode by default #4122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow named debuginfo options in Cargo.toml #11958

Allow named debuginfo options in Cargo.toml #11958

jyn514 commented Apr 11, 2023 •

edited

Loading

rustbot commented Apr 11, 2023

Muscraft left a comment

Muscraft commented Apr 11, 2023

jyn514 commented Apr 11, 2023 •

edited

Loading

Muscraft left a comment

Muscraft Apr 12, 2023

jyn514 Apr 12, 2023 •

edited

Loading

jyn514 Apr 12, 2023 •

edited

Loading

Muscraft commented Apr 14, 2023 •

edited

Loading

Muscraft commented Apr 19, 2023

jyn514 commented Apr 20, 2023 •

edited

Loading

Muscraft left a comment

Muscraft left a comment

Muscraft commented Apr 22, 2023

bors commented Apr 22, 2023

bors commented Apr 22, 2023

ehuss commented Apr 22, 2023

bors commented Apr 22, 2023

Allow named debuginfo options in Cargo.toml #11958

Allow named debuginfo options in Cargo.toml #11958

Conversation

jyn514 commented Apr 11, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Apr 11, 2023

Muscraft left a comment

Choose a reason for hiding this comment

Muscraft commented Apr 11, 2023

jyn514 commented Apr 11, 2023 • edited Loading

Muscraft left a comment

Choose a reason for hiding this comment

Muscraft Apr 12, 2023

Choose a reason for hiding this comment

jyn514 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

jyn514 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Muscraft commented Apr 14, 2023 • edited Loading

Muscraft commented Apr 19, 2023

jyn514 commented Apr 20, 2023 • edited Loading

Muscraft left a comment

Choose a reason for hiding this comment

Muscraft left a comment

Choose a reason for hiding this comment

Muscraft commented Apr 22, 2023

bors commented Apr 22, 2023

bors commented Apr 22, 2023

ehuss commented Apr 22, 2023

bors commented Apr 22, 2023

jyn514 commented Apr 11, 2023 •

edited

Loading

jyn514 commented Apr 11, 2023 •

edited

Loading

jyn514 Apr 12, 2023 •

edited

Loading

jyn514 Apr 12, 2023 •

edited

Loading

Muscraft commented Apr 14, 2023 •

edited

Loading

jyn514 commented Apr 20, 2023 •

edited

Loading