ports/unix/modffi: Fix signed integer cast in return_ffi_value. #15767
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15767 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 163 163
Lines 21294 21294
=======================================
Hits 20960 20960
Misses 334 334 ☔ View full report in Codecov by Sentry. |
|
Code size report: |
mchlswyr
changed the title
Casting an ffi_arg to a signed int may truncate the value. E.g., when the ffi_arg is 64-bit and the signed int is 32-bit. Also, casting an ffi_arg to a larger signed type will not sign extend the value. E.g., when the ffi_arg is 32-bit and the larger signed type is int64_t. If the value is signed, it should be cast to ffi_sarg, which is the same size as ffi_arg. Signed-off-by: Michael Sawyer <mjfsawyer@gmail.com>
Added the "long" modffi tests. The tests could not be added to the existing ffi_types test because two .exp files were required for the 32-bit and 64-bit results. Code common to both the ffi_types and type "long" tests was factored into ffi_int_base. ffi_types was renamed to ffi_int_types to group the related tests under the "ffi_int" prefix. Signed-off-by: Michael Sawyer <mjfsawyer@gmail.com>
projectgus
approved these changes
Sep 10, 2024
Contributor
projectgus
left a comment
projectgus
left a comment
There was a problem hiding this comment.
This looks very good to me, especially the additional test coverage. Thanks, @mchlswyr!
dpgeorge
approved these changes
Sep 10, 2024
Member
dpgeorge
left a comment
dpgeorge
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR builds on #7358. In
return_ffi_value, integers of typechar,short, andlongare cast to typeint. In an x86-64 Linux build,longis 64-bit andintis 32-bit, so the value is truncated.Testing
I added some
longtests. The tests could not be added to the existing ffi_types test because two .exp files were required for the 32-bit and 64-bit results. Code common to both the ffi_types andlongtests was factored into ffi_int_base. ffi_types was renamed to ffi_int_types to group the related tests under the "ffi_int" prefix.I ran the tests in the following four cases:
MICROPY_FORCE_32BIT=0: ffi_int_long64 will fail;MICROPY_FORCE_32BIT=1: all tests will pass or skip;MICROPY_FORCE_32BIT=0: all tests will pass or skip;MICROPY_FORCE_32BIT=1: all tests will pass or skip.