Posix-based windows implementation #497

patricoferris · 2023-04-27T18:01:28Z

This is a revised version of #490 (as part of #125). As mentioned in the previous PR, for an Eio 1.0 we're looking to have functional support for windows in a rigorous manner. To that end this PR implements a Windows backend using a mostly unchanged Eio_posix backend (directly copied over) with the following modifications and points of note:

Filesystem operations (mkdir, open etc.) are not currently supported.
Subprocess support is not currently supported either.
The "polling" mechanism has been changed to use Unix.select I started looking into adding a Windows backend to iomux then I stumbled across poll and the use of Wepoll. I tried to simply use WSApoll but of course that completely doesn't work for anything but sockets, so to minimise the diff and just get something working I went with Unix.select which I believe should just work for files and sockets a like and is pretty well tested.
I ran into some issues with MDX so I couldn't just copy the tests over from Eio_main tests. I'll look into that, it seemed to be something about a dll and flexlink or something.
It uses this PR for Unix_cstruct unix: add Cstruct_unix.{read,write,writev,send,recv} mirage/ocaml-cstruct#302 -- if this can't be merged, we could probably vendor the stubs. We still need other functions implemented like pread and pwrite which is a bit awkward because of the differences in how the FD current offset is changed compared to Unix systems...

This is an initial draft, I want to go over the Unix.select stuff and of course do more testing. Maybe then a follow-up PR can come in to add FS operations and the other missing pieces.

samwgoldman · 2023-04-27T20:52:02Z

This issue is relevant here. Unix.select works well with sockets on Windows, but the emulation for non-sockets is not 100%.

talex5

Thanks! Are some of the examples able to run on Windows (hello, or even net?).

Deleting everything marked as deprecated would shrink it a bit.

CI is failing to find Eio. Probably just needs opam pin --with-version=dev.

lib_eio_windows/children.mli

lib_eio_windows/dune

lib_eio_windows/eio_windows.ml

lib_eio_windows/eio_windows.mli

lib_eio_windows/fd.ml

lib_eio_windows/fd.mli

lib_eio_windows/low_level.ml

talex5 · 2023-04-28T10:44:13Z

lib_eio_windows/sched.ml

+  let fd_map = Hashtbl.create 10 in
+  let t = { run_q; poll; poll_maxi = (-1); fd_map; eventfd; eventfd_r;
+            active_ops = 0; need_wakeup = Atomic.make false; sleep_q } in
+  let eventfd_ri : int = Obj.magic eventfd_r in


I don't think file descriptors are ints on Windows.

definitely not an integer; https://github.com/ocaml/ocaml/blob/trunk/otherlibs/unix/unixsupport.h#L43

Yep, that's a little embarrassing ^^" Will fix soon

The incorrect use of Obj.magic has been removed in 3ab48ea -- I'm now relying on comparing Unix.file_descr for the set of things to check to read and write.

patricoferris · 2023-04-28T11:22:15Z

Thanks! Are some of the examples able to run on Windows (hello, or even net?).

Yep both work :)) And yes, I was a little too copy-paste happy and can remove all the bits that are not needed shortly.

