artichoke / intaglio Goto Github PK

cargo miri test changed to be more compatible with cargo test, but this means we had to find a different way to pass flags to the interpreter, so there now is a MIRIFLAGS environment variable for that. The old way still works for now, but is deprecated.

This affects the following line:

Hi, thanks for the great library 😁 I've been working on a compiler that has many Option<Symbol>s scattered throuhgout. This takes 8 bytes to store, since it needs 4 bytes for the u32, and 4 bytes for the discimminant + padding.

If you replace u32 with NonZeroU32, the compiler can see that the all-0 bit pattern is not valid, so it can use that for the None case, saving 4 bytes:

println!("{}", std::mem::size_of::<u32>());
println!("{}", std::mem::size_of::<Option<u32>>());
println!("{}", std::mem::size_of::<NonZeroU32>());
println!("{}", std::mem::size_of::<Option<NonZeroU32>>());

prints:

4
8
4
4

I've already made this change locally, and it works well, so I'd like to upstream (if it's useful to other people). However, this would be a breaking change, and it feels hard to justify a breaking change for a small (but noticeable) perf hit.

Currently, I see 3 ways of doing this:

• change the function signatures to accept NonZeroU32 instead of u32 (and replace some From impls with TryFrom)
• make the affected APIs panic, and add docs saying that they cannot take 0 as a parameter
• make the APIs simply use u32::MAX when passed 0 (or some other "random" constant)

The first option seems ideal, but is a breaking change to the API signature. The second is what I've implemented in my local change, but is also a breaking change, just without the compiler errors, which is arguably worse. The third option just makes me feel really uneasy.

It's also worth mentioning that I'm not at all familiar with Ruby, and I understand this project is part of a system that needs to be compatible with existing versions of Ruby in some way. If there is a reason why NonZeroU32 is inappropriate here, then it probably makes sense to just fork it. I came across this repo in a comparison of various string interners, and it was the only one that supported PathBuf.

I'd be happy to PR the changes that I've made, but before going further, I thought it would be best to check what the best way forward would be

Thanks 😁

In upgrading to version 1.9.0, we hit a break with the following compilation error:

325 | impl NFSFileSystem for MirrorFS {
    |                        ^^^^^^^^ `NonNull<OsStr>` cannot be sent between threads safely
    |
    = help: within `intaglio::internal::Interned<OsStr>`, the trait `Send` is not implemented for `NonNull<OsStr>`

This seems to be a regression from 1.8.*.

This is with Cargo/rustc version 1.69.

Following up on #117, let's flesh out the remaining owned string/slice pairs in std.

This should be pretty easy to add and would clean up a lot of code in artichoke-backend which deals with mruby expecting NUL-terminated symbols in its own way.

Things to do:

• Implement Slice for Path in internal.rs.
• copy-paste the source in bytes.rs to path.rs
• Fixup the argument types and docs.
• Add a path feature for exposing this new interner.

Following up on #117, let's flesh out the remaining owned string/slice pairs in std.

This should be pretty easy to add and would clean up a lot of code in artichoke-backend which deals with mruby expecting NUL-terminated symbols in its own way.

Things to do:

• Implement Slice for OsStr in internal.rs.
• copy-paste the source in bytes.rs to osstr.rs
• Fixup the argument types and docs.
• Add a osstr feature for exposing this new interner.

HashMap::try_reserve was stabilized in Rust 1.57.0 (current MSRV is 1.58.0).

This should be pretty easy to add and would clean up a lot of code in artichoke-backend which deals with mruby expecting NUL-terminated symbols in its own way.

Things to do:

• Implement Slice for CStr in internal.rs.
• copy-paste the source in bytes.rs to cstr.rs
• Fixup the argument types and docs.
• Add a cstr feature for exposing this new interner.

See:

• rust-lang/miri#2045

The current hard mode is:

$ MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-check-number-validity -Zmiri-symbolic-alignment-check -Zrandomize-layout" cargo +nightly miri test --test leak_drop

Blockers:

• rust-lang/rust#96163

https://github.com/artichoke/intaglio/actions/runs/5663120563/job/15344308090

error: Undefined Behavior: trying to retag from <99387> for SharedReadOnly permission at alloc31495[0x0], but that tag does not exist in the borrow stack for this location
   --> /home/runner/work/intaglio/intaglio/src/bytes.rs:713:45
    |
713 |         debug_assert_eq!(self.get(id), Some(slice));
    |                                             ^^^^^
    |                                             |
    |                                             trying to retag from <99387> for SharedReadOnly permission at alloc31495[0x0], but that tag does not exist in the borrow stack for this location
    |                                             this error occurs as part of retag at alloc31495[0x0..0x64]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <99387> was created by a SharedReadOnly retag at offsets [0x0..0x64]
   --> /home/runner/work/intaglio/intaglio/src/bytes.rs:708:30
    |
708 |         let slice = unsafe { name.as_static_slice() };
    |                              ^^^^^^^^^^^^^^^^^^^^^^
help: <99387> was later invalidated at offsets [0x0..0x64] by a Unique retag (of a reference/box inside this compound value)
   --> /home/runner/work/intaglio/intaglio/src/bytes.rs:711:23
    |
711 |         self.vec.push(name);
    |                       ^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `intaglio::bytes::SymbolTable::intern::<std::vec::Vec<u8>>` at /home/runner/work/intaglio/intaglio/src/bytes.rs:713:45: 713:50
note: inside `bytes::dealloc_owned_data`
   --> tests/leak_drop/bytes.rs:9:22
    |
9   |         let sym_id = table.intern(symbol.clone()).unwrap();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> tests/leak_drop/bytes.rs:4:25
    |
3   | #[test]
    | ------- in this procedural macro expansion
4   | fn dealloc_owned_data() {
    |                         ^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

error: test failed, to rerun pass `--test leak_drop`