Skip to content

Seeing ICE due to u32 overflow #214

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
smklein opened this issue Nov 28, 2023 · 6 comments · Fixed by #216
Closed

Seeing ICE due to u32 overflow #214

smklein opened this issue Nov 28, 2023 · 6 comments · Fixed by #216

Comments

@smklein
Copy link
Contributor

smklein commented Nov 28, 2023

I have a repo I'm attempting to profile with these instructions: https://blog.rust-lang.org/inside-rust/2020/02/25/intro-rustc-self-profile.html#profiling-the-compiler

cargo +nightly rustc --lib -- -Zself-profile -Zself-profile-events=default,args

This results in the following output:

thread 'rustc' panicked at /rust/deps/measureme-10.1.1/src/stringtable.rs:105:62:                                                                                                                                                                                                                                                                                                                                                                               
called `Option::unwrap()` on a `None`

Which appears to be the unwrap within:

#[inline]
pub fn from_addr(addr: Addr) -> StringId {
let id = addr.0.checked_add(FIRST_REGULAR_STRING_ID).unwrap();
StringId::new(id)
}

I noticed that the type of StringId is a u32:

pub struct StringId(u32);

And since we're calling from_addr, we care about that type too:

/// An address within a data stream. Each data stream has its own address space,
/// i.e. the first piece of data written to the events stream will have
/// `Addr(0)` and the first piece of data written to the string data stream
/// will *also* have `Addr(0)`.
//
// TODO: Evaluate if it makes sense to add a type tag to `Addr` in order to
// prevent accidental use of `Addr` values with the wrong address space.
#[derive(Clone, Copy, Eq, PartialEq, Debug)]
pub struct Addr(pub u32);

Why do we believe this will be limited to u32? As a small data point, the "...mm_profdata" file I was emitting looks like it's just over 4 GiB in size, which sorta aligns with the idea of a u32 overflow.

@smklein
Copy link
Contributor Author

smklein commented Nov 28, 2023

For reproducing:

# Starting within Omicron repo...
cd nexus
cargo +nightly rustc --lib -- -Zself-profile -Zself-profile-events=default,query-keys

And for context:

$ rustc +nightly --version
rustc 1.76.0-nightly (6cf088810 2023-11-26)

@smklein
Copy link
Contributor Author

smklein commented Nov 28, 2023

This looks similar to:

This is consistently reproducible for me

@smklein
Copy link
Contributor Author

smklein commented Nov 28, 2023

Also looks like rust-lang/rust#99282

@smklein
Copy link
Contributor Author

smklein commented Dec 7, 2023

My attempt to patch this exists in #216

@bors bors closed this as completed in 8193f4d Dec 16, 2023
@michaelwoerister
Copy link
Member

Let's keep this open until the change has made it into the compiler.

@michaelwoerister
Copy link
Member

The update is in the nightly compiler now.

Thanks again for implementing the fix, @smklein!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
2 participants