With regards to CI, I'm trying to get it working over on a different branch to avoid noise here https://github.com/patricoferris/eio/actions/runs/4829634872/jobs/8604900630. Setup-ocaml uses an old version of opam I think so I can't use --with-version :( -- there are just lots of blockers to having OCaml 5 and windows including:

Fixes to ocplib-endian Remove dependency on base-bytes OCamlPro/ocplib-endian#26
The patches are needed for ocamlbuild Windows/Cygwin: hygienic check fails for some packages ocaml/ocamlbuild#316 (so far I've cheekily installed the duniverse version)
Ideally we'd have David's patches upstreamed too https://github.com/dra27/opam-repository/tree/windows-5.0

These are all probably prerequisites before an Eio_windows release?

avsm · 2023-04-29T12:27:04Z

Where's the dependency on ocplib-endian coming from? We removed it from cstruct a while back, and it shouldn't be necessary these days.

patricoferris · 2023-04-29T12:54:00Z

Where's the dependency on ocplib-endian coming from?

Crowbar I think https://ocaml.org/p/crowbar/latest

polytypic · 2023-04-29T15:09:12Z

I ran into some issues with MDX so I couldn't just copy the tests over from Eio_main tests. I'll look into that, it seemed to be something about a dll and flexlink or something.

On Windows OCaml uses flexdll / flexlink for DLLs. So, on Windows, if you want to be able to load a C library dynamically, e.g. to use it from the OCaml toplevel, utop, or mdx, for example, then that library needs to be compiled (with __declspec(dllimport) annotations where needed) and linked (using flexlink) in a special way to support it.

I ran into this problem when trying to run tests with the luv backend. See here for an ugly hack with which I managed to actually be able to build luv to support dynamic loading on Windows.

patricoferris · 2023-04-29T17:15:13Z

Thanks @polytypic, I think that explains some of the errors I was seeing (which I've disabled I think). I'm not sure yet what's causing the new mdx errors in the CI (the stackoverflows with pretty bizarre line numbers).

patricoferris · 2023-04-29T17:31:25Z

Hmm are we going to be bitten here by FD_SETSIZE in select ? https://github.com/ocaml/ocaml/blob/23dab79a4e42856aa33816b9c79c3d4d79959cb9/otherlibs/unix/select_win32.c#L942

The default size FD_SETSIZE (currently 1024) is somewhat smaller than the current kernel limit to the number of open
files. However, in order to accommodate programs which might potentially use a larger number of open files with select, it is possible to increase this size within a program by providing a larger definition of FD_SETSIZE before the inclusion of <sys/types.h>

patricoferris · 2023-04-29T17:44:10Z

@polytypic just for extra clarity this was the kind of error I was seeing with MDX -- it looks very similar to the one you had with Luv only but not quite the same maybe.

+Cannot load required shared library dllcstruct_unix_stubs.
+Reason: C:/OCaml64/home/PatrickFerris/.opam/5.0/lib/stublibs\dllcstruct_unix_st
ubs.dll: flexdll error: cannot relocate caml_failwith RELOC_REL32, target is too
 far: fffffffd958da134  ffffffff958da134.

polytypic · 2023-04-30T09:25:07Z

flexdll error: cannot relocate caml_failwith RELOC_REL32, target is too far: fffffffd958da134 ffffffff958da134.

Yes, I saw those kinds of errors also. The functions that may/need to be dynamically loaded should be marked with __declspec(dllimport).

avsm · 2023-04-30T10:30:33Z

We shouldn't have to manually annotate declspec(dllimport) in user stubs like cstruct these days. The full background is in ocaml/ocaml#1633, but I'm not sure what the current state of those tools are as Cygwin64 itself has been a moving target.

talex5

I left a few minor comments in-line, but basically good to merge once CI is passing. Currently, it says:

Error: Library "eio_windows" in _build/default/lib_eio_windows is hidden

Probably ocaml/dune#5505 again. The solution is to use (* -*- tuareg -*- *) mode in the dune test file and disable the tests when not on Windows (see https://github.com/ocaml-multicore/eio/pull/475/files).

.github/workflows/main.yml

lib_eio/unix/fork/dune

lib_eio/unix/fork_action.c

lib_eio_windows/eio_windows_stubs.c

lib_eio_windows/fs.ml

lib_eio_windows/sched.ml

talex5

I'm not sure yet what's causing the new mdx errors in the CI (the stackoverflows with pretty bizarre line numbers

[mdx] Fatal error: File "ctf.md", lines 33570790-33570791: Stack overflow

Does it work locally on Windows?

Given that ctf.md is using almost no features of Eio (just Eio.Private.Ctf.mint_id), this seems likely to be an MDX bug. If we can reproduce it without the #require "eio", we can report it there.

It's a good sign that the CI is able to build on Windows though, even if the tests fail. Just checking the build works is probably OK for this PR. We know many of the real tests will fail anyway.

lib_eio/unix/fork_action.c

patricoferris · 2023-05-03T10:41:57Z

Does it work locally on Windows?

It's working locally for me (it would be nice to have a second person verify that). I agree it seems to be something going wrong with MDX and Windows, but only appearing in the CI. I'll see if I can make a demo repo with MDX failing like that to report (and maybe fix ^^") upstream -- we really need mdx to work on Windows in the long run !

patricoferris · 2023-05-03T14:09:46Z

I threw a very simple md file into the tests that has zero dependencies and it also fails with the stackoverflow error see https://github.com/patricoferris/eio/actions/runs/4872160326/jobs/8689998313#step:5:22

EDIT: And so does macOS ^^" https://github.com/patricoferris/eio/actions/runs/4872160326/jobs/8689998021

EDIT EDIT: Also, I was probably meant to have done something with CRLF... Whilst investigating MDX issues I wrote a markdown file with this and it just hangs when you try to do things it seems.

patricoferris · 2023-05-03T14:45:27Z

For the CRLF bits see realworldocaml/mdx#294

patricoferris · 2023-05-04T16:46:58Z

Yeah, so to summarise my thoughts, the MDX issues are:

CLRF issues may be causing the stack overflow issues because MDX simply doesn't support reading this and presumably something internally is spinning and eventually overflowing or something like that. It hangs locally for me. This could be fixed by the linked issue.
Using https://github.com/patricoferris/eio/blob/eef2ef5e45c7da8d3900e8b9c203ad62bff0f2d1/.github/workflows/main.yml#LL42C7-L45C42 also changes the problems of stack overflow on windows back to the FlexDLL issues https://github.com/patricoferris/eio/actions/runs/4877253251/jobs/8702055436#step:6:62

So perhaps for this PR we can disable all MDX tests on Windows in the meantime and work to fix the declspec(dllimport) issues? To make up for the disable tests I can add more to the Alcotest tests on the windows backend?

talex5 · 2023-05-04T17:37:48Z

So perhaps for this PR we can disable all MDX tests on Windows in the meantime and work to fix the declspec(dllimport) issues?

Sounds good to me!

Co-authored-by: Thomas Leonard <talex5@gmail.com>

polytypic · 2023-05-05T09:26:32Z

I just got this PR to build and test on my Windows machine. I'll try to take a moment to read through the changes today.

lib_eio_windows/sched.ml

talex5 · 2023-05-07T16:15:22Z

lib_eio/unix/fork_action.c

@@ -67,13 +70,17 @@ CAMLprim value eio_unix_fork_execve(value v_unit) {
 }

 static void action_fchdir(int errors, value v_config) {
+  #ifdef _WIN32
+  uerror("Unsupported operation on windows", Nothing);


We can't allocate OCaml exceptions here (or in the other actions) as we're in the child and anything involving GC isn't going to work.

Suggested change

uerror("Unsupported operation on windows", Nothing);

eio_unix_fork_error(errors, "action_fchdir", "Unsupported operation on windows");

_exit(1);

Ahh good catch thanks! Fixed the others too in cab124d

patricoferris · 2023-05-08T15:17:15Z

I do wonder if (when it is ready) we should merge this into a stable Windows branch instead of the main branch, as it will probably be a pain to cut a release of Eio if this is in there?

talex5 · 2023-05-08T16:26:03Z

I do wonder if (when it is ready) we should merge this into a stable Windows branch instead of the main branch, as it will probably be a pain to cut a release of Eio if this is in there?

We already can't cut a release, because we got rid of the Windows support from libuv and didn't replace it with anything yet.

nojb · 2023-05-23T08:57:34Z

I understand that the POSIX backend for Windows is just a stopgap measure while waiting for a more performant IOCP backend, but just for future reference here's another issue with the emulation in Unix.select on Windows that we ran into some time ago: ocaml/ocaml#7935

patricoferris mentioned this pull request Apr 27, 2023

Initial windows scheduler #490

Closed

talex5 reviewed Apr 28, 2023

View reviewed changes

talex5 approved these changes May 1, 2023

View reviewed changes

talex5 reviewed May 3, 2023

View reviewed changes

lib_eio/unix/fork_action.c Outdated Show resolved Hide resolved

patricoferris added 11 commits May 5, 2023 00:26

Posix-based windows implementation

29d41d4

Remove unused process module

cf6ba07

Remove deprecated FD module

e609295

Update comments

a9e2943

Update CI

4bd46b5

More posix to windows clean up

ec54ccb

Remove incorrect Obj.magic

40d758b

Update comments and stubs

0917e26

Remove Fs from windows

7fe1080

Add tuareg condition to windows test

4610543

Fix bug introduced in fork_actions

106ea8a

patricoferris and others added 3 commits May 5, 2023 00:26

Use ifndef

fa934d0

Co-authored-by: Thomas Leonard <talex5@gmail.com>

Disable MDX tests on Windows

695499e

Add more windows tests and fix network code

f3d7051

patricoferris force-pushed the posix-windows branch from 15a8db2 to f3d7051 Compare May 4, 2023 23:26

Fix rebase mistake in stubs

e5b57fa

polytypic reviewed May 5, 2023

View reviewed changes

lib_eio_windows/sched.ml Show resolved Hide resolved

patricoferris added 2 commits May 5, 2023 12:44

Support DLA on Windows

9a9c02b

Add kcas

393ec68

polytypic approved these changes May 6, 2023

View reviewed changes

patricoferris mentioned this pull request May 7, 2023

Fs implementation for windows #509

Merged

talex5 reviewed May 7, 2023

View reviewed changes

Don't allocate OCaml exceptions in Windows fork actions

cab124d

patricoferris force-pushed the posix-windows branch from b56dfe3 to cab124d Compare May 8, 2023 15:09

talex5 merged commit 439e538 into ocaml-multicore:main May 8, 2023

talex5 mentioned this pull request May 8, 2023

Windows support #125

Open

9 tasks

talex5 added this to the 0.10 milestone May 31, 2023

talex5 mentioned this pull request Jun 2, 2023

[new release] eio_windows, eio_posix, eio_main, eio_linux and eio (0.10) ocaml/opam-repository#23864

Merged

Sudha247 mentioned this pull request Jul 4, 2023

Support Windows realworldocaml/mdx#295

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Posix-based windows implementation #497

Posix-based windows implementation #497

patricoferris commented Apr 27, 2023 •

edited

Loading

samwgoldman commented Apr 27, 2023

talex5 left a comment

talex5 Apr 28, 2023

avsm Apr 29, 2023

patricoferris Apr 29, 2023

patricoferris Apr 29, 2023 •

edited

Loading

patricoferris commented Apr 28, 2023 •

edited

Loading

avsm commented Apr 29, 2023

patricoferris commented Apr 29, 2023

polytypic commented Apr 29, 2023

patricoferris commented Apr 29, 2023

patricoferris commented Apr 29, 2023

patricoferris commented Apr 29, 2023

polytypic commented Apr 30, 2023

avsm commented Apr 30, 2023

talex5 left a comment

talex5 left a comment

patricoferris commented May 3, 2023

patricoferris commented May 3, 2023 •

edited

Loading

patricoferris commented May 3, 2023

patricoferris commented May 4, 2023 •

edited

Loading

talex5 commented May 4, 2023

polytypic commented May 5, 2023

talex5 May 7, 2023

patricoferris May 8, 2023

patricoferris commented May 8, 2023 •

edited

Loading

talex5 commented May 8, 2023

nojb commented May 23, 2023

	uerror("Unsupported operation on windows", Nothing);
	eio_unix_fork_error(errors, "action_fchdir", "Unsupported operation on windows");
	_exit(1);

Posix-based windows implementation #497

Posix-based windows implementation #497

Conversation

patricoferris commented Apr 27, 2023 • edited Loading

samwgoldman commented Apr 27, 2023

talex5 left a comment

Choose a reason for hiding this comment

talex5 Apr 28, 2023

Choose a reason for hiding this comment

avsm Apr 29, 2023

Choose a reason for hiding this comment

patricoferris Apr 29, 2023

Choose a reason for hiding this comment

patricoferris Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

patricoferris commented Apr 28, 2023 • edited Loading

avsm commented Apr 29, 2023

patricoferris commented Apr 29, 2023

polytypic commented Apr 29, 2023

patricoferris commented Apr 29, 2023

patricoferris commented Apr 29, 2023

patricoferris commented Apr 29, 2023

polytypic commented Apr 30, 2023

avsm commented Apr 30, 2023

talex5 left a comment

Choose a reason for hiding this comment

talex5 left a comment

Choose a reason for hiding this comment

patricoferris commented May 3, 2023

patricoferris commented May 3, 2023 • edited Loading

patricoferris commented May 3, 2023

patricoferris commented May 4, 2023 • edited Loading

talex5 commented May 4, 2023

polytypic commented May 5, 2023

talex5 May 7, 2023

Choose a reason for hiding this comment

patricoferris May 8, 2023

Choose a reason for hiding this comment

patricoferris commented May 8, 2023 • edited Loading

talex5 commented May 8, 2023

nojb commented May 23, 2023

patricoferris commented Apr 27, 2023 •

edited

Loading

patricoferris Apr 29, 2023 •

edited

Loading

patricoferris commented Apr 28, 2023 •

edited

Loading

patricoferris commented May 3, 2023 •

edited

Loading

patricoferris commented May 4, 2023 •

edited

Loading

patricoferris commented May 8, 2023 •

edited

Loading