Conversation
qinsoon
left a comment
qinsoon
left a comment
There was a problem hiding this comment.
The implementation looks reasonable to me. The major issue is the implementation mmap_fixed (see details below).
|
@sepcnt Thanks for opening this PR. Just to better understand the context: do you plan to use MMTk on Windows for a specific project or runtime, or is this PR mainly to add general Windows support to mmtk-core? Either way is fine -- I’d just like to understand your use case so we can make better long-term decisions around Windows support. |
While I am planning to build a runtime on the top of mmtk, this PR is mainly focus on making a 1:1 port. |
sepcnt
force-pushed
the
feat-windows-support
branch
from
8226c63 to
b11ef21
Compare
|
The failed test for the Julia binding is unrelated with this PR. |
|
@sepcnt Thank you for your PR. While it is nice to support a popular platform, I think this change should be made after we refactor the mmtk-core code base and isolate OS-specific parts into dedicated modules, as suggested in #1420. This PR makes changes that are scattered over many modules, and often uses the pattern Given that supporting Windows is not a priority of ours, it will be even better if it is possible to maintain Windows support outside our organization, as I mentioned in #1420 (comment). A refactoring is required to make this possible. |
I feel it is a good idea to merge this PR before the refactoring. Supporting for an OS that is significantly different from Unix-like systems could actually help guide the refactoring process for an OS interface for us. |
A porting effort like this can definitely guide our progress of porting to another OS. But given that none of our core members can test this PR locally, and this PR is making a massive amount of changes to the code base using AI-assisted tools, I consider it a risk to merge it into the master branch directly. We may keep this pull request, and do our refactoring with this PR as a reference. |
Most of the changes are gated with sudo dpkg --add-architecture i386
sudo apt-get update && sudo apt-get install -y mingw-w64 wine64 wine32 xvfb
rustup target add x86_64-pc-windows-gnuAnd in [target.x86_64-pc-windows-gnu]
runner = "xvfb-run -a wine"
linker = "x86_64-w64-mingw32-gcc" |
Honestly, that does not seem to be a prerequisite for a PR to be merged. To me, for a PR to be merged, it needs to:
We have code reviews and CI. Those are intended to mitigate the risk of merging to master. You can do code review and list the risky parts or code that may cause issues, and expect them to be addressed properly before merging. As for AI/LLM coding, there is a lot of discussion recently. I don't want to go deep on this. To my own experience, human can write bad code, and AI (at this stage) under human supervision can write good code. So my general attitude is neutral -- I judge a PR by its content, rather than who wrote it (AI or human). |
If it's not yet a prerequisite, it should be. Code reviewing and testing are ways to know if this version works. But if we accept this pull request, not only does it mean that we are accepting this change, but it also means we will have the responsibility to maintain it. It means for every change we make, we need to ensure we don't break Windows. And if some changes accidentally breaks Windows, we need to debug it. Given the result of this PR, i.e.
There are also legal concerns about AI-generated code. AI-trained coding agents may be trained from and/or directly produce snippets that are incompatible with our license (Apache2 + MIT). Particularly, we cannot accept snippets that are GPL-licensed. |
|
@sepcnt We’ve decided to implement #1420 before merging this PR. I’m planning to work on it in January, and once that is in place we can rebase this PR on top of the new interface. You’re very welcome to contribute to #1420 if you’re interested or motivated to help with that work. Again thanks for this PR. |
This PR addresses #1420. This PR is based on top of #1418, and includes all the changes for Windows (for testing purpose). It is likely that Windows support will be removed from this PR, and will be merged separately. This PR does not try to refactor our malloc interface -- I am not sure if malloc should be included in the OS interface or not. This PR consolidates the current multiple mmap functions (such as dzmmap, mmap_fixed, mmap_noreplace, mmap_noreserve, etc), and use `MmapStrategy` to specify the expected mmap behavior. --------- Co-authored-by: sepcnt <30561671+sepcnt@users.noreply.github.com>
| while addr < end { | ||
| let mut mbi: MEMORY_BASIC_INFORMATION = std::mem::zeroed(); | ||
| let q = VirtualQuery( | ||
| addr as *const _, | ||
| &mut mbi, | ||
| std::mem::size_of::<MEMORY_BASIC_INFORMATION>(), | ||
| ); | ||
| if q == 0 { | ||
| return Err(io::Error::last_os_error()); | ||
| } | ||
|
|
||
| let region_base = mbi.BaseAddress as *mut u8; | ||
| let region_size = mbi.RegionSize; | ||
| let region_end = region_base.add(region_size); | ||
|
|
||
| // Calculate the intersection of [addr, end) and [region_base, region_end) | ||
| let _sub_begin = if addr > region_base { | ||
| addr | ||
| } else { | ||
| region_base | ||
| }; | ||
| let _sub_end = if end < region_end { end } else { region_end }; | ||
|
|
||
| match mbi.State { | ||
| MEM_FREE => saw_free = true, | ||
| MEM_RESERVE => saw_reserved = true, | ||
| MEM_COMMIT => saw_committed = true, | ||
| _ => { | ||
| return Err(io::Error::other("Unexpected memory state in mmap_fixed")); | ||
| } | ||
| } | ||
|
|
||
| // Jump to the next region (VirtualQuery always returns "continuous regions with the same attributes") | ||
| addr = region_end; | ||
| } | ||
|
|
||
| // 1. All FREE: make a new mapping in the region | ||
| // 2. All RESERVE/COMMIT: treat as an existing mapping, can just COMMIT or succeed directly | ||
| // 3. MIX of FREE + others: not allowed (semantically similar to MAP_FIXED_NOREPLACE) | ||
| if saw_free && (saw_reserved || saw_committed) { | ||
| return Err(io::Error::from_raw_os_error( | ||
| windows_sys::Win32::Foundation::ERROR_INVALID_ADDRESS as i32, | ||
| )); | ||
| } | ||
|
|
||
| if saw_free && !saw_reserved && !saw_committed { | ||
| // All FREE: make a new mapping in the region | ||
| let mut allocation_type = MEM_RESERVE; | ||
| if commit { | ||
| allocation_type |= MEM_COMMIT; | ||
| } | ||
|
|
||
| let res = VirtualAlloc( | ||
| ptr as *mut _, | ||
| size, | ||
| allocation_type, | ||
| strategy.prot.get_native_flags(), | ||
| ); | ||
| if res.is_null() { | ||
| return Err(io::Error::last_os_error()); | ||
| } | ||
|
|
||
| Ok(start) | ||
| } else { |
There was a problem hiding this comment.
Despite all the VirtualQuery checks we do, the subsequent VirtualAlloc may still fail in multi-threaded programs due to TOCTOU. And it takes time if the given region contains multiple mmap entries. But it is useful for sanity check.
I suggest we guard all the checks with a feature or debug_assertion so that we don't do them in production. (It's even better if we can extract all the sanity check parts to a separate function so that we can reuse it.) In production, we only call VirtualAlloc. And we can port the sanity check to Unix-like systems by parsing /proc/self/maps (or using a third-party crate for parsing it), but that still has the TICTOU problem and can only serve debug purposes.
Ideally, we should reserve the region of memory for metadata and the heap so that we won't need such checks.
| // If decommit fails, we try to release the memory. This might happen if the memory was | ||
| // only reserved. | ||
| let res_release = unsafe { VirtualFree(start.to_mut_ptr(), 0, MEM_RELEASE) }; | ||
| if res_release == 0 { | ||
| Err(std::io::Error::last_os_error()) | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
@wks commented in #1439 (comment):
We shouldn't use MEM_RELEASE.
- MEM_RELEASE is supposed to be paired with VirualAlloc and be used as if they were malloc-free pairs. The doc says "If you specify this value (MEM_RELEASE), dwSize must be 0 (zero), and lpAddress must point to the base address returned by the VirtualAlloc function when the region is reserved. The function fails if either of these conditions is not met." Obviously this is completely different from how we use munmap.
- It will remove the reserved state. We don't do it, just like we don't "un-quarantine" memory in Unix-like systems.
Instead if MEM_DECOMMIT fails, we should return failure.
|
One high-level comment is that line-end handling should be done automatically. If the current CI scripts result in errors related to line ends on Windows, it is likely that the CI scripts are using command line tools ( We either
In either of the two cases, there is no need to manually convert line ends ( |
|
At this point, we try to reuse the existing CI scripts for Windows. In those scripts, we do text parsing on source code files, so the scripts need to handle Windows' CR LF. Imagine if we used a different set of CI scripts for Windows -- those scripts would have to deal with CR LF as well, as they would be running on Windows, and dealing with Windows files. |
I disagree. Scripts can be written in a style that is agnostic of line ending styles. The scripts should see lines. That is, a file consists of multiple lines. Every time it reads a line from a text file, it gets all the text until the LF or CR LF.
From the code strip_cr() {
local s=$1
printf '%s' "${s%$'\r'}"
}we infer that the input The documentation of the On https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax, it mentioned that
The git-bash is provided by https://gitforwindows.org/ But there is no documentation about its Bash port. It is quite a surprise that Git and Git-Bash have different assumptions about line ends with the default configuration. Maybe we can find a way to let Git-Bash use CR LF line ending in the |
We can do something like this: https://github.com/rust-lang/rust/blob/859951e3c7c9d0322c39bad49221937455bdffcd/.gitattributes#L3 |
3 participants
This pull request fix #961, adds initial Windows support to the project, focusing on enabling CI, memory management, thread affinity, and malloc functionality for Windows targets. The changes include updates to CI scripts and workflows to handle Windows-specific cases, platform-specific Rust code for memory and thread management, and a new implementation for malloc using Windows APIs.
Windows platform support
Cargo.tomland implemented platform-specific code for thread affinity and CPU detection insrc/scheduler/affinity.rs, using Windows APIs for processor count and thread affinity. [1] [2] [3] [4]src/util/malloc/library.rs, usingHeapAlloc,HeapFree, and related APIs; updated allocation logic insrc/util/malloc/malloc_ms_util.rsto support Windows. [1] [2] [3] [4] [5] [6]CI and workflow updates
.github/scripts/ci-common.shand.github/workflows/minimal-tests-core.ymlto add Windows jobs, set up bash shell defaults, and skip incompatible setup steps for Windows runners; also excludedmalloc_jemallocon Windows in feature initialization. [1] [2] [3] [4] [5] [6]Memory management abstraction
src/util/memory.rsto abstract memory protection, mapping, and error handling for Windows, including platform-specific flag translation and error codes, and using Windows APIs for memory operations. [1] [2] [3] [4] [5] [6] [7] [8]General codebase cleanup
libc::mprotectfromsrc/policy/copyspace.rs, replacing it with platform-abstracted memory protection functions. [1] [2] [3]These changes lay the foundation for building, testing, and running the project on Windows, with further improvements expected for full platform parity.