Conversation
Just to clarify, if one is not using > 1 domains, are these fixes relevant? |
nojb
left a comment
nojb
left a comment
There was a problem hiding this comment.
Thanks for a very clean and easy to read diff. I checked that the PR changes nothing for non-MSVC systems. Furthermore, @shym's script makes it clear that the MASM version of the assembly bits are correct.
I hope to do large-scale testing with LexiFi's codebase in order to provide more feedback (and of OCaml 5 in general), but that will take a bit of time, as first I need to synchronize the codebase with the current trunk. In any case, I don't think that should matter for merging the PR; the existing tests give more than enough confidence to move forward with it.
Technically I would be happy to approve the PR in its current form, but a few decisions (eg regarding winpthreads) need wider discussion, so am not approving just now in order not to preempt that discussion. (And also to give me time to do another pass over the patch in the coming days.)
Thanks for the hard work!
That's correct - single-domain (even with systhreads) will be absolutely fine. As far as we can tell, the whole of the runtime is fine with 17.8, but we didn't manage to rule out whether user C code might suffer from the issues fixed in 17.9 when using our headers. |
|
OK, @dra27 beat me to it 😅 |
MSVC dev here. If you run into issues let us know, we are actively working on improving support for the C compiler and we rely on feedback to help prioritize what gets done first. There are still a few more atomics fixes that will be rolling out over the next few releases (atomic compound assignments with mismatching types are a bit broken). Once that round of fixes are released I plan on deprecating the |
I would suggest doing nothing for now and only address this issue if there is demand for it. |
Just to confirm: the reason we can't put this directly in the tree is because of the license?
This is easy to do and can be done independently. We only need to find someone with the required access rights. Who would that be? |
I don’t think the license makes it impossible, but a submodule is maybe making the separation clearer. My main reasons for prefering a submodule are that:
(The maintenance of
If there’s interest, I’ve drafted a release workflow that attaches archives (with and without winpthreads) to github releases, to make sure that’s easy to do. |
There was a problem hiding this comment.
I gave another pass over the PR. Couldn't spot any issues. I think we could move the winpthread sources to the ocaml organization (but I don't have sufficient rights to do so). The Jenkins changes do not need to happen before the PR is merged.
LGTM
|
@Octachron - I don't think there's a downside to updating the Windows Jenkins workers to use VS2022 (it builds 4.14 too, obviously!). It would have the benefit of being able to run this through precheck before it goes in? |
|
Updating the Windows Jenkins worker sounds fine to me. |
|
Nicolás Ojeda Bär (2024/02/06 05:27 -0800):
> * [ ] shym/winpthreads replaced with ocaml/winpthreads (or another solution)
Just to confirm: the reason we can't put this directly in the tree is because of the license?
> * [ ] Jenkins workers upgraded to VS 2022 17.8+ and precheck passing
This is easy to do and can be done independently. We only need to find
someone with the required access rights. Who would that be?
It's usually @damiendoligez who does this kind of work...
|
Our patches have been merged upstream, so |
That's good to hear, thanks! @Octachron what do you think about creating |
|
The submodule is now pointing to ocaml/winpthreads (thanks, @xavierleroy for setting that all up!). There's a problem with Jenkins, which @damiendoligez is on. I'm anticipating that we may need to tweak the Jenkins |
An instance of ocaml#8901
Slight over-removal in ocaml#11904
At the moment, winpthreads objects are directly linked into the runtime when compiling with MSVC, so we should not mark those symbols with any dll* declaration This avoids another incompatibility between MSVC (which does not accept a `__declspec(dll*)` between `*` and a symbol name, only before the `*`) and GCC (which does), the code being mainly developed for GCC
We avoid including `platform.h` (which is really only necessary here to declare `caml_plat_mutex`) in `io.h` because that would end up pulling in `pthread.h` but we want to hide it on the MSVC port as it is not the native way to handle threads. Co-authored-by: Samuel Hym <samuel.hym@rustyne.lautre.net>
Before, its implementation was an empty function Co-authored-by: Antonin Décimo <antonin@tarides.com>
The translation of amd64nt.asm produces the same code as amd64.S does for mingw-w64. Co-authored-by: Samuel Hym <samuel.hym@rustyne.lautre.net>
Require MSVC 19.38 (bundled in Visual Studio 17.8) or above for its C11 atomics support (even if it's still experimental) Co-authored-by: Antonin Décimo <antonin@tarides.com> Co-authored-by: Samuel Hym <samuel.hym@rustyne.lautre.net>
Shell32 is required for `caml_win32_xdg_defaults` Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Add a `--with-winpthreads-msvc` option to the `configure` script (similar to `--with-flexdll`) to configure where winpthreads sources should be found (default to the `winpthreads` submodule directory, if it is checked out) When the sources are in another directory, copy them into a `winpthreads-sources` directory (similarly to what is done for `flexdll`) since we want to compile only the objects we need and to build them with the same compiler options as the OCaml runtime Build and link the winpthreads objects we need statically in the various flavours of the runtime, instead of linking with the `.lib` Co-authored-by: David Allsopp <david.allsopp@metastack.com>
|
I ran a precheck and it passed (the mingw64 check failed for what looks like unrelated reasons): https://ci.inria.fr/ocaml/job/precheck/963/ |
|
Can you restart that particular job? It failed due to an hopefully transient network error. |
I couldn't figure out how to restart that one job, but I relaunched the full |
It passed! |
The test is skipped on the 32-bit runner because it doesn’t have the latest Visual Studio, but the test runs fine on Github CI so I’m confident there should not be a problem lurking there.
Nothing I thought of, for my part. |
|
The Jenkins Windows workers for 32-bit need upgrading as they actually run 32-bit Windows 10, and Visual Studio 2022 is 64-bit! @damiendoligez is onto this, but that's obviously a much bigger piece of work. I think this is therefore good to go! 🥳 |
Restore the MSVC port of OCaml (cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
(cherry picked from commit 2e2b17d)
6 participants
This PR restores MSVC support in OCaml 5.x, the last remaining platform. The PR replaces @dra27's prototype restoration of the MSVC port in #11835, and is joint work of @dra27, @MisterDA, @shym and @dustanddreams. It completes the work of the following PRs:
CAMLthread_localmacro for TLS variables #12811windows.hin a few places it is required #12832readonly_fileslist in a test #12833camlatomic.hand remove the dependency ofdomain.honplatform.h#12839.oextension in the Makefile #12950We are very grateful to Microsoft for recent fixes to the (experimental) atomics support in Visual Studio following bug reports we were able to make as a result of working on this port. Particular thanks to @msprotz for putting everyone in touch and @Rastaban for the fixes!
There have been various versions of this restoration plan over the last year, so to attempt some clarity:
The issues identified in #11835 are all addressed, with the following notes:
MASM reserved keywords
The first commit (renaming a C primitive in the testsuite) works around a long-known issue in #8901. This should be fixed one day, but the port had this issue in 4.x, so it doesn't seem necessary to block 5.x on it.
winpthreads
winpthreads is a part of the mingw-w64 library which implements pthread.h in terms of Windows native synchronisation primitives (in the same vein as our own systhreads library does in OCaml 4.x). The first port of the Multicore OCaml project to Windows in ocaml-multicore/ocaml-multicore#173 for mingw-w64 used this library as a "temporary" workaround, as it allowed all the existing pthreads code (right up to linking with
-lpthread) to be reused, and allowed efforts to be focused instead on the memory management code, timers and other things which had to be ported. The use of it in 2018 was regarded as temporary for two reasons, principally that we knew that the library was mingw-w64 only, so MSVC support would be difficult.Subsequently, @dra27 happened on sgeto/winpthreads-msvc which demonstrated the principle that the winpthreads library could be relatively easily compiled using MSVC, and not pulling in masses of other dependencies.
@shym and @MisterDA have formalised the process of extracting those sources from the mingw-w64 project directly, and @MisterDA has been heroically contributing patches for MSVC support to the mingw-w64 project.
It is now worth mentioning the second reason for regarding the use of winpthreads as temporary. It has always been @stedolan's intention that OCaml's use of pthreads should be limited - i.e. the runtime eschews "exotic" features of pthreads, mainly for portability concerns. We strongly believe that we should be able to reimplement OCaml's use of pthreads in a more abstract way (exactly as the systhreads library does in 4.x), and therefore implement both the mingw-w64 and MSVC ports without the use of winpthreads. Further, we'd expect to be able to be more performant, simply because our own primitives would not need to be worrying about the full weight of the POSIX spec for pthreads, which of course winpthreads has to.
However, that's not a small piece of work (and there is no prototype or WIP for this!). We therefore propose to use winpthreads for MSVC, but not to expose this fact in any way to users. For the mingw-w64 port, it is perfectly reasonable for users to write C code which depends on pthreads, because the mingw-w64 project provides it. We are not proposing to extend that to the MSVC port. This introduces a requirement in the public part of our headers to ensure that
pthread.his never included. Fortunately, this can be done with only a very minor adjustment toio.hto ensure that thecaml_plat_mutexistypedef'ed without requiringpthread.h. For MSVC, there is no danger in "hard-coding" this, because we control thepthread.hheader (i.e. we're hard-coding our own type definition). Internally, the runtime still features the pthreads functions that the runtime uses, but this situation is identical to that in mingw-w64 port already.There is a licensing concern, as winpthreads is jointly BSD3 and MIT licensed, both of which are less liberal than our LGPL licence (only inasmuch as the reproduction of the copyright is required by these licences). Note that all users of the mingw-w64 port have always been affected by this licence limitation, because winpthreads is a core part of GCC, but the licence change is new for the MSVC port. It is also equivalent to the licensing change imposed by the use of zstd (which is dual BSD-3/GPL-2+).
Given:
we propose including winpthreads as a submodule, which is what is implemented here. The instructions for creating this repository are included in
HACKING.adocand this would want updating to ocaml/winpthreads prior to merging. The repository is set-up as a subtree of the mingw-w64 project simply in order to reduce the cloning time required; this process is mechanical. An additional benefit of having a submodule, is that we can very clearly show in ocaml/winpthreads if we have deviated from the winpthreads project, and similarly synchronise with the project to update our vendored sources.Note that, at the moment, a series of patches under review upstream are required for winpthreads to compile cleanly with MSVC. The master branch on shym/winpthreads tracks commit b4515a69 in the mingw-w64 project, and the additional commits are on branch msvc-v4, see this PR.
The use of the submodule is remarkably similar to flexdll, but with a few slightly different consequences:
git-archive (which is what underpins GitHub's automatic source tarballs) does not have the facility to include submodule contents. There is an open question for discussion as to whether we would take the step of publishing a release tarball which includes them (I'm confident that the licence doesn't matter for the sources tarball - if you don't build the code, then your binaries can't be encumbered by that code's licence), or whether we should publish a separate winpthreads-only tarball, or some hybrid of these. For opam packaging, this makes no difference whatsoever - opam does not mind having to download multiple sources, and we already have to deal with flexdll this way. What does change is that MSVC OCaml would definitely not be compilable from just the GitHub source archive, and we have to decide if that matters.
CI
@dra27 has work in motion to switch our Windows CI completely over to GitHub actions. The workflow added here is entirely compatible with this - the subsequent switch would see the MSVC workflow folded into that. There is immediate benefit here in using GitHub Actions for MSVC testing and, for now, continuing to use AppVeyor for mingw-w64 in that the two run in parallel.
Prior to merging, we will need to upgrade the Jenkins workers to have the latest release of Visual Studio 2022 installed.
MASM / gas
#11835 notes the maintenance nightmare of having amd64.S and amd64nt.asm containing the same Windows implementation. There are two possible paths for merging these files - one is to switch gas to use Intel syntax and so allow amd64.S to be preprocessed into a form which MASM can use; the other is to use ocamlopt itself to generate the runtime object, allowing OCaml to be used as the "assembly language", as in the amd64 backend itself.
We've elected for now to go with neither of those approaches, but we're very keen to ensure that the two files don't unwittingly diverge. The compilation artefact produced for both mingw-w64 and MSVC are essentially identical (i.e. the compiler differs; Windows doesn't). @shym has implemented a CI check which allows to assemble amd64.S using mingw-w64's assembler and then use dumpbin on both that and the MASM compilation of amd64nt.asm and compare them. The dissassembly of these files should be largely identical (modulo some known semantically equivalent differences). @shym is a fan of Awk, but please don't hold that against him - the script is for CI only, so doesn't affect or complicate the build of OCaml further!
General overview of the commits
Rename C stub test to avoid MASM keywordsInstall default.manifest on 32bit stillDrop dllimport on winpthreads symbols with MSVCRemove dependency on platform.h from io.h on the MSVC portImplement cpu_relax on MSVCRestore the MSVC native backendwinpthreadssubmodule (this needs to be updated to point to the definitive location of the repository)WIP Add a winpthreads submodule for the MSVC portwinpthreadsRe-enable MSVCAdd shell32.lib in cclibs for the MSVC portSupport winpthreads for MSVC in configure and MakefileAdd a GitHub CI workflow to test the MSVC portAdd an awk script to generate a canonical version of dumpbin outputsCompare the runtime/amd64* object files in CI to keep them in syncAdd instructions to generate and update winpthreads to HACKING.adocRemove the notice that the MSVC port is disabled in README.win32Blocking tasks