rust-vmm / vm-memory

Virtual machine's guest memory crate
Apache License 2.0
305 stars 98 forks source link

Tests fail on 32-bit #225

Closed plugwash closed 6 months ago

plugwash commented 1 year ago

In debian we recently updated the vm-memory crate to version 0.10.0, our CI workers ran the tests (with previous versions the tests were not run due to dependency issues) and this revealed serveral failures. All of them appeared to be issues in the testsuite rather than in the crate itself. Some of them affected all 32-bit architectures we tested on where as others only affected a subset. Three of them seem to just be issues in the tests.

  1. "test_bytes" failed to build on all 32-bit architectures tested with "the trait bytes::AtomicAccess is not implemented for u64", I changed "1u64" to "1usize" to fix this.
  2. An out of memory failure in benches/main.rs, this only seems to happen if the "backend-mmap" feature is enabled. I fixed this by using a lower value of REGION_SIZE on 32-bit architectures in benches/mmap/mod.rs
  3. check_byte_valued_type assumed that all types tested would either have natural alignment or would have an alignment of 1. This is not the case for u64/i64 on most 32-bit architectures. I fixed the test to take account of this.

There is also a fourth issue but it is one where I will not blame you if you declare it "out of scope". This fourth issue can only be encountered with a patched vmm-sys-util crate, as that crate currently fails to build at all on architectures that do not support AtomicU64.

src/bitmap/backend/atomic_bitmap.rs will not build on architectures that do not have AtomicU64. The most popular 32-bit architectures do have AtomicU64 but some of the less popular ones do not. This code is included when the "backend-atomic" feature is enabled, it is also included when building tests regardless of the feature configuration.

It seems to me that atomic_bitmap.rs could be made to use AtomicUsize instead of AtomicU64 but writing that modification goes beyond what I'm prepared to do as a distro maintainer.

So for now I prepared a patch that add and adjusts some attributes so it is at least possible to run most of the tests for other feature configurations on architectures that do not have AtomicU64.

https://salsa.debian.org/rust-team/debcargo-conf/-/blob/4b1f227286fa3da2468e98c85bd453c626555c47/src/vm-memory/debian/patches/fix-tests-32-bit.patch

https://salsa.debian.org/rust-team/debcargo-conf/-/blob/4b1f227286fa3da2468e98c85bd453c626555c47/src/vm-memory/debian/patches/skip-tests-atomic-u64.patch

mjt0k commented 1 year ago

Now with vm-memory 0.12 release, it fails in a more difficult way on debian, notable on armel (which also don't have AtomicU64):

https://ci.debian.net/data/autopkgtest/testing/armel/r/rust-vhost-user-backend/35901107/log.gz

234s error[E0432]: unresolved import `std::sync::atomic::AtomicU64`
234s  --> /usr/share/cargo/registry/vm-memory-0.12.0/src/bitmap/backend/atomic_bitmap.rs:6:25
234s   |
234s 6 | use std::sync::atomic::{AtomicU64, Ordering};

Now AtomicU64 is used in actual bitmap implementation, not only in tests. And this fails on platforms where there's no AtomicU64.

Ablu commented 8 months ago

vm-memory does not currently support anything but 64-bit. We are making it more explicit with https://github.com/rust-vmm/vm-memory/pull/275. I would recommend to restrict the package to 64-bit on debian.

Adding 32-bit support may be possible, but so far nobody has stated interest and code assumes 64-bit in a couple of places. So if 32bit support is desired I would recommend filing a new issue to discuss what is needed and how it could be pulled off.