From e7fbf7223f640c5121b21183cd26ecd3ccb6a820 Mon Sep 17 00:00:00 2001 From: iximeow Date: Wed, 20 Mar 2024 01:27:33 -0700 Subject: [PATCH 1/3] improve codegen of fmt_num to delete unreachable panic it seems LLVM doesn't realize that `curr` is always decremented at least once in either loop formatting characters of the input string by their appropriate radix, and so the later `&buf[curr..]` generates a check for out-of-bounds access and panic. this is unreachable in reality as even for `x == T::zero()` we'll produce at least the character `Self::digit(T::zero())` for at least one character output, and `curr` will always be at least one below `buf.len()`. adjust `fmt_int` to make this fact more obvious to the compiler, which fortunately (or unfortunately) results in a measurable performance improvement for workloads heavy on formatting integers. --- library/core/src/fmt/num.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/core/src/fmt/num.rs b/library/core/src/fmt/num.rs index ab2158394bf1..abe833a4bf2d 100644 --- a/library/core/src/fmt/num.rs +++ b/library/core/src/fmt/num.rs @@ -79,11 +79,11 @@ unsafe trait GenericRadix: Sized { if is_nonnegative { // Accumulate each digit of the number from the least significant // to the most significant figure. - for byte in buf.iter_mut().rev() { + loop { let n = x % base; // Get the current place value. x = x / base; // Deaccumulate the number. - byte.write(Self::digit(n.to_u8())); // Store the digit in the buffer. curr -= 1; + buf[curr].write(Self::digit(n.to_u8())); // Store the digit in the buffer. if x == zero { // No more digits left to accumulate. break; @@ -91,11 +91,11 @@ unsafe trait GenericRadix: Sized { } } else { // Do the same as above, but accounting for two's complement. - for byte in buf.iter_mut().rev() { + loop { let n = zero - (x % base); // Get the current place value. x = x / base; // Deaccumulate the number. - byte.write(Self::digit(n.to_u8())); // Store the digit in the buffer. curr -= 1; + buf[curr].write(Self::digit(n.to_u8())); // Store the digit in the buffer. if x == zero { // No more digits left to accumulate. break; From cea973f857b63e7c208e822d95c06c4883062449 Mon Sep 17 00:00:00 2001 From: iximeow Date: Sat, 23 Mar 2024 11:33:09 -0700 Subject: [PATCH 2/3] try adding a test that LowerHex and friends don't panic, but it doesn't work --- library/core/benches/fmt.rs | 14 ++++++++++++++ tests/codegen/fmt_int_no_panic.rs | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 tests/codegen/fmt_int_no_panic.rs diff --git a/library/core/benches/fmt.rs b/library/core/benches/fmt.rs index d1cdb12e50f8..a02bd6065425 100644 --- a/library/core/benches/fmt.rs +++ b/library/core/benches/fmt.rs @@ -148,3 +148,17 @@ fn write_u64_min(bh: &mut Bencher) { test::black_box(format!("{}", 0u64)); }); } + +#[bench] +fn write_u8_max(bh: &mut Bencher) { + bh.iter(|| { + test::black_box(format!("{}", u8::MAX)); + }); +} + +#[bench] +fn write_u8_min(bh: &mut Bencher) { + bh.iter(|| { + test::black_box(format!("{}", 0u8)); + }); +} diff --git a/tests/codegen/fmt_int_no_panic.rs b/tests/codegen/fmt_int_no_panic.rs new file mode 100644 index 000000000000..efd0530bc888 --- /dev/null +++ b/tests/codegen/fmt_int_no_panic.rs @@ -0,0 +1,24 @@ +// Trying to check that formatting u8/u32/u64/etc do not panic. +// +// This test does not correctly do so yet. + +//@ compile-flags: -O + +#![crate_type = "lib"] + +// expected to need to write some kind of `impl core::fmt::Write` on a struct like this to avoid +// unrelated panics if `String::write_str` can't make space.. +// struct CanAlwaysBeWrittenTo; + +use std::fmt::Write; + +// CHECK-LABEL: @format_int_doesnt_panic +#[no_mangle] +pub fn format_int_doesnt_panic(s: &mut String) -> std::fmt::Result { + // CHECK-NOT: panic + // ... but wait! this will definitely panic if `s.vec.reserve_for_push()` cannot alloc! this + // shouldn't pass! + write!(s, "{:x}", 0u8)?; + write!(s, "{:x}", u8::MAX)?; + Ok(()) +} From a54d6ad03945d071ad74cb0363c12fd613fd9430 Mon Sep 17 00:00:00 2001 From: iximeow Date: Wed, 13 Nov 2024 09:50:00 -0800 Subject: [PATCH 3/3] Delete tests/codegen/fmt_int_no_panic.rs this test was included for demonstrative and discussion purposes but does not test what its name alleges - in fact it does not test much of value at all! as mentioned in this comment, https://github.com/rust-lang/rust/pull/122770#issuecomment-2474256965 , writing a correct test for this codegen outcome is difficult (partially because the public interface to std::fmt intentionally includes an #[inline(never)] function!), so to test this correctly will require more than i can offer in 122770. --- tests/codegen/fmt_int_no_panic.rs | 24 ------------------------ 1 file changed, 24 deletions(-) delete mode 100644 tests/codegen/fmt_int_no_panic.rs diff --git a/tests/codegen/fmt_int_no_panic.rs b/tests/codegen/fmt_int_no_panic.rs deleted file mode 100644 index efd0530bc888..000000000000 --- a/tests/codegen/fmt_int_no_panic.rs +++ /dev/null @@ -1,24 +0,0 @@ -// Trying to check that formatting u8/u32/u64/etc do not panic. -// -// This test does not correctly do so yet. - -//@ compile-flags: -O - -#![crate_type = "lib"] - -// expected to need to write some kind of `impl core::fmt::Write` on a struct like this to avoid -// unrelated panics if `String::write_str` can't make space.. -// struct CanAlwaysBeWrittenTo; - -use std::fmt::Write; - -// CHECK-LABEL: @format_int_doesnt_panic -#[no_mangle] -pub fn format_int_doesnt_panic(s: &mut String) -> std::fmt::Result { - // CHECK-NOT: panic - // ... but wait! this will definitely panic if `s.vec.reserve_for_push()` cannot alloc! this - // shouldn't pass! - write!(s, "{:x}", 0u8)?; - write!(s, "{:x}", u8::MAX)?; - Ok(()) -}