Skip to content
Merged
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 60 additions & 23 deletions library/core/src/fmt/num.rs
Original file line number Diff line number Diff line change
Expand Up @@ -690,15 +690,13 @@ impl u128 {
unsafe { core::hint::assert_unchecked(offset <= buf.len()) }
offset -= 4;

// pull two pairs
let quad = remain % 1_00_00;
remain /= 1_00_00;
let pair1 = (quad / 100) as usize;
let pair2 = (quad % 100) as usize;
buf[offset + 0].write(DECIMAL_PAIRS[pair1 * 2 + 0]);
buf[offset + 1].write(DECIMAL_PAIRS[pair1 * 2 + 1]);
buf[offset + 2].write(DECIMAL_PAIRS[pair2 * 2 + 0]);
buf[offset + 3].write(DECIMAL_PAIRS[pair2 * 2 + 1]);
// SAFETY: quad is a remainder modulo 10_000. The offset checks
// above reserve exactly four bytes in buf.
unsafe {
write_quad(buf.get_unchecked_mut(offset..offset + 4), quad);
}
}

// Format per two digits from the lookup table.
Expand Down Expand Up @@ -814,32 +812,71 @@ impl i128 {
}
}

/// Writes `quad` as exactly four digits (for example: `42` becomes `"0042"`).
///
/// # Safety
///
/// `quad` must be below 10_000 and `buf` must contain exactly four bytes.
#[inline(always)]
unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {

@inkreasing inkreasing Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if this is taking over review (sorry if it is), but if buf has to be exactly four elements long, why not do it like this?

Suggested change
unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
unsafe fn write_quad(buf: &mut [MaybeUninit<u8>; 4], quad: u64) {

Using it should not be too hard, since https://doc.rust-lang.org/std/primitive.slice.html#impl-TryFrom%3C%26mut+%5BT%5D%3E-for-%26mut+%5BT;+N%5D and if perf sensitive unwrap_unchecked.

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

core::hint::assert_unchecked(buf.len() == 4)

already provides the compiler with the same guarantees a &mut [MaybeUninit<u8>; 4] would, so switching types wouldn't remove any unsafe or improve codegen, every call site already builds this slice via get_unchecked_mut(range), itself unsafe

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, unfortunately it's not super ergonomic to cast between arrays and slices this way, and since it's unsafe anyway, it's not really worth it.

// SAFETY: These are this function's caller-provided invariants.
unsafe {
core::hint::assert_unchecked(quad < 10_000);
core::hint::assert_unchecked(buf.len() == 4);
}
Comment thread
chirizxc marked this conversation as resolved.

let quad = quad as u32;

// Note: this is equivalent to `quad / 100`, but contains no division instructions.
let high = (quad * const { (1 << 19) / 100 + 1 }) >> 19;
let low = quad - high * 100;
let high = high as usize;
let low = low as usize;

// SAFETY: `high` and `low` are below 100 because `quad` is below 10_000.
// The destination has four bytes by the precondition, and the two source
// pairs are disjoint from it because `DECIMAL_PAIRS` is static RO storage.
unsafe {
let pairs = DECIMAL_PAIRS.as_ptr();
let dst = buf.as_mut_ptr().cast::<u8>();
Comment thread
chirizxc marked this conversation as resolved.
Outdated

core::ptr::copy_nonoverlapping(pairs.add(high * 2), dst, 2);
core::ptr::copy_nonoverlapping(pairs.add(low * 2), dst.add(2), 2);

@clarfonthey clarfonthey Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Honestly was thinking maybe this would be worded better as write_copy_of_slice to avoid the raw pointer arithmetic, but I don't think it matters too much.

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

diff --git a/library/core/src/fmt/num.rs b/library/core/src/fmt/num.rs
index d703b817b5a..f94e5433a51 100644
--- a/library/core/src/fmt/num.rs
+++ b/library/core/src/fmt/num.rs
@@ -692,11 +692,12 @@ unsafe fn _fmt_inner(self, buf: &mut [MaybeUninit<u8>]) -> usize {
 
             let quad = remain % 1_00_00;
             remain /= 1_00_00;
-            // SAFETY: quad is a remainder modulo 10_000. The offset checks
-            // above reserve exactly four bytes in buf.
-            unsafe {
-                write_quad(buf.get_unchecked_mut(offset..offset + 4), quad);
-            }
+
+            write_quad(
+                // SAFETY: `offset >= 4` was asserted above.
+                unsafe { buf.get_unchecked_mut(offset..offset + 4) },
+                quad,
+            );
         }
 
         // Format per two digits from the lookup table.
@@ -813,12 +814,8 @@ pub fn format_into(self, buf: &mut NumBuffer<Self>) -> &str {
 }
 
 /// Writes `quad` as exactly four digits (for example: `42` becomes `"0042"`).
-///
-/// # Safety
-///
-/// `quad` must be below 10_000 and `buf` must contain exactly four bytes.
 #[inline(always)]
-unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
+fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
     // SAFETY: These are this function's caller-provided invariants.
     unsafe {
         core::hint::assert_unchecked(quad < 10_000);
@@ -834,15 +831,10 @@ unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
     let low = low as usize;
 
     // SAFETY: `high` and `low` are below 100 because `quad` is below 10_000.
-    // The destination has four bytes by the precondition, and the two source
-    // pairs are disjoint from it because `DECIMAL_PAIRS` is static RO storage.
-    unsafe {
-        let pairs = DECIMAL_PAIRS.as_ptr();
-        let dst = buf.as_mut_ptr().cast_init();
+    unsafe { core::hint::assert_unchecked(high < 100 && low < 100) }
 
-        core::ptr::copy_nonoverlapping(pairs.add(high * 2), dst, 2);
-        core::ptr::copy_nonoverlapping(pairs.add(low * 2), dst.add(2), 2);
-    }
+    buf[0..2].write_copy_of_slice(&DECIMAL_PAIRS[high * 2..high * 2 + 2]);
+    buf[2..4].write_copy_of_slice(&DECIMAL_PAIRS[low * 2..low * 2 + 2]);
 }
 
 /// Encodes the 16 least-significant decimals of n into `buf[OFFSET .. OFFSET +
@@ -863,20 +855,20 @@ fn enc_16lsd<const OFFSET: usize>(buf: &mut [MaybeUninit<u8>], n: u64) {
         let quad = remain % 1_00_00;
         remain /= 1_00_00;
 
-        // SAFETY: `OFFSET + quad_index * 4` starts one of the four
-        // non-overlapping four-byte regions proven in bounds above.
-        unsafe {
-            write_quad(
-                buf.get_unchecked_mut(OFFSET + quad_index * 4..OFFSET + (quad_index + 1) * 4),
-                quad,
-            );
-        }
+        write_quad(
+            // SAFETY: `OFFSET + 16 <= buf.len()` and `quad_index < 4`, so this range is within `buf`.
+            unsafe {
+                buf.get_unchecked_mut(OFFSET + quad_index * 4..OFFSET + (quad_index + 1) * 4)
+            },
+            quad,
+        );
     }
 
-    // SAFETY: OFFSET starts the first four-byte region proven in bounds above.
-    unsafe {
-        write_quad(buf.get_unchecked_mut(OFFSET..OFFSET + 4), remain);
-    }
+    write_quad(
+        // SAFETY: `OFFSET + 16 <= buf.len()` was asserted above.
+        unsafe { buf.get_unchecked_mut(OFFSET..OFFSET + 4) },
+        remain,
+    );
 }
 
 /// Euclidean division plus remainder with constant 1E16 basically consumes 16

I applied this patch; it seems that now we can make write_quad safe and slightly reduce the amount of unsafe code in the places where it was previously needed. Also, to help the compiler, I had to add another assert_unchecked:

unsafe { core::hint::assert_unchecked(high < 100 && low < 100) }

for

    buf[0..2].write_copy_of_slice(&DECIMAL_PAIRS[high * 2..high * 2 + 2]);
    buf[2..4].write_copy_of_slice(&DECIMAL_PAIRS[low * 2..low * 2 + 2]);

@chirizxc chirizxc Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

}
}

