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 128-bit int regression on big-endian with Python <3.13 #4291

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

musicinmybrain
Copy link
Contributor

Fixes #4290.

As described in the issue, a change from Ok(<$rust_type>::from_le_bytes(buffer)) to Ok(<$rust_type>::from_ne_bytes(buffer)) was unconditional, but was only appropriate when the preceding Python-3.13-only block was compiled. This PR makes that change conditional on Python version, too.

I tested this as a patch on all Fedora releases and EPEL9 and confirmed the tests now pass on all Fedora primary architectures, for both Python 3.13 and previous interpreter versions.


Thank you for contributing to PyO3!

By submitting these contributions you agree for them to be dual-licensed under PyO3's MIT OR Apache-2.0 license.

Please consider adding the following to your pull request:

  • an entry for this PR in newsfragments - see [https://pyo3.rs/main/contributing.html#documenting-changes]
    • or start the PR title with docs: if this is a docs-only change to skip the check
  • docs to all new functions and / or detail in the guide
  • tests for all new or changed functions

PyO3's CI pipeline will check your pull request, thus make sure you have checked the Contributing.md guidelines. To run most of its tests
locally, you can run nox. See nox --list-sessions
for a list of supported actions.

@decathorpe
Copy link
Contributor

This might be a slightly simpler version of the fix (also confirmed to work) which keeps the number of cfg-gated blocks to 2 instead of adding a third one:

diff --git a/src/conversions/std/num.rs b/src/conversions/std/num.rs
index effe7c7..1304e73 100644
--- a/src/conversions/std/num.rs
+++ b/src/conversions/std/num.rs
@@ -238,15 +238,18 @@ mod fast_128bit_int_conversion {
                         unsafe { ffi::PyNumber_Index(ob.as_ptr()).assume_owned_or_err(ob.py())? };
                     let mut buffer = [0u8; std::mem::size_of::<$rust_type>()];
                     #[cfg(not(Py_3_13))]
-                    crate::err::error_on_minusone(ob.py(), unsafe {
-                        ffi::_PyLong_AsByteArray(
-                            num.as_ptr() as *mut ffi::PyLongObject,
-                            buffer.as_mut_ptr(),
-                            buffer.len(),
-                            1,
-                            $is_signed.into(),
-                        )
-                    })?;
+                    {
+                        crate::err::error_on_minusone(ob.py(), unsafe {
+                            ffi::_PyLong_AsByteArray(
+                                num.as_ptr() as *mut ffi::PyLongObject,
+                                buffer.as_mut_ptr(),
+                                buffer.len(),
+                                1,
+                                $is_signed.into(),
+                            )
+                        })?;
+                        Ok(<$rust_type>::from_le_bytes(buffer))
+                    }
                     #[cfg(Py_3_13)]
                     {
                         let mut flags = ffi::Py_ASNATIVEBYTES_NATIVE_ENDIAN;
@@ -272,8 +275,8 @@ mod fast_128bit_int_conversion {
                                 "Python int larger than 128 bits",
                             ));
                         }
+                        Ok(<$rust_type>::from_ne_bytes(buffer))
                     }
-                    Ok(<$rust_type>::from_ne_bytes(buffer))
                 }
 
                 #[cfg(feature = "experimental-inspect")]

@musicinmybrain
Copy link
Contributor Author

This might be a slightly simpler version of the fix (also confirmed to work) which keeps the number of cfg-gated blocks to 2 instead of adding a third one:

I think this is a clear improvement over the original, which was:

diff --git a/src/conversions/std/num.rs b/src/conversions/std/num.rs
index effe7c7c..6841e9dc 100644
--- a/src/conversions/std/num.rs
+++ b/src/conversions/std/num.rs
@@ -272,8 +272,10 @@ mod fast_128bit_int_conversion {
                                 "Python int larger than 128 bits",
                             ));
                         }
+                        Ok(<$rust_type>::from_ne_bytes(buffer))
                     }
-                    Ok(<$rust_type>::from_ne_bytes(buffer))
+                    #[cfg(not(Py_3_13))]
+                    Ok(<$rust_type>::from_le_bytes(buffer))
                 }
 
                 #[cfg(feature = "experimental-inspect")]

Your version not only reduces the number of conditionals, but keeps related code close together.

I will amend and force-push.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for fixing, and sorry for the pain caused!

@davidhewitt davidhewitt added this pull request to the merge queue Jun 26, 2024
Merged via the queue into PyO3:main with commit 8f7450e Jun 26, 2024
41 checks passed
@decathorpe
Copy link
Contributor

No problem. Thank you for the quick fix!

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.

many 128-bit number conversion tests fail on s390x-unknown-linux-gnu (big-endian)
3 participants