Skip to content

Add tracing_chrome under "tracing" feature #4406

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Stypox
Copy link
Contributor

@Stypox Stypox commented Jun 18, 2025

  • Added a "tracing" feature that enables Chrome traces, and sets Machine::TRACING_ENABLED to true
  • I ended up adding the tracing_chrome crate by copy-pasting this ~600 line file.
    • As discussed previously, depending on the tracing-chrome crate from crates.io is unfortunately not possible since it depends on tracing_core which conflicts with rustc_private's tracing_core (meaning it would not be possible to use the ChromeLayer in a context that expects a Layer from from rustc_private's tracing_core version).
    • I tried to use cargo's [patch] and [replace] sections, but although they would work for normal libraries, they don't seem to behave as expected when the crate to replace comes from rustc_private, see this Zulip comment for a list of experiments
    • Also see more dicussion in this Zulip thread
    • I am open to trying other stuff to avoid copy-pasting foreign code into Miri, but I didn't want to waste more time on this right now, as it's blocking any work on tracing.
    • Do I need to mention the author and license at the top of the copied file, or is a link (like I've done now) enough for attribution?
  • I moved the logger/tracing setup functions out of miri.rs so the file is less cluttered.
    • Should I put those new files under trace/ (like done now) or should I rather move them under bin/, since only bin/miri.rs uses those functions?
  • The ChromeLayer internally starts a thread to write data to file, and thus relies on a guard to properly flush the file and terminate the thread when dropped. Since std::process::exit() was being called in a few places, I had to restructure the code a bit to avoid exiting directly (which wouldn't call drop() on the guard).
    • As far as I understand rustc does not call exit(), but rather raises an unwinding panic which is then caught by rustc_driver::catch_fatal_errors(). After modifying run_compiler_then_exit to not exit directly, I could confirm that the tracing file was being flushed correctly in case of a compiler error by running RUSTC_LOG=1 ./miri run --features tracing FILE_WITH_SYNTAX_ERRORS.
    • I moved the call to init_early_loggers() after argument parsing, is this ok? I did not see any log/trace call during argument parsing anyway. This avoids having to refactor show_error!() to not exit() directly

Stypox added 3 commits June 19, 2025 12:29
The file was taken unmodified from the following link, except for file attributes at the top: https://github.com/thoren-d/tracing-chrome/blob/7e2625ab4aeeef2f0ef9bde9d6258dd181c04472/src/lib.rs
Depending on the tracing-chrome crate from crates.io is unfortunately not possible since it depends on `tracing_core` which conflicts with rustc_private's `tracing_core` (meaning it would not be possible to use the [ChromeLayer] in a context that expects a [Layer] from from rustc_private's `tracing_core` version)
The tracing_chrome Layer has a connected Guard that needs to be dropped before exiting the process, or else data may not be flushed to the tracing file. This required making a few changes to main() to ensure std::process::exit() is not called without making sure to first drop the guard.
@Stypox Stypox marked this pull request as ready for review June 19, 2025 10:32
@Stypox
Copy link
Contributor Author

Stypox commented Jun 19, 2025

I am not sure why tests fail, when I tried locally they failed even on the latest commit on master ( 2f4f9ac ). I did a git bisect and it told me that either ab135f0 or 42f66f4 are the first bad commit (the first one doesn't build, so I skipped it, while the second is the first where ./miri test failed). However this does not explain why the CI on 2f4f9ac passed.

@tiif
Copy link
Member

tiif commented Jun 19, 2025

I am on x86_64-unknown-linux-gnu, ./miri test for current master branch (2f4f9ac) passed locally for me.

May I know what os are you currently using? And is the failure the same as the one in the CI?

@Stypox
Copy link
Contributor Author

Stypox commented Jun 19, 2025

@tiif On my PC the tests that fail run into errors like "error: unsupported operation: extern static __rust_no_alloc_shim_is_unstable is not supported by Miri". Even just running ./miri run tests/pass/hello.rs triggerrs the same error. But the CI exits with code 143 without printing any error (I'm looking at ubuntu-latest):

2025-06-19T10:55:01.6057618Z ##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
2025-06-19T10:55:01.6062974Z ##[error]Process completed with exit code 143.
2025-06-19T10:55:01.6983400Z Cleaning up orphan processes

I am also on x86_64-unknown-linux-gnu, my OS is Manjaro Linux with kernel 5.15.182-1-MANJARO, I have a AMD Ryzen 7 5800H cpu, let me know if you need anything else.

@tiif
Copy link
Member

tiif commented Jun 19, 2025

I suspect the CI failure might be triggered by the change in this PR, as there were successful CI runs after this one in https://github.com/rust-lang/miri/actions. (I haven't read the code closely, so this is just a guess :)

About the unsupported error, I think maybe the toolchain is outdated. You can try to follow the steps here https://github.com/rust-lang/miri/blob/master/CONTRIBUTING.md#preparing-the-build-environment to update it if you haven't done so recently.

If the problem persists, feel free to ask for help in zulip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants