Fix hang-up issue

[huangqinjin]

A single wine invocation may spawn several long-living Win32 processes automatically. These processes will inherit at least the stderr handle of current wine command and keep it open. Waiting for the wine command's stderr will hang, e.g. CMake waits for the compilation commands: https://gitlab.kitware.com/cmake/cmake/-/blob/0991023c30ed5b83bcb1446b5bcc9c1eae028835/Source/kwsys/ProcessUNIX.c#L1076

Fixes #47

[huangqinjin]

I am still struggling at argument escape for bat file, especially when the compilation commands include -DINC="<stdio.h>".

[huangqinjin]

Pass parameters through environment variables as a workaround.

[mstorsjo]

Thanks for your work on this!

While the commit message explains the overall situation quite well, it doesn't explain much about how this patch accomplishes to fix it. The patch does use a few bash features that I'm not familiar with, so I'd appreciate if you could explain it in more detail; the most important parts of what it does and why in the commit message, and the rest here in the PR.

In particular, things I'd like to understand:

• Earlier versions of the patch just passed arguments as normal to the bat script, and then just executed %*, but you mentioned that there were quoting issues. So with bat scripts, it's not safe to do general argument forwarding by invoking %*, contrary to e.g. "$@" in posix shell and bash?
• Earlier versions just ran wine script.bat args, while this now does wine64 'C:\Windows\System32\cmd.exe' /C $(dirname $0)/wine-msvc.bat; why is that?
• In Handle /notify_update in mt wrapper #63, you did manual remapping of the return code, if %errorlevel%==1090650113 (exit 187), but I don't see that here. But I got the impression that this PR now supersedes the other one; how is that error code remapping handled in this case here? Is there something that truncates/remaps this return code implicitly somewhere here?
• I'm not familiar at all with the syntax used for remapping the stdout/stderr streams; exec {fd1}, {fd1}>&-, and accessing it via /proc/$$/fd/$fd1 - can you explain more in detail what bash syntax features this is and how they behave?
• And the last question kinda follows into the main point; you've described the original issue - child processes are spawned that keep a reference open to the stderr stream. How is that issue fixed here? My rough guess would be that there's a child process within the bash script, which reads from a separate file descriptor into the current stdout/stderr. When the script terminates, the child processes (mspdbsrv or similar) would be left with a closed stdout/stderr file descriptor - is that correctly understood?
• The fact that this relies on /proc/... means that it's rather Linux specific; this means that msvc-wine no longer would work on other OSes such as macOS. Is there a way around this somewho?

[huangqinjin]

@mstorsjo Thanks for the questions, actually I spent quite a lot of time at each. I would like to explain one by one.

Earlier versions just ran wine script.bat args, while this now does wine64 'C:\Windows\System32\cmd.exe' /C $(dirname $0)/wine-msvc.bat; why is that?

On my local machine (openSUSE Tumbleweed) with wine 8.6 (almost latest), I can run wine script.bat args, but on Github Actions which is Ubuntu 22.04 with wine 6.x, it is failed and need to explicitly run with wine cmd /c script.bat args. Further mote, there is cmd.exe in $(dirname $0), so I have to qualify cmd with the full path.

you did manual remapping of the return code, if %errorlevel%==1090650113 (exit 187)

It's needed. I would like to rework #63 once this PR get accepted.

exec {fd1}, {fd1}>&-, /proc/$$/fd/$fd1

• For exec I quote from https://www.gnu.org/software/bash/manual/html_node/Bourne-Shell-Builtins.html

  If no command is specified, redirections may be used to affect the current shell environment.

• Regarding the redirection syntax: https://www.gnu.org/software/bash/manual/html_node/Redirections.html

  Each redirection that may be preceded by a file descriptor number may instead be preceded by a word of the form {varname}. In this case, for each redirection operator except >&- and <&-, the shell will allocate a file descriptor greater than 10 and assign it to {varname}. If >&- or <&- is preceded by {varname}, the value of varname defines the file descriptor to close.

• We can access FDs as normal files through /dev/fd/<FD>, it is just a symlink to /proc/$$/fd/<FD>, where $$ is current shell pid.

How is that issue fixed here?

Before this PR, we launch wine64 exe 2> >(sed >&2), then stderr would connect as:

stderr of {wineserver, mspdbsrv, exe} => (stdin of sed,  stdout of sed) => stderr of wine-msvc => (cmake)

If {wineserver, mspdbsrv} do not exit, so do sed and cmake . With this PR, we launch wine64 exe &>/dev/null, the connection chain no longer exist and {wineserver, mspdbsrv} only keep reference to /dev/null. We also need to close {fd1} and {fd2} for wine64 exe, otherwise {wineserver, mspdbsrv} keep references to them.

