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.

Sign up for GitHub

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

Jump to bottom

Add a dedicated debug-logging option to config.toml #76588

Merged
merged 3 commits into from
Sep 13, 2020
Merged

Add a dedicated debug-logging option to config.toml #76588

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15aa6f3
add debug-logging to config.toml
guswynn Sep 10, 2020
56f5c7f
comments + add max_level_info so false works with debug_assertions on
guswynn Sep 11, 2020
0be66d7
just max_level_info
guswynn Sep 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ features = ['unprefixed_malloc_on_supported_platforms']
[features]
jemalloc = ['jemalloc-sys']
llvm = ['rustc_driver/llvm']
release_max_level_info = ['rustc_driver/release_max_level_info']
3 changes: 2 additions & 1 deletion compiler/rustc_driver/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ crate-type = ["dylib"]

[dependencies]
libc = "0.2"
tracing = { version = "0.1.18", features = ["release_max_level_info"] }
tracing = { version = "0.1.18" }
tracing-subscriber = { version = "0.2.10", default-features = false, features = ["fmt", "env-filter", "smallvec", "parking_lot", "ansi"] }
rustc_middle = { path = "../rustc_middle" }
rustc_ast_pretty = { path = "../rustc_ast_pretty" }
Expand Down Expand Up @@ -38,3 +38,4 @@ winapi = { version = "0.3", features = ["consoleapi", "debugapi", "processenv"]

[features]
llvm = ['rustc_interface/llvm']
release_max_level_info = ['tracing/release_max_level_info', 'tracing/max_level_info']
guswynn marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 8 additions & 2 deletions config.toml.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,13 +321,19 @@
# binary, otherwise they are omitted.
#
# Defaults to rust.debug value
#debug-assertions = false
#debug-assertions = debug

# Whether or not debug assertions are enabled for the standard library.
# Overrides the `debug-assertions` option, if defined.
#
# Defaults to rust.debug-assertions value
#debug-assertions-std = false
#debug-assertions-std = debug-assertions

# Whether or not to leave debug! and trace! calls in the rust binary.
# Overrides the `debug-assertions` option, if defined.
#
# Defaults to rust.debug-assertions value
Copy link
Member

Choose a reason for hiding this comment

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

Hm, so this brings up two interesting questions:

  • Should we default to this being on? If you're doing a lot of rustc development, it can certainly be helpful to have debug logs, but it can also be a pretty annoying slowdown (IIRC, benchmarks show 5-10%).
  • Show we tie to to the old way of turning it on, i.e., debug-assertions?

cc @jyn514, we might want an MCP or similar here to figure out what people think good defaults would be.

Copy link
Member

@jyn514 jyn514 Sep 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think it should start as off-by-default. There was a lot of discussion in #76588 about enabling logging and it was almost a 10% perf hit on some benchmarks: https://perf.rust-lang.org/compare.html?start=22e6099330cde0e7b1529774fe27874f8326de7a&end=ededceef5651050bda1a2058af270da498bb1333.

That said, I would love to get the perf impact low enough that this can be on by default (or even always on).

Copy link
Member

Choose a reason for hiding this comment

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

That said, I would love to get the perf impact low enough that this can be on by default (or even always on).

One possible way to do this is to separate debug! from trace! and have that be opt-in instead. AFAIK most of the compiler uses debug! currently, but I separate the two in rustdoc and it's worked very well. In particular, collect_intra_doc_links prints ludicrous amounts of info with trace but only ~15 lines or so per module with debug: https://github.com/rust-lang/rust/blob/master/src/librustdoc/passes/collect_intra_doc_links.rs#L647

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is implemented now, it should be off by default, and on wit debug-assertions, which matched exactly the semantics of the current config.toml

#debug-logging = debug-assertions

# Debuginfo level for most of Rust code, corresponds to the `-C debuginfo=N` option of `rustc`.
# `0` - no debug info
Expand Down
6 changes: 6 additions & 0 deletions src/bootstrap/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ pub struct Config {
pub rust_codegen_units_std: Option<u32>,
pub rust_debug_assertions: bool,
pub rust_debug_assertions_std: bool,
pub rust_debug_logging: bool,
pub rust_debuginfo_level_rustc: u32,
pub rust_debuginfo_level_std: u32,
pub rust_debuginfo_level_tools: u32,
Expand Down Expand Up @@ -381,6 +382,7 @@ struct Rust {
codegen_units_std: Option<u32>,
debug_assertions: Option<bool>,
debug_assertions_std: Option<bool>,
debug_logging: Option<bool>,
debuginfo_level: Option<u32>,
debuginfo_level_rustc: Option<u32>,
debuginfo_level_std: Option<u32>,
Expand Down Expand Up @@ -591,6 +593,7 @@ impl Config {
let mut debug = None;
let mut debug_assertions = None;
let mut debug_assertions_std = None;
let mut debug_logging = None;
let mut debuginfo_level = None;
let mut debuginfo_level_rustc = None;
let mut debuginfo_level_std = None;
Expand Down Expand Up @@ -634,6 +637,7 @@ impl Config {
debug = rust.debug;
debug_assertions = rust.debug_assertions;
debug_assertions_std = rust.debug_assertions_std;
debug_logging = rust.debug_logging;
debuginfo_level = rust.debuginfo_level;
debuginfo_level_rustc = rust.debuginfo_level_rustc;
debuginfo_level_std = rust.debuginfo_level_std;
Expand Down Expand Up @@ -737,6 +741,8 @@ impl Config {
config.rust_debug_assertions_std =
debug_assertions_std.unwrap_or(config.rust_debug_assertions);

config.rust_debug_logging = debug_logging.unwrap_or(config.rust_debug_assertions);

let with_defaults = |debuginfo_level_specific: Option<u32>| {
debuginfo_level_specific.or(debuginfo_level).unwrap_or(if debug == Some(true) {
1
Expand Down
10 changes: 10 additions & 0 deletions src/bootstrap/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,16 @@ impl Build {
if self.config.llvm_enabled() {
features.push_str(" llvm");
}

// If debug logging is on, then we want the default for tracing:
// https://github.com/tokio-rs/tracing/blob/3dd5c03d907afdf2c39444a29931833335171554/tracing/src/level_filters.rs#L26
// which is everything (including debug/trace/etc.)
// if its unset, if debug_assertions is on, then debug_logging will also be on
// as well as tracing *ignoring* this feature when debug_assertions is on
if !self.config.rust_debug_logging {
features.push_str(" release_max_level_info");
}

features
}

Expand Down