v9 format: Increase StringId and Addr size to u64, fixing ICEs during self-profiling

[smklein]

This PR introduces a new "v9" format for profdata files, which uses u64s for addressing into files instead of u32s. This avoids ICEs which were possible with large traces, mentioned in the attached issues.

Fixes: #214, rust-lang/rust#99282

---

This is my first contribution to this repo, so I've made sure that tests are passing, but I'm interested in ensuring the following work:

• I originally encountered this issue from Seeing ICE due to u32 overflow #214 -- I'd love to get advice on "how to rebuild the self-profiling feature using this code" to ensure this change works end-to-end too. EDIT: I did this, it works! No longer seeing ICEs on large profile traces.
• I see that there are compatibility tests with v7 and v8 formats (e.g. can_read_v8_profdata_files) -- I'd be interested in creating one for v9 too, but I'm not actually that familiar with "how to generate a new .mm_profdata file" without the whole toolchain built to help me. I'm interested in fixing this before merging, but would appreciate pointers! EDIT: This is done -- I added a v9 format test after tracing a minimal rust binary created via cargo new.

[michaelwoerister]

Thanks a lot for the PR, @smklein! I'll try to review next week. I'll need to page in how most of this works too 🙂

[michaelwoerister]

I'd love to get advice on "how to rebuild the self-profiling feature using this code" to ensure this change works end-to-end too.

Do you mean: how can one get tools and a Rust compiler with the measureme version in this PR?

If so, the following steps should do the trick:

• Build the workspace in this repo to get a local build of all the tools.
• Check out a recent version of the Rust compiler and make sure you can build it locally.
• Do a full-text search for measureme = to find all the Cargo.toml files in the compiler code base that reference measureme. Replace these dependencies with a Cargo path dependency pointing to your local version of measureme and rebuild. That should give you a compiler emitting the new format.
• You can use rustup toolchain link to set up a convenient alias for your local rustc toolchain.

[michaelwoerister]

This looks great, almost done 🙂

I've left a few comments below. Regarding the v9 test data, I think it best to generate that using a locally built compiler version that already produces the new data format. Let me know if you need any help with that.

[smklein]

I'm running into an odd issue when building a rust toolchain manually. I can check out Rust, run x.py build + x.py install, and get a toolchain using the master branch, but when I replace:

measureme = "10.0.0"

With

measureme = { path = "/home/smklein/repos/measureme/measureme", version = "11.0.0" }

In all the Cargo.toml files, I'm suddenly seeing the following errors pop up (on Linux, x86-64, if it matters):

error: unexpected `cfg` condition name: `accurate_seqlock_rdpmc`
   --> /home/smklein/repos/measureme/measureme/src/counters.rs:424:26
    |
