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

Bump patched ntapi from v0.3.6 to v0.3.7 #31981

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

ryoqun
Copy link
Member

@ryoqun ryoqun commented Jun 6, 2023

Problem

the solana git repo and published spl crate are using different versions of ntapi, which is troubling consistent [patch]-ing. the repo had been pinning to v0.3.6 via Cargo.lock. but the published spl crate is specifying v0.3.x and resolved to latest one: v0.3.7

Summary of Changes

Just use same version, restoring --locked.

so, this effectively bumps monorepo's ntapi from v0.3.6 to v0.3.7, which seems to contain only small changes:

MSxDOS/ntapi@cbea31e...3a0063a according to:

$ git diff --no-index ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.{6,7}/.cargo_vcs_info.json
diff --git a/home/ryoqun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.6/.cargo_vcs_info.json b/home/ryoqun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.7/.cargo_vcs_info.json
index 8ae34a3dff4..d974c8a1461 100644
--- a/home/ryoqun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.6/.cargo_vcs_info.json
+++ b/home/ryoqun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.7/.cargo_vcs_info.json
@@ -1,5 +1,6 @@
 {
   "git": {
-    "sha1": "cbea31ea9d624169f01ae0997e6698baceb51867"
-  }
-}
+    "sha1": "3a0063ad916e5f0c6972ab53e025689b9f0f92ad"
+  },
+  "path_in_vcs": ""
+}
\ No newline at end of file

new patch commit: solana-labs/ntapi@97ede98 :

$ git diff --no-index ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.7/src/ ./src/
diff --git a/home/ryoqun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.7/src/ntexapi.rs b/./src/ntexapi.rs
index 5fa47c9..d48749d 100644
--- a/home/ryoqun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ntapi-0.3.7/src/ntexapi.rs
+++ b/./src/ntexapi.rs
@@ -1,4 +1,6 @@
 use core::mem::uninitialized;
+#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))]
+use core::ptr::addr_of;
 use core::ptr::read_volatile;
 #[cfg(target_arch = "x86")]
 use core::sync::atomic::spin_loop_hint;
@@ -2780,7 +2782,7 @@ pub const USER_SHARED_DATA: *const KUSER_SHARED_DATA = 0x7ffe0000 as *const _;
 pub unsafe fn NtGetTickCount64() -> ULONGLONG {
     let mut tick_count: ULARGE_INTEGER = uninitialized();
     #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] {
-        *tick_count.QuadPart_mut() = read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad);
+        *tick_count.QuadPart_mut() = read_volatile(addr_of!((*USER_SHARED_DATA).u.TickCountQuad));
     }
     #[cfg(target_arch = "x86")] {
         loop {
@@ -2804,7 +2806,7 @@ pub unsafe fn NtGetTickCount64() -> ULONGLONG {
 #[inline]
 pub unsafe fn NtGetTickCount() -> ULONG {
     #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] {
-        ((read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad)
+        ((read_volatile(addr_of!((*USER_SHARED_DATA).u.TickCountQuad))
             * (*USER_SHARED_DATA).TickCountMultiplier as u64) >> 24) as u32
     }
     #[cfg(target_arch = "x86")] {

so, hopefully, this marks the end of window build restoration journey... this and all previous prs will be backported as-is.

@ryoqun ryoqun requested a review from yihau June 6, 2023 02:05
Comment on lines +145 to +150
# the patch-related configs are needed for rust 1.69+ on Windows; see Cargo.toml
# shellcheck disable=SC2086 # Don't want to double quote $rust_version
"$cargo" $maybeRustVersion \
--config 'patch.crates-io.ntapi.git="https://github.com/solana-labs/ntapi"' \
--config 'patch.crates-io.ntapi.rev="5980bbab2e0501a8100eb88c12222d664ccb3a0a"' \
install spl-token-cli --root "$installDir"
--config 'patch.crates-io.ntapi.rev="97ede981a1777883ff86d142b75024b023f04fad"' \
install --locked spl-token-cli --root "$installDir"
Copy link
Member Author

@ryoqun ryoqun Jun 6, 2023

Choose a reason for hiding this comment

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

@yihau as you suggested at solana-labs/solana-program-library#4471 (comment), we can switch from building to downloading some existing spl tarball from somewhere. seems solang is taking this approach as shown below this file. I have no particular preference; but seeing the below code, needs some work for factoring the below download logic, etc (including testing on Windows). so, moar effort, imo.

So, overall, i think [patch]-ing is acceptable workaround for now. (after all, this won't be needed, once tokio is upgraded).

Copy link
Member Author

Choose a reason for hiding this comment

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

also, this is a bit too theoretical. but this bundled spl-token binary is used rather widely across various integrations. they seldom use cargo install .... so, i think using the same monorepo-pinned rust version is conceptually easier to reason about. admittedly, it's rare for rust to expose quite visible runtime/compile differences between versions. but this is possibility.

Also, it's more easy to ship new binary once rust found to contain critical security vuln (this again very unlikely)

Copy link
Member

Choose a reason for hiding this comment

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

sure. let's do this!

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #31981 (7455754) into master (6b33ff8) will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #31981     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         760      760             
  Lines      207407   207407             
=========================================
- Hits       169958   169950      -8     
- Misses      37449    37457      +8     

Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

🪖

@ryoqun ryoqun merged commit 4227d0e into solana-labs:master Jun 6, 2023
6 checks passed
ryoqun added a commit to ryoqun/solana that referenced this pull request Jun 6, 2023
ryoqun added a commit 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.
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.

None yet

2 participants