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

bootstrap: Don't override debuginfo-level = 1 to mean line-tables-only #112528

Merged
merged 1 commit into from
Jun 11, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jun 11, 2023

This has real differences in the effective debuginfo: in particular, it omits the module-level information and makes perf less useful (it can't distinguish "self" from "child" time anymore).

Allow passing line-tables-only directly in config.toml instead.

See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/debuginfo.20in.20try.20builds/near/365090631 and https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bsteering.5D.202023-06-09/near/364883519 for more discussion. This effectively reverts the cargo half of #110221 to avoid regressing #60020 again in 1.72.

This has real differences in the effective debuginfo: in particular, it omits the module-level information and breaks perf.

Allow passing `line-tables-only` directly in config.toml instead.
@rustbot
Copy link
Collaborator

rustbot commented Jun 11, 2023

r? @clubby789

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

@jyn514 jyn514 changed the title Don't override debuginfo-level = 1 to mean line-tables-only bootstrap: Don't override debuginfo-level = 1 to mean line-tables-only Jun 11, 2023
@rustbot rustbot added 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) labels Jun 11, 2023
@Mark-Simulacrum
Copy link
Member

This has real differences in the effective debuginfo: in particular, it omits the module-level information and makes perf less useful (it can't distinguish "self" from "child" time anymore).

Seems like this makes the line-table-only debug info much less useful than originally expected. I guess we might want some kind of intermediate "perf-useful" debuginfo which isn't quite 1, presuming there's stuff that's still extra with 1.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 11, 2023

📌 Commit 1236939 has been approved by Mark-Simulacrum

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

Rollup of 3 pull requests

Successful merges:

 - rust-lang#112487 (Update documentation for `tools` defaults)
 - rust-lang#112513 (Dont compute `opt_suggest_box_span` span for TAIT)
 - rust-lang#112528 (bootstrap: Don't override `debuginfo-level = 1` to mean `line-tables-only`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c1f2da5 into rust-lang:master Jun 11, 2023
@bors
Copy link
Contributor

bors commented Jun 11, 2023

⌛ Testing commit 1236939 with merge 37998ab...

@rustbot rustbot added this to the 1.72.0 milestone Jun 11, 2023
@jyn514 jyn514 deleted the fix-debuginfo-level branch June 12, 2023 09:24
Comment on lines +76 to +85
StringOrInt::Int(n) => {
let other = serde::de::Unexpected::Signed(n);
return Err(D::Error::invalid_value(other, &"expected 0, 1, or 2"));
}
StringOrInt::String(s) => {
let other = serde::de::Unexpected::Str(s);
return Err(D::Error::invalid_value(
other,
&"expected none, line-tables-only, limited, or full",
));
Copy link

Choose a reason for hiding this comment

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

Hm, it looks like 1 error message must be, not 2

return Err(D::Error::invalid_value(other, &"expected 0 or 1, 2, none, line-tables-only, limited, full"));

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't understand this comment. at this point we already know the config is a string and not a number.

Copy link

@VitWW VitWW Jun 12, 2023

Choose a reason for hiding this comment

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

Deserializing into DebuginfoLevel could be either Int or String.
You wrote one message if Int is out of scope and different message if String is out of scope.
But it must be one same message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants