Add support for SpanTrace capture in ICE reports #103993
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
T-bootstrap
yaahc
commented
Nov 4, 2022
This comment has been minimized.
This comment has been minimized.
oli-obk
requested changes
Nov 5, 2022
| Ok(env) => EnvFilter::new(env), | ||
| _ => EnvFilter::default().add_directive(Directive::from(LevelFilter::WARN)), | ||
| }; | ||
|
|
||
| let error_filter = match env::var(ice_env) { |
There was a problem hiding this comment.
I wonder if we should just enable all levels by default for the spantrace. if people see it as a problem we can always offer ways to reduce it. My use case would always unconditionally enable trace spans for ICEs.
There was a problem hiding this comment.
I was just worried about causing perf issues, but that seems easy enough to test
There was a problem hiding this comment.
Alright, so I tried this a while back and ran into some errors and ended up putting it down. Picked it back up, rebased, dealt with merge conflicts and got it working again and looked at the error in some more detail with @jyn514 and we came to the conclusion that it seems like it's a pre-existing error that I'm triggering but not causing. Jyn felt like they recall encountering the same error in the past.
I want to do some more testing to verify this theory and see if I can repro the issue without having tracing_error installed by enabling TRACE capture on all spans with a custom registry layer. I recall testing this in november and it works at trace level when reporting errors during compilation of other crates, it's only when it's compiling itself with trace enabled on spans that it's running into a panic from a diagnostic not being emitted in time.
There was a problem hiding this comment.
oh, then let's land it as is, so we can debug it on master.
yaahc
commented
Nov 8, 2022
src/tools/tidy/src/deps.rs
Outdated
Comment on lines
258
to
277
| "windows-sys", | ||
| "windows_aarch64_msvc", | ||
| "windows_i686_gnu", | ||
| "windows_i686_msvc", | ||
| "windows_x86_64_gnu", | ||
| "windows_x86_64_msvc", |
There was a problem hiding this comment.
Okay I don't understand where these deps came from. I'm pretty sure there's gotta be some pebkac going on rn but I don't get how these produced an error how, they appear to have already been part of the Cargo.toml
Gonna try removing...
There was a problem hiding this comment.
parking_lot 0.12 now depends on windows-sys. Note that this list here is only for dependencies of the standard library and rustc, not for other tools despite the fact that those use the same lockfile.
This comment has been minimized.
This comment has been minimized.
oli-obk
reviewed
Nov 9, 2022
compiler/rustc_log/src/lib.rs
Outdated
|
|
||
| let subscriber = tracing_subscriber::Registry::default().with(filter).with(layer); | ||
| let subscriber = tracing_subscriber::Registry::default() | ||
| .with(error_layer.with_filter(error_filter)) |
There was a problem hiding this comment.
this layer seems to be affecting the log layer. Not sure what's going on.
There was a problem hiding this comment.
they interact with the same strings in the registry in the background, some of the code ended up being shared, so that may be causing it. I can try reversing their order so the log layer gets applied first and see how that affects things.
https://github.com/tokio-rs/tracing/blob/master/tracing-error/src/subscriber.rs#L52-L59
|
☔ The latest upstream changes (presumably #104591) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
rustbot
removed
the
T-bootstrap
rust-cloud-vms
bot
force-pushed
the
traced-ICE
branch
from
b3ec1ea
to
30ba7e2
Compare
|
These commits modify the If this was intentional then you can ignore this comment. |
rust-cloud-vms
bot
force-pushed
the
traced-ICE
branch
from
30ba7e2
to
6ad0d5e
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
reviewed
Feb 13, 2023
|
Some tests need blessing, but the actually failing ones look like a |
rust-cloud-vms
bot
force-pushed
the
traced-ICE
branch
from
c6b101b
to
2746c97
Compare
yaahc
commented
Mar 9, 2023
| @@ -1,6 +1,7 @@ | |||
| // edition:2021 | |||
| // known-bug: unknown | |||
| // unset-rustc-env:RUST_BACKTRACE | |||
| // rustc-env:RUSTC_ICE_LOG=trace | |||
There was a problem hiding this comment.
I wanted to see one that actually captured some SpanTrace output just to make sure it's working and i understood why the other ones are empty.
I'm going to add some logic to skip adding the "SpanTrace:\n" bit if the SpanTrace itself is empty. I'm also wondering if I should add some cfg directives to set the RUSTC_ICE_LOG level to trace when we're not building rustc itself but I need to investigate how that would work.
There was a problem hiding this comment.
This test suddenly started failing after rebasing on master and I do not yet know why
I tried building latest master to see if it was a bug that just happened to get through and it failed two entirely unrelated tests instead...
yaahc
commented
Mar 9, 2023
compiler/rustc_log/src/lib.rs
Outdated
| Ok(env) => tracing::Level::from_str(&env).unwrap(), | ||
| _ => tracing::Level::WARN, | ||
| }; | ||
| #[cfg(not(bootstrap))] |
There was a problem hiding this comment.
I tried doing some shortcuts to see if I could make it work the way I want prior to figuring our the right solution. This attempts to work around the issue with compiling the stage1 compiler with RUSTC_ICE_LOG=TRACE by only setting that default for the resulting stage1 binary, which would then compile the tests with that as the default level and produce nice SpanTraces in the tests below.
It didn't work. I'm investigating now.
yaahc
commented
Mar 10, 2023
| @@ -638,6 +638,7 @@ impl Session { | |||
| || self.opts.unstable_opts.unpretty.is_some() | |||
| || self.opts.output_types.contains_key(&OutputType::Mir) | |||
| || std::env::var_os("RUSTC_LOG").is_some() | |||
| || std::env::var_os("RUSTC_ICE_LOG").is_some() | |||
There was a problem hiding this comment.
@jyn514 identified the issue with defaulting the log level to TRACE as likely related to (and worked around by) this bit of logic. I don't know if we will want the default output to start to include SpanTraces or not but I'm leaning towards defaulting them to off for now. We can always bump Span levels up to WARN if they're super relevant and we want them to show up all the time.
This way we can leave actually increasing the usage of this functionality to later PRs where it can be based on more practical usage experience.
|
☔ The latest upstream changes (presumably #104833) made this pull request unmergeable. Please resolve the merge conflicts. |
let me push damnit! we meet again rustfmt enable trace by default checkpoint while figuring out this damn rebase reduce log level to avoid bugs in logging remove accidentally readded fn remove unused deps bless these initially, deal with actual output next add example with spantrace output try to hack around the issue to get nice output with proper spantrace output add helper message for spantrace
rust-cloud-vms
bot
force-pushed
the
traced-ICE
branch
from
c89e428
to
8057e62
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #106610) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@yaahc any updates on this? |
|
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change results in output similar to this gist: https://gist.github.com/yaahc/db2dca7b34c499ab15d721a518b2f2b2
r? oli-obk