Skip to content

Conversation

@BCSharp
Copy link
Member

@BCSharp BCSharp commented Feb 4, 2025

This replaces the mock ioctl implementation with the actual syscall.

The tricky part of it is that the ioctl prototype is variadic, is not supported by the Mono.Unix shim, and .NET does not yet support variadic P/Invoke calls on POSIX. There is a long discussion on this topic (dotnet/runtime#48796) but after 4 years it does not seem to converge on any solution. As one of the commentators wrote: "perfect is the enemy of good". Hopefully in the next several years this will be sorted out, but in the meantime, an obscure workaround is needed to make a variadic P/Invoke work on ARM64. Too bad that we have to manage that level of calling convention detail and not the compiler, but I know of no alternative.

I put pragma #error there that gets triggered on any new .NET version we haven't tested yet as a remainder to check if the variadic P/Invokes are finally supported, and if not, just advance the .NET version to the next future one for the next round of checks.

A similar solution could be applied to the fcntl call, which is also variadic, and unblock a bunch of newer F_* commands that are currently being rejected by Mono.Unix, but it is not my priority right now (unless I hit a roadblock there trying to run my favourite modules).


ulong cmd = unchecked((ulong)request);

long data = arg switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I should have suggested this in the last PR, but what about using PythonOps.Index(arg)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at it too, but it doesn't do what is needed here:

  • it doesn't handle System.Reflection.Missing
  • it doesn't handle non-standard integer types like uint, long, ulong
  • it returns object so a switch is still needed to cast to an unmanaged type
  • it tries to call __index__ and this is not what Python does here

But now this code is being used in two places so I guess I might as well factor it out. I'm not sure it deserves to be promoted to PythonOps, but something similar is used in termios, except for the error message. Maybe I just leave it here and make internal; termios is already using some helper methods from fcntl, and these two modules are somewhat coupled.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • it doesn't handle non-standard integer types like uint, long, ulong

They don't have a fast path since they're not typically expected in Python code, but they do end up being resolves via __index__ calls.

  • it returns object so a switch is still needed to cast to an unmanaged type

True. Could have an some helpers such as PythonOps.IndexAsLong.

  • it tries to call __index__ and this is not what Python does here

In recent versions of Python, __index__ is supported pretty much everywhere. For example (only tried on 3.10.12, I guess it's always possible they reverted since):

import fcntl
import termios

class Index:
    def __init__(self, idx):
        self.idx = idx
    def __index__(self):
        return self.idx

fcntl.ioctl(0, Index(termios.TIOCGPGRP), b"  ")

# not a valid call, but does throw TypeError: __index__ returned non-int (type bytes)
fcntl.ioctl(0, Index(termios.TIOCGPGRP), Index(b"  "))

Anyway, I don't have an objection to what you're proposing it just looked a lot like what index does.

Copy link
Member Author

Choose a reason for hiding this comment

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

In recent versions of Python, __index__ is supported pretty much everywhere. For example (only tried on 3.10.12, I guess it's always possible they reverted since):

Ah, I see. On maxOS I am using 3.7 and on Linux 3.6 for testing (which are the oldest I could easily get, 3.4 is nowhere easily available anymore). I see that 3.8 – 3.12 all support __index__. It's moving target…

Could have an some helpers such as PythonOps.IndexAsLong.

I like the idea. From my memory, this or a similar conversion is happening in more places. I think PythonOps.TryGetInt64 that doesn't do unnecessary try/catch would be more practical. Anyway, I'll keep it in mind, this is not something for this PR.

@BCSharp
Copy link
Member Author

BCSharp commented Feb 4, 2025

I forgot to mention, the code does not support ioctl on Mono; I couldn't find a simple way on Mono to detect the ARM64 architecture to do the vararg workaround.

@slozier
Copy link
Contributor

slozier commented Feb 4, 2025

Side note since I know nothing about these functions I looked up the Python docs and the example they give is using a str argument:

fcntl.ioctl(0, termios.TIOCGPGRP, "  ")

I forgot to mention, the code does not support ioctl on Mono; I couldn't find a simple way on Mono to detect the ARM64 architecture to do the vararg workaround.

No problem for Mono. Feels like it's effectively abandoned anyway?

@BCSharp
Copy link
Member Author

BCSharp commented Feb 4, 2025

Side note since I know nothing about these functions I looked up the Python docs and the example they give is using a str argument:

It's a good point. I saw it but concluded that this is an artifact from the 2.x era — it used to be str now it's bytes. This doc page is misleading in a few other ways too. This is a very strangely defined interface, but there are some good reasons behind it. The sole purpose of the third argument for output (i.e. "get") commands like TIOCGPGRP, is to indicate how big the buffer used for the syscall should be; the content is irrelevant. It cannot be int because for a variadic call like this it would be interpreted as an address. On the other hand, CPython is not interpreting the commands in any way, so it does not know when it should be int or an address of a buffer. This the responsibility of the user, and left up to OS to interpret.

So ideally the doc should read:

fcntl.ioctl(0, termios.TIOCGPGRP, b'\0' * 4)

which clearly shows how many bytes the OS buffer should be (on macOS it's 4, on Linux 2 minimum). But when I tested the code, str still works on anything up to the 3.12. Moreover, 2to3 does not migrate those ioctl or fcntl calls from str to bytes, so I wouldn't be surprised if there is some code around that still uses str. It still does get converted to bytes under the hood, which makes using str a particularly bad choice to convey the length of the buffer (it is encoded using UTF-8, which, unless the content of the string is ASCII, will not be the same length as the buffer) but I will implement it for compatibility sake.

@BCSharp
Copy link
Member Author

BCSharp commented Feb 4, 2025

Another note about the misleading documentation. The part on ioctl reads:

If a mutable buffer is passed, then the behaviour is determined by the value of the mutate_flag parameter.

If it is false, the buffer’s mutability is ignored and behaviour is as for a read-only buffer, except that the 1024 byte limit mentioned above is avoided – so long as the buffer you pass is at least as long as what the operating system wants to put there, things should work.

This is not correct. "Things" might not work. If mutate_flag is false, the buffer will be treated as read-only indeed, but then the code needs another writable buffer for the output from the syscall, which, in CPython's case, is 1024 bytes reserved on the stack. So if the length of the read-only buffer (even if it technically mutable, like bytearray) exceeds the 1024 limit, an error is raised nevertheless. This looks like an implementation artifact, and perhaps what the doc says was the original intention of the interface, but it is not how CPython does it. In IronPython, because the buffer is not on stack but a managed array, we do not have this limitation, and could pretty easily implement the behaviour as prescribed by the documentation. However, again for the sake of compatibility, I followed the actual CPython's behaviour.

@BCSharp BCSharp merged commit b5fce64 into IronLanguages:main Feb 5, 2025
8 checks passed
@BCSharp BCSharp deleted the ioctl_syscall branch February 5, 2025 19:29
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