I use a bat file to redirect stdout and stderr of exe into files (actually pipes), then stderr would connect as:

stderr of exe => /proc/$$/fd/$fd2 => (stdin of sed,  stdout of sed) => stderr of wine-msvc => (cmake)

So once exe finishes, cmake receives EOF and then continue processes.

relies on /proc/... means that it's rather Linux specific

I think using fifo instead of /proc/$$/fd/ also work, but I didn't try.

What I tried successfully is to use tmp files to completely capture stdout and stderr of exe first, and then cat the files to stdout and stderr of wine-msvc respectively after exe finishes.

It would be good to setup CI for macOS if we claim support it. I don't have a macOS machine, so cannot test it locally.

[huangqinjin]

Regarding %* in bat, it is equivalent to "$@" in bash, but it becomes magic when the arguments come from different sources. I have done below tests.

test.bat:

@echo off
echo inside
echo %*
%*

test.c:

#include <stdio.h>

int main(int argc, char* argv[])
{
    for (int i = 1; i < argc; ++i)
        printf("argv[%d] = [%s]\n", i, argv[i]);
}

• CreateProcessW(NULL, LR"CMD(cmd /c test.bat test.exe "<a>" "\"a\"")CMD", ...) ✔️

inside
test.exe "\"a\"" "<a>"
argv[1] = ["a"]
argv[2] = [<a>]

• In wineconsole and Windows Command Prompt, cmd /c test.bat test.exe "<a>" "\"a\"" ✔️

inside
test.exe "\"a\"" "<a>"
argv[1] = ["a"]
argv[2] = [<a>]

• In Linux bash and pwsh, wine64 cmd /c test.bat test.exe '"a"' '<a>' ✖️

Invalid name.

• In Linux bash and pwsh, wine64 cmd /c test.bat test.exe '"a"' '^<a^>' ✖️

inside
Invalid name.

Invalid name.

• In Linux bash and pwsh, wine64 cmd /c test.bat test.exe '"a"' '^^^<a^^^>' ✔️

inside
test.exe \"a\" <a>
argv[1] = ["a"]
argv[2] = [<a>]

• In Windows PowerShell and pwsh, cmd /c test.bat test.exe '"a"' '^^^<a^^^>' ✖️

inside
test.exe "a" <a>
argv[1] = [a]
argv[2] = [<a>]

• In Windows PowerShell and pwsh, cmd /c test.bat test.exe '\"a\"' '^^^<a^^^>' ✔️

inside
test.exe \"a\" <a>
argv[1] = ["a"]
argv[2] = [<a>]

[huangqinjin]

I added a commit to use fifo if procfs is not available. Tested on a macOS VM, unfortunately

• mt.exe crashed even without any parameters. -DCMAKE_EXE_LINKER_FLAGS=/MANIFEST:NO -DCMAKE_SHARED_LINKER_FLAGS=/MANIFEST:NO is needed.
• fatal error C1902: Program database manager mismatch; please check your installation #6 . But I cannot find winbind in homebrew. -DCMAKE_MSVC_DEBUG_INFORMATION_FORMAT=Embedded is needed.

[mstorsjo]

Thanks for the explanations! Most of it makes sense, but I'd like to clarify a few more details.

exec {fd1}, {fd1}>&-, /proc/$$/fd/$fd1

• For exec I quote from https://www.gnu.org/software/bash/manual/html_node/Bourne-Shell-Builtins.html

  If no command is specified, redirections may be used to affect the current shell environment.

• Regarding the redirection syntax: https://www.gnu.org/software/bash/manual/html_node/Redirections.html

  Each redirection that may be preceded by a file descriptor number may instead be preceded by a word of the form {varname}. In this case, for each redirection operator except >&- and <&-, the shell will allocate a file descriptor greater than 10 and assign it to {varname}. If >&- or <&- is preceded by {varname}, the value of varname defines the file descriptor to close.

• We can access FDs as normal files through /dev/fd/<FD>, it is just a symlink to /proc/$$/fd/<FD>, where $$ is current shell pid.

How is that issue fixed here?

Before this PR, we launch wine64 exe 2> >(sed >&2), then stderr would connect as:

stderr of {wineserver, mspdbsrv, exe} => (stdin of sed,  stdout of sed) => stderr of wine-msvc => (cmake)

If {wineserver, mspdbsrv} do not exit, so do sed and cmake . With this PR, we launch wine64 exe &>/dev/null, the connection chain no longer exist and {wineserver, mspdbsrv} only keep reference to /dev/null. We also need to close {fd1} and {fd2} for wine64 exe, otherwise {wineserver, mspdbsrv} keep references to them.

