fix up #[cfg(...)] related to test code that uses dirty bitmaps #331
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roypat
requested review from
alexandruag,
bonzini,
jiangliu and
ShadowCurse
as code owners
roypat
force-pushed
the
bitmap-test-cleanup
branch
2 times, most recently
from
34a4ded to
72c93af
Compare
andreeaflorescu
previously approved these changes
Jul 3, 2025
ShadowCurse
previously approved these changes
Jul 3, 2025
BytesHelper::test_access() returned a result that was always immediately unwrapped. Just assert in the function directly instead. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The usesite of this import was behind a cfg, so some feature combos printed a warning about unused imports. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
backend-bitmap uses libc to determine host page size in atomic_bitmap.rs. Currently, commands such as cargo build --no-default-features --features backend-bitmap fail, because libc is an optional dependency, and only enabled if we also enable backend-mmap or rawfd (which there is no reasonable setup in which this wasnt the case, explaining why no one ever ran into this). Technically the libc dependency is only needed on target_family = "unix", but there's no way to enable a feature only on a specific target, and on non-unix systems the libc call is disabled via cfg-ery, so won't cause any problems. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
This overly complex agglomeration of function pointers and closures was used to... still write the same number of LoC in the end. So just eliminate it and inline it into its one use site. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The bitmap backend implementation in vm_memory::bitmap::backend is only exported if the backend-bitmap feature is enabled. However, compiling the crate in test mode (e.g. `cargo test`), also unconditionally enables it. This is weird, and actually causes problems: The bitmap backend code depends on the libc crate, and this weird "enable unconditionally for tests" behavior means we have to go through more trouble in Cargo.toml to get the code to compile in some feature combinations, as not only do we need to add the libc crate as a dependency of the backend-bitmap feature, we also have to add it as a dev-dependency. Add a #[cfg(feature = "backend-bitmap")] to the test module in src/bitmap/mod.rs, as otherwise some of the functions will be warned about being unused in some feature configurations. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Now that the relevant code is no longer unconditionally enabled in cfg(test), explicitly enable the feature for miri Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
bitmap-test-cleanup
branch
from
72c93af to
c7d9c3d
Compare
|
@andreeaflorescu @ShadowCurse sorry, this didnt cleanly rebase on top of main after merging #312, could you reapprove? |
ShadowCurse
approved these changes
Jul 3, 2025
stefano-garzarella
approved these changes
Jul 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, vm-memory doesn't compile on all feature combinations:
cargo check --no-default-features --features backend-bitmapdoes not compile because inbitmap/backend/atomic_bitmap.rswe try to use thelibccrate to determine page size at some point.libcis an optional dependency andbackend-bitmapdoes not enable it. Fixing this for production code is, of course, a one-liner inCargo.toml. However, nothing is ever that straightforward. Turns out our test code use some#[cfg(test)]directives to bypass cargo features, and always compile in thebitmap/backend/mod.rsmodule. This meanscargo test --no-default-featuresis still broken. Instead of hacking around this by simply declaring libc an unconditional dev-dependency, ensure that tests that depend on bitmaps being available have the correct feature cfgs on them, and then drop thecfg(test)stuff.There's a lot more cleanup that can be done here (all functions in
bitmap/mod.rsprobably do not belong there), but this is all I'm willing to do for today.Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.