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

Implement C long as 64-bit on POSIX #1918

Merged
merged 2 commits into from
Feb 23, 2025
Merged

Conversation

BCSharp
Copy link
Member

@BCSharp BCSharp commented Feb 23, 2025

The size of c_long in ctypes was hardcoded to 32-bit, which is correct on Windows, but not correct on 64-bit GCC-compatible systems, like Linux and macOS, where long is 64-bit. This is not limited to ctypes but extends to all l and L typecode usage (StdLib checks that size of c_long matches size of struct('l')). For instance, here is an excerpt from test_fcntl in StdLib, assuming the test runs on Linux:

start_len = "ll"
lockdata = struct.pack('hh'+start_len+'hh', fcntl.F_WRLCK, 0, 0, 0, 0, 0)
fcntl.fcntl(self.f.fileno(), fcntl.F_SETLKW, lockdata)

If l is 4 bytes, it will create the struct that is 8 bytes too short. The test passes because IronPython (like CPython) always reserves at least 1 KiB of the buffer for the fcntl call and (unlike CPython) zeroes the buffer first, so that the syscall incidentally sees the proper numbers of zeros. But in a general case, if the parameters of lockdata (struct flock) are misalligned, the lock will not work correctly. For instance struct.pack('hhllhh', fcntl.F_WRLCK, 0, 0, 8, 0, 0) (lock 8 bytes from position 0) will be understood by the OS as struct.pack('hhqqhh', fcntl.F_WRLCK, 0, 8 << 32, 0, 0, 0) (lock 0 bytes form position 34359738368).

The required change for l/L to become 64-bit on some systems was surprisingly sprawling, so I tried to limit the PR to just that. But I do have similar concerns about n/N, which in CPython maps to ssize_t/size_t, and u which maps to wchar_t, but that is outside the scope of this PR.


Side note: the changes to _ctypes_test.c are substantial but actually pretty simple: I reverted the file to the original version as it is in the 3.4 branch of CPython (with only small removals of CPython specific code). Apparently, at some point it got tweaked in IronPython to make the tests passing, perhaps just to work around the long-size mismatch. This is no longer necessary. However, the test code has been expanded in 3.6, afaik in a backward compatible way, so perhaps it would be worthwhile to use that version to simplify the maintenance of our 3.4 and 3.6, or just upgrade it on the 3.6 side only.

Copy link
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

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

Thanks! I was expecting to have a debate on this (about matching with .NET types and long being dependant on the compiler) when I first read the issue but looking at the implementation it all seems very reasonable.

But I do have similar concerns about n/N, which in CPython maps to ssize_t/size_t, and u which maps to wchar_t, but that is outside the scope of this PR.

For n/N my personal preference would be having them line up with nint/nuint. As for u, it's deprecated (since 3.3), so I don't think we should necessarily be investing effort into it (unless there's a specific scenario you need it for).

However, the test code has been expanded in 3.6, afaik in a backward compatible way, so perhaps it would be worthwhile to use that version to simplify the maintenance of our 3.4 and 3.6, or just upgrade it on the 3.6 side only.

I've been trying to minimize the delta between the 3.4 and 3.6 branches so just using the code from 3.6 sounds like a fine idea (if it doesn't break things).

@BCSharp
Copy link
Member Author

BCSharp commented Feb 23, 2025

For n/N my personal preference would be having them line up with nint/nuint

Indeed, this is my preference too. I believe that it would imply the change to sys.maxsize as well. My hesitation about the change is that .NET indices are essentially fixed to 32-bit (with a few exceptions), and ssize_t is basically the type of an array index (i.e. an index-sized integer). In a number of places, IronPython reports IndexError with a different message [^1] if the value does not fit in Int32. Perhaps that's not a problem. I didn't open an issue about this yet because I guess what is needed first is simply try the change and see what side effects it has, to have something more than a theoretical discussion.

As for u, it's deprecated (since 3.3), so I don't think we should necessarily be investing effort into it (unless there's a specific scenario you need it for).

No, I don't have a specific need for it, afaics. I think it is actually fortunate that it is being deprecated, meaning there is less of a change of existing Python modules using it, which means we are free to repurpose it for System.Char, as it is now, without risking conflicts with the existing Python code.

I've been trying to minimize the delta between the 3.4 and 3.6 branches so just using the code from 3.6 sounds like a fine idea (if it doesn't break things).

OK. I'll submit a separate PR with the change — for the record.


[1]: Example:

>>> b""[1 << 31]      
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: cannot fit 'int' into an index-sized integer
>>> b""[(1 << 31) - 1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: index out of range: 2147483647

@BCSharp BCSharp merged commit b234b40 into IronLanguages:main Feb 23, 2025
8 checks passed
@BCSharp BCSharp deleted the posix_long branch February 23, 2025 23:22
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.

2 participants