I use a bat file to redirect stdout and stderr of exe into files (actually pipes), then stderr would connect as:

stderr of exe => /proc/$$/fd/$fd2 => (stdin of sed,  stdout of sed) => stderr of wine-msvc => (cmake)

So once exe finishes, cmake receives EOF and then continue processes.

Thanks - this is mostly clear, but a few details are still unclear.

What does {fd1}>&- {fd2}>&- do here? The docs you linked say

If >&- or <&- is preceded by {varname}, the value of varname defines the file descriptor to close.

More elaborately, what does that do here, and what would happen if we didn't include it? Does this trigger closing those variable named pipes after the command has completed?

When we launch the executable from within a bat file, I get that wineserver gets launched with /dev/null as stdout/stderr. But doesn't mspdbsrv get launched by e.g. cl.exe - so wouldn't it get the same stderr as cl.exe (which is redirected back to the pipe)? Or is this a case where >&- above closes those pipes, regardless of whether mspdbsrv happened to still have them open?

[huangqinjin]

What does {fd1}>&- {fd2}>&- do here?

fd1 and fd2 are opened by wine-msvc.sh and then inherited by wine64, so we have to close them for wine64 to prevent further inheritance by long-living Win32 processes. This is a form of redirection, it is applied very early after wine64 process forked and before wine64 can do anything.

But doesn't mspdbsrv get launched by e.g. cl.exe - so wouldn't it get the same stderr as cl.exe (which is redirected back to the pipe)?

Firstly, to inherit any Win32 handles on Windows, you have to create the file with inheritance flag on (see lpSecurityAttributes parameter of CreateFile, and launch subprocess with bInheritHandles=TRUE (see CreateProcess). Additionally, for the special stdin/stdout/stderr handles, you need turn on STARTF_USESTDHANDLES of STARTUPINFO as well. I believe mspdbsrv.exe is launched with bInheritHandles=FALSE.

Secondly, on Wine, each Win32 file handle has a underlying Linux fd. Linux fds are inheritable by default. So It is possible that, although stderr handle of cl.exe is not inherited by mspdbsrv.exe by Win32 handle inheritance mechanism, the underlying Linux fd is still inherited through fork/clone. To answer this, I use strace to log all syscall for a run of wine:

strace -o trace -f wine64 cmd /c "@echo XXX > a.txt"

From log file, I see that

recvmsg(7, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="`\0\0\0", iov_len=4}], msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET, cmsg_type       =SCM_RIGHTS, cmsg_data=[9]}], msg_controllen=24, msg_flags=MSG_CMSG_CLOEXEC}, MSG_CMSG_CLOEXEC) = 4
fcntl(9, F_SETFD, FD_CLOEXEC)     = 0
...
write(9, "XXX ", 4)               = 4

The following conclusions are based on the observations from the log, I didn't dig into the source code of wine.

• The file a.txt opened by Win32 API CreateFile is actually opened in wineserver, and then the underlying fd is shared to current process through recvmsg on a Unix Domain Socket (the socket fd is 7 for this case).
• The underlying fd of stdout of Win32 process is not 1 (because of the mechanism above, 9 for this case).
• FD_CLOEXEC is set on the underlying fd. That means those underlying fds are not inherited in subprocess.
• (For a inherited Win32 handle, a subprocess will get a new Linux fd (may be different value from parent) through the same mechanism (recvmsg)).

Both Win32 handle inheritance and Linux fd inheritance are disabled for mspdbsrv.exe, so it doesn't get same stderr of cl.exe.

[mstorsjo]

What does {fd1}>&- {fd2}>&- do here?

fd1 and fd2 are opened by wine-msvc.sh and then inherited by wine64, so we have to close them for wine64 to prevent further inheritance by long-living Win32 processes. This is a form of redirection, it is applied very early after wine64 process forked and before wine64 can do anything.

Hmm, but if we close them soon after wine64 has forked, we wouldn't be getting any output from e.g. cl.exe piped to us via the sed command, to the output? I would kind of expect it to close them after the wine64 process returns - that would make sense to me.

But doesn't mspdbsrv get launched by e.g. cl.exe - so wouldn't it get the same stderr as cl.exe (which is redirected back to the pipe)?

Both Win32 handle inheritance and Linux fd inheritance are disabled for mspdbsrv.exe, so it doesn't get same stderr of cl.exe.

Thanks for the deep investigations! However I don't find that to be consistent with the current situation... Currently before these patches, we get hangs because mspdbsrv ends up inheriting and using the same stdout/stderr as the toplevel invocation of wine-msvc.sh, which gets inherited through cl.exe, right? So if it did get inherited through before, it would get inherited in the same way now, right?

