Skip to content

[backport] Cygwin: lock cygheap during fork #312

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 21, 2025
Merged

[backport] Cygwin: lock cygheap during fork #312

merged 3 commits into from
Sep 21, 2025

Conversation

@lazka
Copy link
Member

@lazka lazka commented Sep 21, 2025 •
edited
Loading

tyan0 and others added 3 commits September 21, 2025 10:31
@tyan0 @lazka
Cygwin: cygheap: Add lock()/unlock() method
95b76b2 
...so that cygheap can be locked/unlocked from outside of mm/cygheap.cc. Reviewed-by: Corinna Vinschen <corinna@vinschen.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit aec6dc7)
@tyan0 @lazka
Cygwin: spawn: Lock cygheap from refresh_cygheap() until child_copy()
881ed94 
...completion in child process because the cygheap should not be changed to avoid mismatch between child_info::cygheap_max and ::cygheap_max. Otherwise, child_copy() might copy cygheap being modified by other process. In addition, to avoid deadlock, move close_all_files() for non- Cygwin processes after unlocking cygheap, since close_all_files() calls cfree(), which attempts to lock cygheap even when it's already locked. Fixes: 977ad54 ("* spawn.cc (spawn_guts): Call refresh_cygheap before creating a new process to ensure that cygheap_max is up-to-date.") Reviewed-by: Corinna Vinschen <corinna@vinschen.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit 9b66723)
@jeremyd2019 @lazka
Cygwin: lock cygheap during fork
a8c3780 
another thread may simultaneously be doing a cmalloc/cfree while the cygheap is being copied to the child. Addresses: https://cygwin.com/pipermail/cygwin/2025-September/258801.html Signed-off-by: Jeremy Drake <cygwin@jdrake.com> (cherry picked from commit 8a5d395)
@lazka lazka force-pushed the cygheap-fork-backport branch from 8d67970 to a8c3780 Compare September 21, 2025 08:32
@lazka lazka requested a review from jeremyd2019 September 21, 2025 08:55
jeremyd2019
jeremyd2019 approved these changes Sep 21, 2025
View reviewed changes
Copy link
Member

@jeremyd2019 jeremyd2019 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and will make rust happier at least. (this won't help the "resource unavailable" fork errors, but will help hangs when building with lots of threads)
@lazka lazka merged commit dea5713 into msys2:msys2-3.6.4 Sep 21, 2025
9 checks passed
@lazka
Copy link
Member Author

lazka commented Sep 21, 2025

So for the other errors retrying might work (I tried 3 times so far for the msys build)?
@jeremyd2019
Copy link
Member

jeremyd2019 commented Sep 21, 2025 •
edited
Loading

No I don't think retrying will help, the base addresses are deterministic. I think you'd need to try different paths (I think that's why it succeeded in MSYS2 CI (/d/_/rust) and not autobuild (/d/S/B)). If you want to try to get it to work in autobuild, you could try changing the paths inside the $srcdir, like renaming the rustc-1.90.0-src directory, or adding build-dir = "B" or other values to the bootstrap.toml (in the [build] section)
@lazka
Copy link
Member Author

lazka commented Sep 21, 2025 •
edited
Loading

Ah, hm, that. I was wondering if we should patch binutils to take a seed from an env var instead (is that binutils??). The python update is also stuck on address conflicts..
@jeremyd2019
Copy link
Member

jeremyd2019 commented Sep 21, 2025 •
edited
Loading

Yeah, binutils - circa https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/emultempl/pep.em;h=60a833947bd524d06241d9d54f5999f69594643d;hb=HEAD#l937 for 64-bit (https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/emultempl/pe.em;h=b522687c1d5b4e4fb1d3d998b6ee5f19232b264c;hb=HEAD#l968 for 32-bit).

The posix_spawn work I was doing helps with rust, because it uses posix_spawn to call the linker. Those changes result in posix_spawn not using a fork, so it doesn't matter that the dll can't load to the same address. I don't know if python uses posix_spawn or fork/exec directly so I don't know if that would help it.
@lazka
Copy link
Member Author

lazka commented Sep 21, 2025 •
edited
Loading

It should in some cases: python/cpython#113118 though cygwin might need
posix_spawn_file_actions_addclosefrom_np for this
@jeremyd2019
Copy link
Member

jeremyd2019 commented Sep 21, 2025

no closefrom in cygwin, so not in posix_spawn either
lazka added a commit to lazka/MSYS2-packages that referenced this pull request Sep 21, 2025
@lazka
msys2-runtime: update patches
c52c6fb 
msys2/msys2-runtime#312
@lazka lazka mentioned this pull request Sep 21, 2025
Merged
lazka added a commit to msys2/MSYS2-packages that referenced this pull request Sep 22, 2025
@lazka
msys2-runtime: update patches (#5658)
e6a004f 
msys2/msys2-runtime#312
@dscho
Copy link
Collaborator

dscho commented Oct 9, 2025

Hrm. It seems that this change causes pretty consistent CI failures over in Cygwin: https://github.com/cygwin/cygwin/actions/workflows/cygwin.yml
@jeremyd2019
Copy link
Member

jeremyd2019 commented Oct 9, 2025

no, it was the switch of windows-latest from 2022 to 2025: cygwin/cygwin@d704d04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@lazka @jeremyd2019 @dscho @tyan0