-
Notifications
You must be signed in to change notification settings - Fork 19
Handle AT_FDCWD
in C to eliminate Obj.magic
#128
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks right to me, but not merging yet as I haven't run an e2e test on eio
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my understanding of the calling convention for optional arguments is correct, even with the default in the code and a non-opaque interface, the option is still allocated at call-sites
That's disappointing. But I agree the new code is cleaner anyway.
This isn't an objection to this PR, but I wanted to comment on the blog post you linked elsewhere (File descriptors are not integers):
I think the blog is trying to argue that FDs-as-ints is just a C wart and therefore OCaml code should never see them (and there should be no way of getting the For example, we could easily imagine rewriting liburing in OCaml. Then the OCaml code would certainly need to know the integer value in order to write it to the ring buffer. The idea that OCaml shouldn't see the ints pushes us in the direction of using more C code, which I dislike. Having access to the ints is dangerous, and I'd much prefer dangerous code to be in OCaml where it's easier to keep it under control. With all that said, I don't particularly mind this PR. The logic being moved to C is only a few lines, and I don't think it matters either way. The main advantage is it works around the lack of a |
My takeaway from the blog is really that Windows file descriptors genuinely aren't ints; on Linux as @talex5 notes they are most definitely integers (actually, just offsets into a kernel-side table) on every Unix I've ever used. So for Linux-specific code, I am in favour of having a I'm in favour of this PR because it's patently obvious from ocaml/ocaml#14258 that any use of Obj should be avoided where possible, and shifting our representation of fds (on Linux only) will make this possible. Perhaps we should be plumbing this deeper into Eio_linux and just not using Unix.file_descr until the last possible minute that it has to be passed into an existing Unix function... |
The point I was trying to make when considering the C type as abstract is that they are not general integers - yes, they are C integers and we then choose in the implementation in OCaml to represent those as OCaml integers an I contend exposing that is the danger. It the C fd were a pointer (or, perish the thought, if you’d had a Unix system which used the full range of I’m a pragmatist - give me an OS with OCaml back to the kernel and we’ll do that (if the bindings were written using ctypes, we’d possibly have a different story) - but while there is C code doing the glue, I think those boundaries are better respected. On a similar pragmatic note, there will never ever be |
Oh, by the by - if it turns out that the option can be better optimised, my suggestion instead would have been to retrieve the magic value from C at program startup |
I think I understand your point now @dra27, but let me restate it to check. It is that the OCaml representation of an integer is different from a C one (the high bit) and so we need to do a representation transformation to turn it into a C fd anyway. And to do that, we should keep the type abstract and only expose safe conversions to and from Unix.file_descr and C ints. Is that right? I think there are three uses of an abstract unix fd:
|
Yes! And indeed in dra27/ocaml@562a497 there's a a logging function for the OCaml side. Key to all of it - and a flaw in OCaml's Unix API at the moment - is I absolutely think you should be able to keep that type abstract in OCaml without incurring any additional conversions or OCaml/C call boundaries (the latter is what I dislike about #129 - no magic, but an extra crossing from OCaml to C). The C API part of it I propose in dra27/ocaml@112b274 also very intentionally inlines down to just the |
Incidentally, I haven't double-checked the details on clambda, but the fact that both flambda 1 and 2 optimise optional arguments properly means that the approach in this specific PR should be different, and I'll push that suggestion at some point in the coming days. |
Thanks for thinking this through so carefully. In terms of uring, we have other allocations can easily elide more (such as the option allocation for ring responses) but am saving that for a future oxcaml port instead... |
As an amusing idle thought, the hard fd limit |
Eliminates both the need to probe
AT_FDCWD
and the need to useObj.magic
in the library to pass it to the C stubs.If my understanding of the calling convention for optional arguments is correct, even with the default in the code and a non-opaque interface, the option is still allocated at call-sites, so I don't think the allocation profile changes.