/// Encodes the 16 least-significant decimals of n into `buf[OFFSET .. OFFSET +
/// 16 ]`.
fn enc_16lsd<const OFFSET: usize>(buf: &mut [MaybeUninit<u8>], n: u64) {

@inkreasing inkreasing Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if std has some special convention, but does this function have to become unsafe?
Since it now depends on the caller not passing n >= 10_000_000_000_000_000 otherwise UB happens.

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, this should be marked as unsafe. My bad; I thought both functions were remarked as unsafe, but I guess not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trying to understand why I missed this, and I guess it's because GitHub has been doing a horrible job trying to show partial diffs when I review things instead of full diffs, and I thought I had reviewed the full diff, but I guess not.

.-.

// Consume the least-significant decimals from a working copy.
// SAFETY: Every caller passes a remainder produced by division by 10^16,
// and every used `OFFSET` specialization reserves sixteen bytes in `buf`.
unsafe {
core::hint::assert_unchecked(n < 10_000_000_000_000_000);
core::hint::assert_unchecked(OFFSET + 16 <= buf.len());
}

// Peel four digits at a time from right to left (12345678 -> 1234 | 5678).
// Since 10_000 is constant, LLVM replaces each division with multiply or shift.
let mut remain = n;

// Format per four digits from the lookup table.
for quad_index in (1..4).rev() {
// pull two pairs
let quad = remain % 1_00_00;
remain /= 1_00_00;
let pair1 = (quad / 100) as usize;
let pair2 = (quad % 100) as usize;
buf[quad_index * 4 + OFFSET + 0].write(DECIMAL_PAIRS[pair1 * 2 + 0]);
buf[quad_index * 4 + OFFSET + 1].write(DECIMAL_PAIRS[pair1 * 2 + 1]);
buf[quad_index * 4 + OFFSET + 2].write(DECIMAL_PAIRS[pair2 * 2 + 0]);
buf[quad_index * 4 + OFFSET + 3].write(DECIMAL_PAIRS[pair2 * 2 + 1]);

// SAFETY: `OFFSET + quad_index * 4` starts one of the four
// non-overlapping four-byte regions proven in bounds above.
unsafe {
write_quad(
buf.get_unchecked_mut(OFFSET + quad_index * 4..OFFSET + (quad_index + 1) * 4),
quad,
);
}
}

// final two pairs
let pair1 = (remain / 100) as usize;
let pair2 = (remain % 100) as usize;
buf[OFFSET + 0].write(DECIMAL_PAIRS[pair1 * 2 + 0]);
buf[OFFSET + 1].write(DECIMAL_PAIRS[pair1 * 2 + 1]);
buf[OFFSET + 2].write(DECIMAL_PAIRS[pair2 * 2 + 0]);
buf[OFFSET + 3].write(DECIMAL_PAIRS[pair2 * 2 + 1]);
// SAFETY: OFFSET starts the first four-byte region proven in bounds above.
unsafe {
write_quad(buf.get_unchecked_mut(OFFSET..OFFSET + 4), remain);
}
}

/// Euclidean division plus remainder with constant 1E16 basically consumes 16
Expand Down
Loading