superpmi: Refactor parallelsuperpmi.cpp to eliminate scattered #ifdef blocks and remove PAL usage

[Copilot]

parallelsuperpmi.cpp was littered with #ifdef TARGET_UNIX guards scattered throughout function bodies, mixing platform logic with control flow and making the code hard to read. Additionally, Ctrl+C signal handling was missing on POSIX (left as TODO-Porting), and the POSIX code path was calling Win32/PAL APIs such as WaitForMultipleObjects and CreateFileA/CloseHandle. This refactor reorganizes all platform-specific code into clean, separated implementations and removes PAL API usage from the POSIX path.

Description

Process spawning

• Split StartProcess into two complete, standalone implementations — one for Win32 (using CreateProcess) and one for POSIX (using fork/execv) — replacing the original single function body that was threaded with interleaved #ifndef TARGET_UNIX / #else / #endif guards.
• Added ParseCommandLineToArgv helper for the POSIX path to tokenize the command-line string into an argv array for execv.
• Added SpmiProcessHandle typedef (pid_t on POSIX, HANDLE on Windows) to unify the process handle array type used throughout the file.

POSIX Ctrl+C handling (previously unimplemented)

• Added PosixCtrlHandler signal handler using signal(SIGINT, ...), implementing the previously skipped TODO-Porting for graceful Ctrl+C handling on Unix.
• Made closeRequested volatile for correct signal-handler access.

Elimination of scattered #ifdef TARGET_UNIX guards in doParallelSuperPMI

• Extracted all remaining inline #ifdef TARGET_UNIX guards from doParallelSuperPMI into named helper functions: RegisterCtrlHandler, GetTempFolderPath, GetDefaultWorkerCount, GetCurrentExePath, OpenWorkerOutputFiles, CloseWorkerOutputFiles, WaitForWorkerProcesses, GetWorkerExitCode, FreeWorkerExitCodes.
• Consolidated all helper implementations into one #ifdef TARGET_UNIX block: all POSIX implementations appear first, then all Windows implementations — no #ifdef appears inside any function body.
• doParallelSuperPMI itself now contains zero preprocessor guards.

PAL/Win32 API removal from POSIX path

• Replaced WaitForMultipleObjects with a waitpid loop in the POSIX WaitForWorkerProcesses implementation, removing one of the last usages of WaitForMultipleObjects with multiple objects on non-Windows.
• Worker output files on POSIX now use open()/close() with O_CLOEXEC instead of CreateFileA/CloseHandle.

#ifdef TARGET_UNIX

static bool RegisterCtrlHandler() { signal(SIGINT, PosixCtrlHandler); return true; }
static bool GetTempFolderPath(char* tempPath) { /* getenv("TMPDIR") */ }
// ... all other POSIX impls ...

#else // !TARGET_UNIX

static bool RegisterCtrlHandler() { /* SetConsoleCtrlHandler */ }
static bool GetTempFolderPath(char* tempPath) { /* GetTempPath */ }
// ... all other Windows impls ...

#endif // TARGET_UNIX

Note

This PR description was generated with AI assistance.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[jkoritzinsky]

This PR removes one of the 2 last usages of WaitForMultipleObjectsEx with multiple objects on non-Windows.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[jkoritzinsky]

@copilot please update the description of the PR based on the initial prompt, not just the last bit of work.

[Copilot]

@copilot please update the description of the PR based on the initial prompt, not just the last bit of work.

Updated the PR description to cover all three commits: splitting StartProcess into separate Win32/POSIX implementations, extracting the inline #ifdef guards into named helpers, and consolidating all helper implementations into one big #ifdef TARGET_UNIX block.

[jkoritzinsky]

@copilot please rewrite the PR title and description based on the whole set of changes in this PR.

[Copilot]

@copilot please rewrite the PR title and description based on the whole set of changes in this PR.

Updated the PR description to cover all the changes across the three commits: splitting StartProcess into separate Win32/POSIX implementations, implementing POSIX Ctrl+C handling (the old TODO-Porting), extracting all inline #ifdef guards into named helpers, consolidating them into one #ifdef TARGET_UNIX block, and removing Win32/PAL APIs (WaitForMultipleObjects, CreateFileA/CloseHandle) from the POSIX path.

Note: the PR title can't be updated through the tooling available to me — a suggested replacement is: superpmi: Refactor parallelsuperpmi.cpp to eliminate scattered #ifdef blocks and remove PAL usage.

[Copilot]

Pull request overview

Refactors parallelsuperpmi.cpp to separate platform-specific behavior (process creation, Ctrl+C handling, temp paths, worker IO, and waiting) into clean Unix vs Windows helper implementations, removing scattered preprocessor guards from doParallelSuperPMI and avoiding PAL/Win32 APIs on the POSIX path.

Changes:

• Split worker process spawning into standalone Win32 (CreateProcess) and POSIX (fork/execv) implementations.
• Implement POSIX Ctrl+C handling and consolidate remaining platform-dependent logic behind helper functions.
• Replace POSIX worker waiting/output file handling with waitpid and open/close (O_CLOEXEC) respectively.

[Copilot]

On POSIX this file now uses errno, strerror, and fork error paths, but the TARGET_UNIX include block doesn’t include <errno.h>. On some toolchains this will fail to compile (errno undeclared). Add <errno.h> (and keep <string.h> via the PCH) under the #ifdef TARGET_UNIX includes.

[Copilot]

closeRequested is written from a POSIX signal handler. Using volatile bool is not async-signal-safe and can be undefined behavior; the portable type for signal handlers is volatile sig_atomic_t (or another async-signal-safe mechanism). Consider changing the flag type accordingly and keep the handler’s work limited to a single store.

[Copilot]

