Use MaybeUninit in DecInt

[SUPERCILEX]

Double checked with cargo miri t --workspace --features all-apis -- dec_int. Also got rid of the fmt::Write impl as that's confusingly complicated.

[sunfishcode]

Looking at the generated machine code in --release, this patch makes DecInt::new slightly longer. It eliminates the zeroing of the array at the beginning, but adds a bounds check. Is there a way this code could be structured to avoid that?

[SUPERCILEX]

Eyyyyyy, we're doing compiler optimization driven development! So in trying to remove all panics I discovered that since we accept the itoa::Integer trait we technically also accept i128s which meant we could crash if their representation was too large. I changed the number of bytes in DecInt to match itoa::Buffer+1 so that all panics are removed. However, I'm not sure if this is the right choice given that we're targeting file descriptors... maybe we should actually only have space for 10 str digits and assert that fact (which removes the double panics)?

[sunfishcode]

Would it work to add a type parameter to DecInt? Decint<T> that calculates the size of the buffer as a function of the size of T?

[SUPERCILEX]

Damn, I love that idea but don't it's possible in stable rust. This is an associated type:

error: generic parameters may not be used in const operations
  --> src/path/dec_int.rs:66:28
   |
66 |     buf: [MaybeUninit<u8>; T::BUF_SIZE + 1],
   |                            ^^^^^^^^^^^ cannot perform const operation using `T`
   |
   = note: type parameters may not be used in const expressions
   = help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

I also tried a lookup table version:

error: generic parameters may not be used in const operations
  --> src/path/dec_int.rs:66:53
   |
66 |     buf: [MaybeUninit<u8>; BUF_SIZES[mem::size_of::<T>()] + 1],
   |                                                     ^ cannot perform const operation using `T`
   |
   = note: type parameters may not be used in const expressions
   = help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

[sunfishcode]

Hrm. I'm not sure what to suggest here then.

We do also use DecInt for PID values, which are usually u32, but is usize on redox.

[SUPERCILEX]

Ok I think I figured out the best we can do. I went back to a buffer size of 21 but added explicit assertions that fail if you try to print a huge number. I also added buffer sizing guarantees for the optimizer so panics and the assertion are compiled out for anything smaller than an i64. It's hella ugly but the assembly is super clean so there's that.

[SUPERCILEX]

Errors are unrelated:

home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustix-0.38.39/src/backend/libc/fs/types.rs:957:30
    |
957 |     LockShared = bitcast!(c::LOCK_SH),
    |                              ^^^^^^^ not found in `c`

error[E0425]: cannot find value `LOCK_EX` in module `c`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustix-0.38.39/src/backend/libc/fs/types.rs:959:33
    |
959 |     LockExclusive = bitcast!(c::LOCK_EX),
    |                                 ^^^^^^^ not found in `c`

error[E0425]: cannot find value `LOCK_UN` in module `c`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustix-0.38.39/src/backend/libc/fs/types.rs:961:26
    |
961 |     Unlock = bitcast!(c::LOCK_UN),
    |                          ^^^^^^^ not found in `c`

error[E0425]: cannot find value `LOCK_SH` in module `c`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustix-0.38.39/src/backend/libc/fs/types.rs:963:41
    |
963 |     NonBlockingLockShared = bitcast!(c::LOCK_SH | c::LOCK_NB),
    |                                         ^^^^^^^ not found in `c`

[sunfishcode]

Cool, that approach sounds good. Those errors are now fixed on main.

[SUPERCILEX]

Hmmm, are you sure? I just rebased 20 mins ago.

[sunfishcode]

Oh, this is the issue where rustix dev-depends on itself. The libc crate broke semver on solaris, breaking the latest release version of rustix, which is what gets pulled in in a cargo test. I've now released v0.38.40 to fix this.

[SUPERCILEX]

Thank you! Just re-triggered the build.

[sunfishcode]

And I've now fixed the test build in #1214.

[SUPERCILEX]

We have a green build! :)

[sunfishcode]

It's worth commenting that this also happens to be enough to hold a u64 and NUL terminator, because even though u64::MAX needs one more decimal digit than i64::MAX, it doesn't need the "-", so it works out.

[sunfishcode]

The logic here is pretty subtle, so I've attempted to analyze the different cases:

If the condition here is false, this code executes undefined behavior. If this condition is true, in release builds, the assert below is optimized out. If this condition is true, in debug builds, the assert below is not optimized out.

If I'm not mistaken, this is all effectively the same as if we removed the unreachable_unchecked() and all the code it depends on above here, and just changed the assert below to a debug_assert.

[SUPERCILEX]

Not quite, the assertion is a bit of a red herring. It's there to make the error message nicer on {u,i}128s because otherwise we panic on bounds check failures. If you remove the hint, you'll get bounds check panics on all ints (or rather the assertion will be compiled in, but if we made it debug_assert then release mode would see bounds checks).

It's probably easier to see this by playing with https://rust.godbolt.org/z/dqb53Pj8o. Change the param in the function foo at the top to see how the different monomorphizations look.

[sunfishcode]

Ah, thanks, I get it now. I played around with the link a little, and came up with this: https://rust.godbolt.org/z/4TEvYc4x8. It codegens to the same code, but does the assert before the unreachable hint so that we don't invoke UB before the assert in debug builds, and it doesn't use the TypeId checks. What do you think?

[SUPERCILEX]

Unfortunately that has UB if you pass in a u128::MAX for example (str_buf will be 40 long while buf is only 21). For debug builds they should already be getting an assertion as long as they don't enable optimizations: https://doc.rust-lang.org/src/core/hint.rs.html#100. One idea is to change the TypeId stuff to panic on 128 bit values so that we can use your simplified hint condition, but that'll still have very ugly TypeId checks.