Skip to content

Conversation

Stypox
Copy link
Contributor

@Stypox Stypox commented Jul 22, 2025

This PR contains a few misc fixes for the tracing code that was recently added:

@rustbot ready

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2025

Thank you for contributing to Miri!
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Jul 22, 2025
// so they appear a separate trace line in trace visualization tools, see
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.jh64i9l3vwa1
Some(span.id().into_u64())
Some(span.id().into_u64() as i64)
Copy link
Member

Choose a reason for hiding this comment

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

This might wrap around -- isn't that a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's by design, I tested and Perfetto accepts 64-bit 2s complement signed integers (i.e. from -2^63 to 2^63-1), so wrapping around is intended

Copy link
Member

Choose a reason for hiding this comment

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

Then please call cast_signed instead, and ideally add a comment saying that perfetto is fine with negative numbers that for in an i64, but not fine with numbers above i64::MAX. Bare as casts are always sus.

Copy link
Member

Choose a reason for hiding this comment

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

Then please call cast_signed instead

This is still unresolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I didn't see the comment. Fixed now.

@Stypox Stypox force-pushed the misc-tracing-fixes branch from a8e42fa to 58d8b16 Compare July 23, 2025 13:16
@Stypox
Copy link
Contributor Author

Stypox commented Jul 23, 2025

I improved the markdown comment and also included info about why we cast u64 to i64 with wraparound

@Stypox Stypox force-pushed the misc-tracing-fixes branch from 58d8b16 to 0a8a3a1 Compare July 23, 2025 14:43
Stypox added 3 commits July 25, 2025 09:22
Perfetto gives an error if an id does not fit in an 64-bit signed integer in 2's complement.
@Stypox Stypox force-pushed the misc-tracing-fixes branch from 0a8a3a1 to 8f58f19 Compare July 25, 2025 07:23
@RalfJung RalfJung enabled auto-merge July 25, 2025 07:33
@RalfJung RalfJung added this pull request to the merge queue Jul 25, 2025
Merged via the queue into rust-lang:master with commit 199669b Jul 25, 2025
14 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Jul 25, 2025
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.

3 participants