Windows StartProcess leaks the thread handle returned in PROCESS_INFORMATION (pi.hThread) because it’s never closed. This can accumulate when spawning many workers. Close pi.hThread after successful CreateProcess (and ensure pi.hProcess remains returned).

Suggested change

	*hProcess = pi.hProcess;
	*hProcess = pi.hProcess;
	CloseHandle(pi.hThread);

[Copilot]

ParseCommandLineToArgv splits only on spaces/tabs and ignores quoting/escaping. Many arguments in this tool (paths like -details <path>, -reproName <path>, etc.) can legitimately contain spaces, which will break execv on POSIX. Consider building an argv vector directly (without round-tripping through a command-line string) or implement proper shell/Windows-style quoting rules consistently with how the command line is constructed.

Suggested change

	// Count tokens
	int count = 0;
	char* p = commandLine;
	bool inArg = false;
	while (*p)
	{
	if (*p == ' ' || *p == '\t')
	{
	inArg = false;
	}
	else if (!inArg)
	{
	inArg = true;
	count++;
	}
	p++;
	}
	char** argv = new char*[count + 1];
	*argc = count;
	int idx = 0;
	p = commandLine;
	while (*p)
	{
	while (*p == ' ' || *p == '\t')
	p++;
	if (*p == '\0')
	break;
	char* start = p;
	while (*p && *p != ' ' && *p != '\t')
	p++;
	size_t len = (size_t)(p - start);
	argv[idx] = new char[len + 1];
	memcpy(argv[idx], start, len);
	argv[idx][len] = '\0';
	idx++;
	}
	argv[idx] = nullptr;
	// Implement simple shell-like parsing:
	// - Space and tab separate arguments, unless inside single or double quotes.
	// - Single and double quotes are not included in the resulting arguments.
	// - Backslash escapes the next character, allowing spaces, quotes, and
	// backslashes to be included literally.
	std::vector<std::string> args;
	std::string current;
	bool inSingleQuote = false;
	bool inDoubleQuote = false;
	for (char* p = commandLine; *p != '\0'; ++p)
	{
	char c = *p;
	if (c == '\\')
	{
	// Escape the next character if present.
	++p;
	if (*p == '\0')
	{
	// Trailing backslash: treat as a literal backslash.
	current.push_back('\\');
	break;
	}
	current.push_back(*p);
	continue;
	}
	if (!inSingleQuote && c == '"')
	{
	inDoubleQuote = !inDoubleQuote;
	continue;
	}
	if (!inDoubleQuote && c == '\'')
	{
	inSingleQuote = !inSingleQuote;
	continue;
	}
	if (!inSingleQuote && !inDoubleQuote && (c == ' ' || c == '\t'))
	{
	if (!current.empty())
	{
	args.push_back(current);
	current.clear();
	}
	continue;
	}
	current.push_back(c);
	}
	if (!current.empty())
	{
	args.push_back(current);
	}
	*argc = static_cast<int>(args.size());
	char** argv = new char*[*argc + 1];
	for (int i = 0; i < *argc; i++)
	{
	const std::string& arg = args[static_cast<size_t>(i)];
	size_t len = arg.size();
	argv[i] = new char[len + 1];
	memcpy(argv[i], arg.c_str(), len);
	argv[i][len] = '\0';
	}
	argv[*argc] = nullptr;

[Copilot]

PosixCtrlHandler doesn’t use signum, which is likely to trigger -Wunused-parameter warnings (often treated as errors). Either mark it unused (e.g., cast to void) or annotate appropriately.

Suggested change

	{
	{
	(void)signum;

[Copilot]

POSIX OpenWorkerOutputFiles: if opening stderr fails after stdout succeeded, the already-open stdout fd is leaked. Close wd.hStdOutput before returning false (and consider resetting to -1).

[Copilot]

Windows OpenWorkerOutputFiles: if opening stderr fails after stdout succeeds, wd.hStdOutput is leaked. Close the stdout handle before returning false (and consider resetting handles to INVALID_HANDLE_VALUE).

[Copilot]

POSIX WaitForWorkerProcesses treats any waitpid failure as permanent. If waitpid is interrupted (errno==EINTR), this will mark the worker as failed and, worse, may skip reaping the child (leaving it running/zombie). Loop/retry on EINTR, and only treat other errno values as failure.

[Copilot]

Windows WaitForWorkerProcesses ignores the return value of WaitForMultipleObjects. If it fails (e.g., invalid handle), the code will proceed as though workers completed and then attempt to read exit codes. Check for WAIT_FAILED/WAIT_TIMEOUT and log/return an error state.

Suggested change

	WaitForMultipleObjects(count, handles, true, INFINITE);
	DWORD waitResult = WaitForMultipleObjects(count, handles, TRUE, INFINITE);
	if (waitResult == WAIT_FAILED)
	{
	LogError("WaitForMultipleObjects failed. GetLastError()=%u", GetLastError());
	return nullptr;
	}
	if (waitResult == WAIT_TIMEOUT)
	{
	LogError("WaitForMultipleObjects timed out unexpectedly.");
	return nullptr;
	}
	if ((waitResult < WAIT_OBJECT_0) || (waitResult >= WAIT_OBJECT_0 + static_cast<DWORD>(count)))
	{
	LogError("WaitForMultipleObjects returned unexpected result: %u", waitResult);
	return nullptr;
	}

[Copilot]

POSIX RegisterCtrlHandler doesn’t validate whether signal(SIGINT, ...) succeeded (SIG_ERR). Consider checking the return value and failing the registration if it can’t be installed.

Suggested change

	signal(SIGINT, PosixCtrlHandler);
	if (signal(SIGINT, PosixCtrlHandler) == SIG_ERR)
	{
	LogError("Failed to register SIGINT handler.");
	return false;
	}