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 temporary references for volatile read #12

Merged
merged 2 commits into from
Sep 23, 2022
Merged

Conversation

bdbai
Copy link
Contributor

@bdbai bdbai commented Aug 7, 2022

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
Copy link

CryZe commented Sep 22, 2022

Bump, as this compatibility lint reached stable today.

@poliorcetics
Copy link

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

@bdbai
Copy link
Contributor Author

bdbai commented Sep 23, 2022

@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
Copy link

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
Copy link
Contributor Author

bdbai commented Sep 23, 2022

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?

@MSxDOS MSxDOS merged commit 24fc1e4 into MSxDOS:master Sep 23, 2022
tesaguri added a commit to tesaguri/pipitor that referenced this pull request Feb 19, 2023
This fixes a compile error of a dependency.

cf. <MSxDOS/ntapi#12>
tesaguri added a commit to tesaguri/pipitor that referenced this pull request Feb 19, 2023
This fixes a compile error of a dependency.

cf. <MSxDOS/ntapi#12>
tesaguri added a commit to tesaguri/pipitor that referenced this pull request Feb 19, 2023
This fixes a compile error of a dependency.

cf. <MSxDOS/ntapi#12>
tesaguri added a commit to tesaguri/pipitor that referenced this pull request Feb 19, 2023
This fixes a compile error of a dependency.

cf. <MSxDOS/ntapi#12>
tesaguri added a commit to tesaguri/pipitor that referenced this pull request Feb 19, 2023
This fixes a compile error of a dependency.

cf. <MSxDOS/ntapi#12>
@hecrj
Copy link

hecrj commented Mar 7, 2023

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
Copy link
Contributor Author

bdbai commented Mar 9, 2023

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

@hecrj
Copy link

hecrj commented Mar 9, 2023

@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
Copy link

ryoqun commented Apr 26, 2023

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
Copy link

ryoqun commented May 2, 2023

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 ryoqun mentioned this pull request May 11, 2023
ryoqun pushed a commit to ryoqun/ntapi that referenced this pull request Jun 4, 2023
* Bump Rust toolchain version for CI to 1.64

* Fix temporary references for volatile read
ryoqun added a commit to ryoqun/solana that referenced this pull request Jun 5, 2023
ryoqun added a commit to ryoqun/solana that referenced this pull request Jun 5, 2023
ryoqun added a commit to solana-labs/solana that referenced this pull request Jun 5, 2023
* Patch ntapi to restore windows build

* Update Cargo.lock...

* Add comment for justification of this patching

MSxDOS/ntapi#11
MSxDOS/ntapi#12

* Revert "ci: stop windows building on master temporarily (#31353)"

This reverts commit 2dcdfff.

* Use solana-labs fork

* Ugh..
ryoqun pushed a commit to solana-labs/ntapi that referenced this pull request Jun 6, 2023
* Bump Rust toolchain version for CI to 1.64

* Fix temporary references for volatile read
ryoqun added a commit to ryoqun/solana that referenced this pull request Jun 6, 2023
* Patch ntapi to restore windows build

* Update Cargo.lock...

* Add comment for justification of this patching

MSxDOS/ntapi#11
MSxDOS/ntapi#12

* Revert "ci: stop windows building on master temporarily (solana-labs#31353)"

This reverts commit 2dcdfff.

* Use solana-labs fork

* Ugh..
ryoqun added a commit to solana-labs/solana that referenced this pull request Jun 6, 2023
* Shift crossbeam comment for upcoming 2nd patch... (#31963)

* Patch ntapi to restore windows build (#31961)

* Patch ntapi to restore windows build

* Update Cargo.lock...

* Add comment for justification of this patching

MSxDOS/ntapi#11
MSxDOS/ntapi#12

* Revert "ci: stop windows building on master temporarily (#31353)"

This reverts commit 2dcdfff.

* Use solana-labs fork

* Ugh..

* Patch ntapi more thoroughly (#31970)

* Patch spl-token-cli build as well...

* Patch sbf/Cargo.toml for consistency

* Remove --locked for cli-arg based patch... (#31971)

* Bump patched ntapi from v0.3.6 to v0.3.7 (#31981)

* Revert "ci: stop windows build (#31893)"

This reverts commit 30f9e43.
@ryoqun
Copy link

ryoqun commented Jun 9, 2023

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

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.

ntapi 0.3.7 contains code that will be incompatible with future versions of Rust
6 participants