Support for 32-bit architectures

[agausmann]

I would love to be able to use ustr for my WebAssembly project, but it is a 32-bit architecture. What are the blockers for supporting 32-bit?

[anderslanglands]

Hi Adam, there's no fundamental reason why it shouldn't work on 32-bit - just that I don't have a 32-bit system to test it on and there's a lot of unsafe in there so I added the compile-time pointer size check more as a safeguard against someone trying to use it on a system I hadn't tested at all and something awful happening.

It may even work OOTB if you remove the check as I don't think I explicity assume the size of a pointer is 8 bytes anywhere. You might want to just try removing that check and running the tests (make sure you do so with RUST_TEST_THREADS=1) to see what happens. I'm happy to guide you through any issues you might encounter.

[agausmann]

Ah yeah, I did do that, and it caught some kind of UB, but I didn't really know how to read the message and track down the error:

$ cargo miri test --target i686-unknown-linux-gnu -- -Zmiri-ignore-leaks
    Checking ustr v0.5.0 (/home/adam/dev/contrib/ustr)
warning: unnecessary parentheses around block return value
  --> src/bumpalloc.rs:61:9
   |
61 |         (self.end as usize - self.ptr as usize)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
   |
   = note: `#[warn(unused_parens)]` on by default


running 6 tests
error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
  --> src/hash.rs:50:5
   |
50 |     assert_eq!(hasher.finish(), u1.precomputed_hash());
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: inside `hash::test_hashing` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/macros/mod.rs:70:21
note: inside closure at src/hash.rs:41:1
  --> src/hash.rs:41:1
   |
41 | / fn test_hashing() {
42 | |     use crate::ustr as u;
43 | |
44 | |     use std::hash::Hash;
...  |
61 | |     assert_eq!(hm.get(&u2), Some(&42));
62 | | }
   | |_^
   = note: inside `<[closure@src/hash.rs:41:1: 62:2] as std::ops::FnOnce<()>>::call_once - shim` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5
   = note: inside `hash::test::__rust_begin_short_backtrace::<fn()>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:517:5
   = note: inside closure at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:508:30
   = note: inside `<[closure@DefId(22:631 ~ test[6beb]::run_test[0]::{{closure}}[2]) 0:fn()] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5
   = note: inside `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/liballoc/boxed.rs:1008:9
   = note: inside `<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>> as std::ops::FnOnce<()>>::call_once` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:318:9
   = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:331:40
   = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:274:15
   = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394:14
   = note: inside `hash::test::run_test_in_process` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:541:18
   = note: inside closure at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:450:39
   = note: inside `hash::test::run_test::run_test_inner` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:475:13
   = note: inside `hash::test::run_test` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:505:28
   = note: inside `hash::test::run_tests::<[closure@DefId(22:230 ~ test[6beb]::console[0]::run_tests_console[0]::{{closure}}[2]) 0:&mut hash::test::console::ConsoleTestState, 1:&mut std::boxed::Box<dyn hash::test::formatters::OutputFormatter>]>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:284:13
   = note: inside `hash::test::run_tests_console` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/console.rs:280:5
   = note: inside `hash::test::test_main` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:120:15
   = note: inside `hash::test::test_main_static` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:139:5
   = note: inside `main`
   = note: inside closure at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:34
   = note: inside closure at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:73
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<[closure@DefId(1:6028 ~ std[e028]::rt[0]::lang_start_internal[0]::{{closure}}[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:130:5
   = note: inside closure at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:13
   = note: inside `std::panicking::r#try::do_call::<[closure@DefId(1:6027 ~ std[e028]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:331:40
   = note: inside `std::panicking::r#try::<i32, [closure@DefId(1:6027 ~ std[e028]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:274:15
   = note: inside `std::panic::catch_unwind::<[closure@DefId(1:6027 ~ std[e028]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394:14
   = note: inside `std::rt::lang_start_internal` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:51:25
   = note: inside `std::rt::lang_start::<()>` at /home/adam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:5
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

test hash::test_hashing ...
error: could not compile `ustr`.

To learn more, run the command again with --verbose.

---

Another thing that I'm concerned about is the assumption of the layout of a Ustr in memory, where you have len using a usize offset and precomputed_hash using a u64 offset. I don't think it will cause any issues except for pointer widths larger than 64 bit, since the u64 offset will just skip past the usize for any pointer width not larger than 64 bit. I just haven't checked how the insertion in the StringCache works, if it lays it out in the same way for other pointer widths.

[agausmann]

So I think there are some assumptions made in StringCache that may not hold in the general case, which is what causing u1.precomputed_hash() to be not properly initialized in the error above. It needs to be looked at more closely, there is a lot going on in this library.

At the moment, I've decided to use a HashSet<CString> for my project instead, since it is more easily auditable and reduces my dependency count a bit

[anderslanglands]

Yep I've found the issue in this case. I think StringCache it's actually fine, it was the Ustr::precomputed_hash() accessor was assuming that that the hash was 16 bytes before the characters which won't be true on a 32-bit platform.

Could you try pulling and running the tests again please?

[agausmann]

The miri tests pass on i686 now, though I'm not convinced that your solution is completely sound. It's not clear that the *const u64 is properly aligned in all cases from the perspective of Ustr. I guess if StringCacheEntrys are always placed at aligned locations (which both the allocator and StringCacheEntry::next_entry seem to guarantee), then it should be okay. Might be a good idea to address that in the relevant safety comments, and also update them to agree with the new offset logic.

[anderslanglands]

Thanks for checking! Yes indeed correct alignment is guaranteed by the allocator. I'll update the comments to match. In fact what I'll probably do is move the conversion from char* to StringCacheEntry* to a dedicated method so that precomputed_hash() and len() can both be safe wrappers around it.

…

On Tue, 7 Apr 2020 at 09:18, Adam Gausmann ***@***.***> wrote: The miri tests pass on i686 now, though I'm not convinced that your solution is completely sound. It's not clear that the *const u64 is properly aligned in all cases from the perspective of Ustr. I guess if StringCacheEntrys are always placed at aligned locations (which both the allocator and StringCacheEntry::next_entry seem to guarantee), then it should be okay. Might be a good idea to address that in the relevant safety comments though. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOYQXKO4B64WOW2OGY4XDLRLJBKBANCNFSM4L5L6H6Q> .

[Lesiuk]

32-bit crashes randomly with STATUS_ACCESS_VIOLATION on heavy multithreaded workloads in my application. I'm pretty sure It's Ustr because when I changed all of the Strings from Ustr to String everything is fine. Also everything is fine with 64bit.

[anderslanglands]

Sorry for the delayed response here. Do you have an idea of how many strings are being created by your application?