Support Microsoft toolchain

[jonahbeckford]

trafficstars

~~This PR is incomplete (not all the tests work yet) but builds with the cl.exe Microsoft compiler in a MSYS2 environment. (Specifically on the Windows distribution I announced at https://discuss.ocaml.org/t/ann-windows-friendly-ocaml-4-12-distribution-diskuv-ocaml-0-1-0/8358). I hope it is far enough along I can get a meaningful review of it.~~

Key changes:

• cl.exe supports complex numbers but only minimally compliant with C99. That means an implementation that is a C struct with no ability to do arithmetic (ex. a + b). The vast majority of this PR is replacing float _Complex, etc. with compiler appropriate typedefs.
• cl.exe generates .obj files not .o files
• A couple places I had to use cygpath to force a Windows path rather than a Unix path. Those are the changes in the Makefiles with MSYS2 ifeq macros.

Outstanding issues:

• [x] I'm a bit puzzled about libffi. I can get ctypes-foreign to build against MSYS2's libffi, but MSYS2's libffi is dynamically linked against msys-2.0.dll (basically equivalent to cygwin1.dll) which is very problematic. I think, but do not know, that the following line in Makefile.rules:

  OCAMLMKLIB_EXTRA_FLAGS=-ldopt "-link -static-libgcc" # see GPR#1535
  

  is referring to https://github.com/ocaml/ocaml/pull/1535 and probably was placed there so that libffi in particular is statically linked when built from Cygwin. If so then I can do the same (statically link libffi so OCaml code is not linked against MSYS2) or just switch entirely to a native Windows build of libffi. I'm leaning towards the native Windows build even though it is a bit more complicated because I am guessing that forcing static linking is going to cause more problems later. Any thoughts?

• [x] I stuck in a ctypes_ldouble_complex_div does not exist on current platform error for the Microsoft toolchain (for some bizarre reason Microsoft does not provide complex division in its C library but does in its C++ library) although it is straight-forward enough to provide a logarithm / subtract / exponentiation implementation inside ctypes. Which approach best fits with ctypes?

• [x] The test in tests/test-ldouble/test_ldouble.ml:

  let test_int_conversions _ =
  begin
    assert_equal max_int (LDouble.to_int
    		    (LDouble.of_int max_int))
      ~printer:string_of_int;
  
    assert_equal min_int (LDouble.to_int
    		    (LDouble.of_int min_int))
      ~printer:string_of_int;
  end
  

  is failing with:

  expected: 4611686018427387903 but got: -4611686018427387904
  

  I'm actually finding debugging this a bit difficult because I don't know (yet) how to debug mixed OCaml and C code. How are you doing this? (There are likely other test failures because I haven't run through the full test suite)

Thanks!

[fdopen]

Ok, I will try to have a closer look at it later this week.

I played around with cl.exe long time ago (-> https://github.com/fdopen/ocaml-ctypes/commits/msvc ).

first issue: Just compile libffi from source with cl.exe. The msys/mingw platform and msvc use different conventions. Don't mix libraries/headers between those platforms, unless you know what you are doing.

Back then, I had to patch the source of libffi and compile both libffi and ctypes with special cflags. Hopefully these problems have been solved by now.

third issue: On mvsc64 long double is identic to double (that's not the case with mingw64). Therefore LDouble.of_int max_int isn't a lossless operation.

[jonahbeckford]

Thanks for the feedback, and triple thanks for maintaining the mingw repository!

Native Windows libffi: I just got the native Windows build of libffi working (they are terrible at documentation, but it builds without patches with the correct magic flags). For now those build instructions are here: https://gitlab.com/diskuv/diskuv-ocaml/-/blob/next/installtime/msys2/compile-native-libffi.sh . I can now test and do a comparison with your earlier Windows branch (which looks comprehensive; thanks!) ... that may take a couple days.

Complex number division: After thinking further, it doesn't make much sense to provide an alternate implementation for MSVC. If someone is doing complex division, they should know how to implement that operation with conjugates+multiplication or log+subtraction+exp. A ctypes implementation would have to be general-purpose (ex. be numerically stable across a wide domain of inputs).

Long double: I totally forgot that long doubles were same width as doubles in MSVC. Since ctypes is exposing the real C implementation, I will just need to adjust the long double test for Windows only.

[jonahbeckford]

Testing and fixing complete:

• All tests pass, and the only test that had to be skipped for MSVC is test-builtins which uses the atomic __sync_or_and_fetch that is unavailable in the MSVC runtime libraries.
• I incorporated many things from your previous implementation. Without it I doubt this would be working; thanks!

I also added Merlin definitions for the tests so I could troubleshoot ctypes better.

There is no urgency to review it (I realize it is big) although this one package is very important to the overall MSVC OCaml ecosystem.

I'll submit this PR as a patch to the next preview release of my Windows distribution, and when this PR is ready I'll backport any changes you may want (or better yet, just use the latest ocaml-ctypes release).

[jonahbeckford]

Thanks @fdopen and @nojb . I've updated the PR with your requested changes.

[jonahbeckford]

Latest PR change is "Regression fixes for Linux and 4.03".

Tested the following on Ubuntu 18.04 with OCaml 4.03.0:

$ ocaml --version
The OCaml toplevel, version 4.03.0

$ opam install ./ctypes.opam ./ctypes-foreign.opam --with-test

$ make XEN=false libffi.config ctypes-base ctypes-stubs ctypes-foreign

$ make run-examples

$ _build/ncurses.native 

$ _build/ncurses-cmd.native 

[yallop]

I've enabled the CI on this PR, to make testing easier. (The macos failures are due to an unrelated opam issue, and can be ignored).

[fdopen]

I've finally tried to compile it. The latest version of libffi seems to support msvc without any hacks.

Did you verify that ctypes_tls_callback is called? It's not covered by the test suite and it didn't work for me when I've tested it manually (but I've used an older version of cl.exe). My old solution is way more verbose but works (see https://github.com/fdopen/ocaml-ctypes/commit/a2b66a1c45f4db2b0f4ec575564e937dfb2f4b3d )

[jonahbeckford]

Did you verify that ctypes_tls_callback is called? It's not covered by the test suite and it didn't work for me when I've tested it manually (but I've used an older version of cl.exe). My old solution is way more verbose but works (see fdopen@a2b66a1 )

I'll add that as-is into the PR. Thanks.

I suppose I can just print something or look at the debugger to verify that ctypes_tls_callback is called.

I'm going to try (*) to do my last round of testing by going through https://github.com/aantron/luv's test cases, which is a heavy user of ctypes and which is well tested. One fix from this round of testing is https://github.com/ocamllabs/ocaml-ctypes/pull/685/commits/55c05cff0fc3a4df5bcf86dfb9247407e72b3d6a

(*) I'm not going to spend too long on that though. Switching compilers from GCC to MSVC for luv may create other issues unrelated to ctypes.

[jonahbeckford]

Status Update:

• (not blocking) Ran into problems unrelated to ctypes with my downstream test candidate: https://github.com/aantron/luv/pull/126 . Haven't heard back.
• Still pending: Verify ctypes_tls_callback (already integrated your change for it). I'm currently delayed on this small task because (unrelated to ctypes) I'm prepping a new Windows DKML release to fix some critical bugs.

I'll release this PR as-is under a highly unstable designation; doing so will make it easier to for downstream package maintainers to validate this PR works for them. After the release I'll come back to this PR and finish the last ctypes_tls_callback task.

[jonahbeckford]

I ran test-threads after removing all of its test cases except test_register_thread:

There is a small but important diff with ctypes_tls_callback in the PR commit titled Fix bug in ctypes_tls_callback segments.

The only part I don't feel good about is there is no TlsFree to go along with TlsAlloc. I believe the lack of TlsFree will cause TLS_OUT_OF_INDEXES and possible segfaults if dllctypes-foreign_stubs.dll is loaded/unloaded repeatedly. That should be rare but I can imagine it happening in a chain like something.exe -> plugin.dll -> dllctypes-foreign_stubs.dll when the plugin is loaded/unloaded. But fixing that issue affects the MinGW code as well, so it doesn't belong in this too-big PR.

The testing and review are complete from my side. I am ready for your review. Thanks for the patience!

[yallop]

It would be good to have this merged. @fdopen, are you able to (re)review now that the known issues are resolved?

[yallop]

@jonahbeckford: would you like to close this PR? I'd like to have it merged, if possible, so that you don't need to maintain a fork, but from what you've posted elsewhere it sounds like it's abandoned.

[jonahbeckford]

@jonahbeckford: would you like to close this PR? I'd like to have it merged, if possible, so that you don't need to maintain a fork, but from what you've posted elsewhere it sounds like it's abandoned.

Well, I'm not going to babysit this PR further, but it seems unwise to close it: the underlying issue still exists, it is a major issue, and it needs someone motivated to merge it with the latest code base. That someone motivated will waste a huge amount of time, and perhaps introduce bugs, if they can't find this PR. Perhaps this can get a label like stalled-waiting-for-merge-and-test?

[yallop]

It'd be helpful to have someone rebase this against the latest build system changes, but I'd be much happier merging it if there were some way for me to verify both that it works now, and that it'll continue to work in the future. Ideally, we'd have GitHub Actions CI set up for the Microsoft toolchain.