Use MaybeUninit in DecInt #1201

SUPERCILEX · 2024-10-29T00:25:17Z

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 · 2024-10-30T17:08:46Z

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 · 2024-10-31T03:06:18Z

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 · 2024-11-04T23:54:34Z

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 · 2024-11-05T02:42:40Z

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 · 2024-11-09T16:26:27Z

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 · 2024-11-09T17:44:20Z

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 · 2024-11-09T18:07:36Z

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 · 2024-11-09T18:16:12Z

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

SUPERCILEX · 2024-11-09T18:22:11Z

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

sunfishcode · 2024-11-10T15:56:42Z

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 · 2024-11-10T16:14:57Z

Thank you! Just re-triggered the build.

sunfishcode · 2024-11-10T16:27:32Z

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

SUPERCILEX · 2024-11-10T17:07:34Z

We have a green build! :)

sunfishcode · 2024-11-10T17:14:03Z

src/path/dec_int.rs

-    // 20 `u8`s is enough to hold the decimal ASCII representation of any
-    // `u64`, and we add one for a NUL terminator for `as_c_str`.
-    buf: [u8; 20 + 1],
+    // Enough to hold an i64 and NUL terminator.


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 · 2024-11-12T16:58:08Z

src/path/dec_int.rs

+            };
+            if str_buf.len() > max_buf_size {
+                unsafe { core::hint::unreachable_unchecked() }
+            }


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.

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.

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?

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.

…ze out any panics in DecInt::new

SUPERCILEX · 2024-11-20T13:12:31Z

Alrighty, thoughts now that all the icky stuff was removed?

sunfishcode

Cool, this looks much prettier, thanks for working on this with upstream! Just a few minor comments:

tests/path/dec_int.rs

src/path/dec_int.rs

* Use MaybeUninit in DecInt * Fix DecInt::new panicking on integers greater than 64 bits and optimize out any panics in DecInt::new * Add tests so people messing with DecInt can catch UB with miri * Use new itoa --------- Co-authored-by: Alex Saveau <[email protected]>

sunfishcode · 2024-12-09T01:17:14Z

This is now released in rustix 0.38.42.

SUPERCILEX force-pushed the decint branch from 8a50529 to 4183bb0 Compare October 31, 2024 03:08

SUPERCILEX force-pushed the decint branch 4 times, most recently from 25a332a to 22e9bf9 Compare November 9, 2024 17:58

SUPERCILEX force-pushed the decint branch from 22e9bf9 to f4447b3 Compare November 10, 2024 16:14

SUPERCILEX force-pushed the decint branch from d048754 to 99545ee Compare November 10, 2024 16:39

sunfishcode reviewed Nov 12, 2024

View reviewed changes

SUPERCILEX force-pushed the decint branch from 9efbe39 to a46a59b Compare November 14, 2024 01:15

Alex Saveau added 4 commits November 19, 2024 20:43

Use MaybeUninit in DecInt

ad56bde

Fix DecInt::new panicking on integers greater than 64 bits and optimi…

db636de

…ze out any panics in DecInt::new

Tweak comments

b0f5234

Add tests so people messing with DecInt can catch UB with miri

3ff3b66

SUPERCILEX force-pushed the decint branch from 2990def to b8181bb Compare November 20, 2024 01:43

Remove TypeId hacks assuming itoa changes go through

27fd91a

SUPERCILEX force-pushed the decint branch from b8181bb to 27fd91a Compare November 20, 2024 01:44

Use new itoa

181ac3b

sunfishcode reviewed Nov 20, 2024

View reviewed changes

tests/path/dec_int.rs Show resolved Hide resolved

src/path/dec_int.rs Outdated Show resolved Hide resolved

Alex Saveau added 2 commits November 20, 2024 19:15

Address review feedback

36adbb0

Bump

6e9dd42

sunfishcode merged commit 61f17d6 into bytecodealliance:main Nov 21, 2024
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use MaybeUninit in DecInt #1201

Use MaybeUninit in DecInt #1201

SUPERCILEX commented Oct 29, 2024

sunfishcode commented Oct 30, 2024

SUPERCILEX commented Oct 31, 2024

sunfishcode commented Nov 4, 2024

SUPERCILEX commented Nov 5, 2024

sunfishcode commented Nov 9, 2024

SUPERCILEX commented Nov 9, 2024

SUPERCILEX commented Nov 9, 2024

sunfishcode commented Nov 9, 2024

SUPERCILEX commented Nov 9, 2024

sunfishcode commented Nov 10, 2024

SUPERCILEX commented Nov 10, 2024

sunfishcode commented Nov 10, 2024

SUPERCILEX commented Nov 10, 2024

sunfishcode Nov 10, 2024

sunfishcode Nov 12, 2024 •

edited

Loading

SUPERCILEX Nov 13, 2024

sunfishcode Nov 14, 2024

SUPERCILEX Nov 14, 2024

SUPERCILEX commented Nov 20, 2024

sunfishcode left a comment

sunfishcode commented Dec 9, 2024

Use MaybeUninit in DecInt #1201

Use MaybeUninit in DecInt #1201

Conversation

SUPERCILEX commented Oct 29, 2024

sunfishcode commented Oct 30, 2024

SUPERCILEX commented Oct 31, 2024

sunfishcode commented Nov 4, 2024

SUPERCILEX commented Nov 5, 2024

sunfishcode commented Nov 9, 2024

SUPERCILEX commented Nov 9, 2024

SUPERCILEX commented Nov 9, 2024

sunfishcode commented Nov 9, 2024

SUPERCILEX commented Nov 9, 2024

sunfishcode commented Nov 10, 2024

SUPERCILEX commented Nov 10, 2024

sunfishcode commented Nov 10, 2024

SUPERCILEX commented Nov 10, 2024

sunfishcode Nov 10, 2024

Choose a reason for hiding this comment

sunfishcode Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

SUPERCILEX Nov 13, 2024

Choose a reason for hiding this comment

sunfishcode Nov 14, 2024

Choose a reason for hiding this comment

SUPERCILEX Nov 14, 2024

Choose a reason for hiding this comment

SUPERCILEX commented Nov 20, 2024

sunfishcode left a comment

Choose a reason for hiding this comment

sunfishcode commented Dec 9, 2024

sunfishcode Nov 12, 2024 •

edited

Loading