Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix riscv32 atomics and fix tests on 32-bit platforms #533

Merged
merged 6 commits into from
Apr 8, 2022

Conversation

xobs
Copy link
Contributor

@xobs xobs commented Apr 1, 2022

This fixes tests on 32-bit platforms. Previously, the Encode would encode a usize and decode a u64. This works fine on 64-bit platforms, but on 32-bit platforms this results in a mismatch.

Force usize to be encoded as a u64 even on 32-bit platforms.

Additionally, the 32-bit riscv target does not implement AtomicU64 or AtomicI64, so remove those impelementations on that platform.

xobs added 2 commits April 1, 2022 16:18
This test checks the range, which is necessarily different when running
on another platform.

Signed-off-by: Sean Cross <[email protected]>
The deserialization process assumes that usize/isize are 64-bits, as
does the spec at
https://github.com/bincode-org/bincode/blob/trunk/docs/spec.md#varintencoding

Force `usize` and `isize` to be encoded as `u64` and `i64` to force
32-bit platforms to conform to this spec.

This fixes running tests under `cargo test --target i686-pc-windows-msvc`

Signed-off-by: Sean Cross <[email protected]>
Not all platforms support AtomicI64 or AtomicU64. Use the
`target_has_atomic` config flag to determine whether to include these.

This fixes bincode-org#532.

Signed-off-by: Sean Cross <[email protected]>
@xobs xobs force-pushed the fix-tests-32-bit branch from a309baf to a5219fa Compare April 1, 2022 10:22
Copy link
Contributor

@VictorKoenders VictorKoenders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could make this bigger and add target_has_atomic to all atomic impls, and deprecate the atomic feature

src/features/atomic.rs Outdated Show resolved Hide resolved
src/features/atomic.rs Outdated Show resolved Hide resolved
Now that there is an attribute to indicate which atomic features are
supported on this platform, remove the `atomic` Feature and use these
new attributes.

Signed-off-by: Sean Cross <[email protected]>
@xobs
Copy link
Contributor Author

xobs commented Apr 2, 2022

Thanks for the suggestions. I've annotated all Atomic implementations.

I assume that Bool is the same as 8, and usize is the same as ptr.

I'm not sure what the best way is to deprecate the feature, so I removed it wholesale.

@codecov
Copy link

codecov bot commented Apr 2, 2022

Codecov Report

Merging #533 (b7f1a1d) into trunk (36ab23c) will decrease coverage by 1.36%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            trunk     #533      +/-   ##
==========================================
- Coverage   70.82%   69.45%   -1.37%     
==========================================
  Files          47       46       -1     
  Lines        3215     3179      -36     
==========================================
- Hits         2277     2208      -69     
- Misses        938      971      +33     
Impacted Files Coverage Δ
src/atomic.rs 0.00% <ø> (ø)
src/lib.rs 68.75% <ø> (ø)
tests/alloc.rs 92.42% <ø> (ø)
tests/atomic.rs

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36ab23c...b7f1a1d. Read the comment docs.

@VictorKoenders
Copy link
Contributor

Can you pull in the new cross-platform build pipeline, and see if bincode now compiles for your target arch?

@xobs
Copy link
Contributor Author

xobs commented Apr 8, 2022

It does build for my platform still, so it looks good to me!

[10:10:39 am] E:/Code/bincode> cargo build --lib --target riscv32imac-unknown-xous-elf
   Compiling virtue v0.0.7
   Compiling bincode_derive v2.0.0-rc.1 (E:\Code\bincode\derive)
   Compiling bincode v2.0.0-rc.1 (E:\Code\bincode)
    Finished dev [unoptimized + debuginfo] target(s) in 2.39s
[10:10:43 am] E:/Code/bincode>

Signed-off-by: Sean Cross <[email protected]>
@VictorKoenders
Copy link
Contributor

If you run cargo fmt and cargo clippy I think this is good to merge

@xobs
Copy link
Contributor Author

xobs commented Apr 8, 2022

Alright, I just ran cargo fmt to fix that up.

@VictorKoenders VictorKoenders merged commit 2d3f420 into bincode-org:trunk Apr 8, 2022
@VictorKoenders
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants