search

MSxDOS / ntapi

Rust FFI bindings for Native API
Apache License 2.0
100 stars 33 forks source link

Fix temporary references for volatile read #12

Closed bdbai closed 2 years ago

bdbai commented 2 years ago

Taking a reference to an unaligned field is an undefined behavior, even if it is converted to a pointer immediately. Although the memory access here must be aligned given the fixed base address and the offset of the field, rustc still issue a warning due to the struct being 4 packed. Here we can use core::ptr::addr_of to create a pointer to a field without introducing a reference.

Fixes #11.

CryZe commented 2 years ago

Bump, as this compatibility lint reached stable today.

poliorcetics commented 2 years ago

The CI is failing because core::ptr::addr_of was stabilised in 1.51, MSRV needs to be updated

bdbai commented 2 years ago

@poliorcetics thanks for pointing that out! I have raised #13 and hopefully will unblock this PR once merged. Raising the CI toolchain version without fixing the errors does not work. They have to be done in one go.

poliorcetics commented 2 years ago

I agree the CI checks don't affect the rest, but keeping 1.51 as the minimum check in CI means new code will less easily break it. I'm personally not affected by high MSRVs but I think keeping it to three or four versions back at least is nice for people who can only update their compiler after some time (notably in entreprise projects)

bdbai commented 2 years ago

keeping 1.51 as the minimum check in CI means new code will less easily break it

Valid point! I have never thought of that. However, the msvc targets in the recipe even use nightly without a fixed date. I am not sure to what extent the author wants to maintain the compatibility.

Maybe @MSxDOS can explain a bit so that the others can help?

hecrj commented 1 year ago

Any chance this fix could be backported and released as 0.3.8? The beta toolchain seems to be erroring out on these already.

bdbai commented 1 year ago

@hecrj Changes in 0.4.0 are mostly replacing deprecated APIs, so I assume you may safely upgrade from 0.3.7.

hecrj commented 1 year ago

@bdbai I am not depending on ntapi directly, but through sysinfo. I fixed the issue by simply updating it, but it would still be great to release a patch with the fixes to avoid broken builds throughout the ecosystem.

ryoqun commented 1 year ago

Any chance this fix could be backported and released as 0.3.8? The beta toolchain seems to be erroring out on these already.

hey, we second on this. really appreciate 0.3.8 just with these small patch only. we are stuck like this (a bit outdated) tokio => mio => ntapi and our outdated mio's Cargo.toml declares 0.3 on ntapi: https://crates.io/crates/mio/0.7.14/dependencies

cc: @yihau

ryoqun commented 1 year ago

Any chance this fix could be backported and released as 0.3.8? The beta toolchain seems to be erroring out on these already.

hey, we second on this. really appreciate 0.3.8 just with these small patch only. we are stuck like this (a bit outdated) tokio => mio => ntapi and our outdated mio's Cargo.toml declares 0.3 on ntapi: https://crates.io/crates/mio/0.7.14/dependencies

cc: @yihau

@MSxDOS : friendly ping. :) hope releasing this small 0.3.8 won't take much time.

ryoqun commented 1 year ago

fyi: for anyone who is still stuck at ntapi 0.3.x, I've patched ntapi like this: https://github.com/solana-labs/solana/pull/31982