424 |             if (cfg!(not(accurate_seqlock_rdpmc)) || true) && pmc_width != 48 {
    |                          ^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: expected names are: `bootstrap`, `crossbeam_loom`, `debug_assertions`, `doc`, `doctest`, `emulate_second_only_system`, `feature`, `miri`, `no_btreemap_remove_entry`, `overflow_checks`, `panic`, `parallel_compiler`, `proc_macro`, `relocation_model`, `rustix_use_libc`, `sanitize`, `span_locations`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `test`, `unix`, `windows`, `windows_raw_dylib`
note: the lint level is defined here
   --> /home/smklein/repos/measureme/measureme/src/lib.rs:34:9
    |
34  | #![deny(warnings)]
    |         ^^^^^^^^
    = note: `#[deny(unexpected_cfgs)]` implied by `#[deny(warnings)]`

error: unexpected `cfg` condition name: `accurate_seqlock_rdpmc`
   --> /home/smklein/repos/measureme/measureme/src/counters.rs:474:56
    |
474 |             let (counter, offset, pmc_width) = if cfg!(accurate_seqlock_rdpmc) && false {
    |                                                        ^^^^^^^^^^^^^^^^^^^^^^

error: unexpected `cfg` condition name: `accurate_seqlock_rdpmc`
   --> /home/smklein/repos/measureme/measureme/src/counters.rs:503:22
    |
503 |             if (cfg!(accurate_seqlock_rdpmc) || cfg!(unserialized_rdpmc)) && false {
    |                      ^^^^^^^^^^^^^^^^^^^^^^

error: unexpected `cfg` condition name: `unserialized_rdpmc`
   --> /home/smklein/repos/measureme/measureme/src/counters.rs:503:54
    |
503 |             if (cfg!(accurate_seqlock_rdpmc) || cfg!(unserialized_rdpmc)) && false {
    |                                                      ^^^^^^^^^^^^^^^^^^

error: unexpected `cfg` condition name: `unserialized_rdpmc`
   --> /home/smklein/repos/measureme/measureme/src/counters.rs:526:17
    |
526 |         if cfg!(unserialized_rdpmc) && false {
    |                 ^^^^^^^^^^^^^^^^^^

I see these cfg statements within measureme, but I'm not sure where they'd normally come from.

[smklein]

Adding #[allow(unexpected_cfgs)] on each of the cfg! sites in measureme/src/counters.rs appears to mitigate this issue, not sure if that's the desired approach though.

[smklein]

Related: I really don't grok those configuration flags! accurate_seqlock_rdpmc and unserialized_rdpmc both only appear as strings within this file (I couldn't fine them anywhere else across github!), and they appear in boolean expressions where they're removable anyway?

measureme/measureme/src/counters.rs

Line 424 in 2371077

if (cfg!(not(accurate_seqlock_rdpmc)) || true) && pmc_width != 48 {

if (x || true) && pmc_width != 48 { 
->
if pmc_width != 48 {

... Okay, reading comments like this: #143 (comment) I suspect these aren't intended to be exposed anywhere, and perhaps the warning is new?

This makes me believe that intentionally ignoring those errors in measureme is the right approach.

[michaelwoerister]

The "unexpected cfg" errors seem to come from this new compiler feature:
https://github.com/rust-lang/rfcs/blob/master/text/3013-conditional-compilation-checking.md

We can add #![allow(unexpected_cfgs)] to counters.rs for now. The correct way to fix this, would be use a feature instead of vanilla cfg!()s.

[michaelwoerister]

We can add #![allow(unexpected_cfgs)] to counters.rs for now.

I'll do this as part of the 10.1.2 release.

[bors]

☔ The latest upstream changes (presumably #217) made this pull request unmergeable. Please resolve the merge conflicts.

[michaelwoerister]

I've done the 10.1.2 release, so the PR will need a rebase now.

[smklein]

I went ahead and added a v9 format test with cdd29c6 ! I was also able to successfully build a bespoke rust toolchain using this PR, and I'm able to produce very large profiles (~20 GiB) without panicking, where I was previously seeing panics at the ~4 GiB mark.

Thanks for all the great feedback, @michaelwoerister !

[michaelwoerister]

I'm able to produce very large profiles (~20 GiB) without panicking

Awesome! (and kind of terrifying 😅)

[michaelwoerister]

I left a few minor comments that would be nice to fix, but this is basically good to go. Thank you so much for working on this!

The only thing that's really left to do is cleaning up the git history. Can you rebase all your changes on top of the main branch? It might be easiest to squash everything into a single commit.

[smklein]

I left a few minor comments that would be nice to fix, but this is basically good to go. Thank you so much for working on this!

The only thing that's really left to do is cleaning up the git history. Can you rebase all your changes on top of the main branch? It might be easiest to squash everything into a single commit.

I'm happy to do this, but fwiw, I usually do work in another repo that uses the "Squash and Merge" option on Github:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits

The advantage is that the master branch commit history is "one-commit-per-PR", but within the context of a PR, individual commits don't get "force-pushed" into a single commit (which makes Github code review harder to navigate).

I'm happy to smush all these commits together into a single commit, but I like this config option because it does what you're describing without making review history hard to navigate. Your call!

[michaelwoerister]

Yeah, that does make sense. In other context I'm using that approach too. However, for the Rust project, rebasing is the standard and the only thing that bors, the build bot, is capable of.

[smklein]

I've gone ahead and collapsed this whole thing into a single commit

[michaelwoerister]

Thank you, @smklein!

@bors r+

[bors]

📌 Commit 4c716fc has been approved by michaelwoerister

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4c716fc with merge 8193f4d...

[bors]

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 8193f4d to master...