Even if the pipe actually does get inherited, I guess it's still understandable that the root issue does get fixed since the stdout of wine-msvc.sh is decoupled from the inner ones (and the pipe/fifo to the wine processes does get closed when wine64 has returned)?

[huangqinjin]

but if we close them soon after wine64 has forked, we wouldn't be getting any output from e.g. cl.exe piped to us via the sed command, to the output?

{fd1}>&- {fd2}>&- is only applied to wine64 process, but wine-msvc.sh still keep fd1 fd2, so we can write to them (in bat file) through /proc/$$/fd/ ($$ is the pid of wine-msvc.sh), until wine-msvc.sh exits.

So if it did get inherited through before, it would get inherited in the same way now, right?

Yes, but what get inherited? Linux fd 1 and 2 (actually all fds without FD_CLOEXEC). After this patch, they point to /dev/null. The Win32 stderr handle (not fd 2, it is redirected to pipe) of cl is not inherited by mspdbsrv.

[mstorsjo]

but if we close them soon after wine64 has forked, we wouldn't be getting any output from e.g. cl.exe piped to us via the sed command, to the output?

{fd1}>&- {fd2}>&- is only applied to wine64 process, but wine-msvc.sh still keep fd1 fd2, so we can write to them (in bat file) through /proc/$$/fd/ ($$ is the pid of wine-msvc.sh), until wine-msvc.sh exits.

Ok, fair enough.

So if it did get inherited through before, it would get inherited in the same way now, right?

Yes, but what get inherited? Linux fd 1 and 2 (actually all fds without FD_CLOEXEC). After this patch, they point to /dev/null. The Win32 stderr handle (not fd 2, it is redirected to pipe) of cl is not inherited by mspdbsrv.

Right, so the Linux level file descriptors do get inherited - kinda contrary to what you said before:

Both Win32 handle inheritance and Linux fd inheritance are disabled for mspdbsrv.exe, so it doesn't get same stderr of cl.exe.

But that does pretty much sum up my questions on the patch - thanks for taking the time to explain!

I think this can be merged then. I'm not a huge fan of all the extra complexity that it brings, but it does seem to be worthwhile, for avoiding hangs if there are background processes spawned.

[huangqinjin]

Right, so the Linux level file descriptors do get inherited - kinda contrary to what you said before:

Both Win32 handle inheritance and Linux fd inheritance are disabled for mspdbsrv.exe, so it doesn't get same stderr of cl.exe.

Sorry for causing confusion. I should write more explicitly:

Both Win32 handle inheritance and handle's underlying Linux fd inheritance are disabled for mspdbsrv.exe.

[mstorsjo]

Right, so the Linux level file descriptors do get inherited - kinda contrary to what you said before:

Both Win32 handle inheritance and Linux fd inheritance are disabled for mspdbsrv.exe, so it doesn't get same stderr of cl.exe.

Sorry for causing confusion. I should write more explicitly:

Both Win32 handle inheritance and handle's underlying Linux fd inheritance are disabled for mspdbsrv.exe.

Ah, right, that explains it. Thanks!

[mstorsjo]

This seems to have caused some amount of difference in how calling cl reacts to having no parameters at all. Previously, when I just invoked cl, I got this:

$ ~/msvc2022-17.6-preview5/bin/x64/cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.36.32530 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

usage: cl [ option... ] filename... [ /link linkoption... ]

Now after this has been merged, I get this:

$ ~/msvc2022-17.6-preview6/bin/x64/cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.36.32530 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line warning D9024 : unrecognized source file type '', object file assumed
Microsoft (R) Incremental Linker Version 14.36.32530.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:.exe 
LINK : warning LNK4001: no object files specified; libraries used
LINK : warning LNK4068: /MACHINE not specified; defaulting to X64
LINK : fatal error LNK1561: entry point must be defined

[huangqinjin]

@mstorsjo Sorry for that! Please see #67.

[mstorsjo]

I ran into another regression caused by this rewrite.

When building ffmpeg with MSVC, some commands that execute a quite long commandline seem to fail.

I tried to set up an example of this in a branch; see https://github.com/mstorsjo/msvc-wine/commits/ffmpeg. https://github.com/mstorsjo/msvc-wine/actions/runs/5007552272/jobs/8974306311 is a github actions that failed on a command with a very long command line, while https://github.com/mstorsjo/msvc-wine/actions/runs/5007649126/jobs/8974529022 is a run with the form of wine-msvc.sh reverted to the old simpler form, which I expect that will succeed.

[huangqinjin]

@mstorsjo Please try #68!