ruby-changes:52821
From: k0kubun <ko1@a...>
Date: Sat, 13 Oct 2018 00:14:58 +0900 (JST)
Subject: [ruby-changes:52821] k0kubun:r65033 (trunk): mjit_worker.c: suppress child process's output properly
k0kubun 2018-10-13 00:14:51 +0900 (Sat, 13 Oct 2018) New Revision: 65033 https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=65033 Log: mjit_worker.c: suppress child process's output properly Prior to this commit, some of parent process's output was unintentionally suppressed. We couldn't suppress only child process's output with spawnvp. Instead of that, this commit uses CreateProcess directly to redirect stdout and stderr only for child process. As it's dealing with HANDLE returned from CreateProcess, now waitpid macro needs to CloseHandle it. win32/win32.c: Introduce rb_w32_start_process which is designed for MJIT worker. Other similar functions can't be used since they are using ALLOCV that may trigger GC, which should be avoided on MJIT worker. Modified files: trunk/mjit_worker.c trunk/win32/win32.c Index: win32/win32.c =================================================================== --- win32/win32.c (revision 65032) +++ win32/win32.c (revision 65033) @@ -1298,6 +1298,47 @@ is_batch(const char *cmd) https://github.com/ruby/ruby/blob/trunk/win32/win32.c#L1298 #define utf8_to_wstr(str, plen) mbstr_to_wstr(CP_UTF8, str, -1, plen) #define wstr_to_utf8(str, plen) wstr_to_mbstr(CP_UTF8, str, -1, plen) +/* License: Ruby's */ +MJIT_FUNC_EXPORTED HANDLE +rb_w32_start_process(const char *abspath, char *const *argv, int out_fd) +{ + /* NOTE: This function is used by MJIT worker, i.e. it must not touch Ruby VM. + This function is not using ALLOCV that may trigger GC. So this can't be + replaced with some families in this file. */ + struct ChildRecord *child; + char *cmd; + size_t len; + WCHAR *wcmd = NULL, *wprog = NULL; + HANDLE outHandle = NULL; + + if (out_fd) { + outHandle = (HANDLE)rb_w32_get_osfhandle(out_fd); + } + + len = join_argv(NULL, argv, FALSE, filecp(), 1); + cmd = alloca(sizeof(char) * len); + join_argv(cmd, argv, FALSE, filecp(), 1); + + if (!(wcmd = mbstr_to_wstr(filecp(), cmd, -1, NULL))) { + errno = E2BIG; + return 0; + } + if (!(wprog = mbstr_to_wstr(filecp(), abspath, -1, NULL))) { + errno = E2BIG; + return 0; + } + + child = CreateChild(wcmd, wprog, NULL, NULL, outHandle, outHandle, 0); + if (child == NULL) { + return 0; + } + child->pid = 0; /* release the slot */ + + free(wcmd); + free(wprog); + return child->hProcess; +} + /* License: Artistic or GPL */ static rb_pid_t w32_spawn(int mode, const char *cmd, const char *prog, UINT cp) @@ -2112,6 +2153,7 @@ rb_w32_wstr_to_mbstr(UINT cp, const WCHA https://github.com/ruby/ruby/blob/trunk/win32/win32.c#L2153 WCHAR * rb_w32_mbstr_to_wstr(UINT cp, const char *str, int clen, long *plen) { + /* This is used by MJIT worker. Do not trigger GC or call Ruby method here. */ WCHAR *ptr; int len = MultiByteToWideChar(cp, 0, str, clen, NULL, 0); if (!(ptr = malloc(sizeof(WCHAR) * len))) return 0; Index: mjit_worker.c =================================================================== --- mjit_worker.c (revision 65032) +++ mjit_worker.c (revision 65033) @@ -110,7 +110,7 @@ https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L110 #define dlclose(handle) (!FreeLibrary(handle)) #define RTLD_NOW -1 -#define waitpid(pid,stat_loc,options) (WaitForSingleObject((HANDLE)(pid), INFINITE), GetExitCodeProcess((HANDLE)(pid), (LPDWORD)(stat_loc)), (pid)) +#define waitpid(pid,stat_loc,options) (WaitForSingleObject((HANDLE)(pid), INFINITE), GetExitCodeProcess((HANDLE)(pid), (LPDWORD)(stat_loc)), CloseHandle((HANDLE)pid), (pid)) #define WIFEXITED(S) ((S) != STILL_ACTIVE) #define WEXITSTATUS(S) (S) #define WIFSIGNALED(S) (0) @@ -572,6 +572,17 @@ static pid_t https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L572 start_process(const char *path, char *const *argv) { pid_t pid; + /* + * Not calling non-async-signal-safe functions between vfork + * and execv for safety + */ + int dev_null = rb_cloexec_open(ruby_null_device, O_WRONLY, 0); + char fbuf[MAXPATHLEN]; + const char *abspath = dln_find_exe_r(path, 0, fbuf, sizeof(fbuf)); + if (!abspath) { + verbose(1, "MJIT: failed to find `%s' in PATH", path); + return -1; + } if (mjit_opts.verbose >= 2) { int i; @@ -583,43 +594,42 @@ start_process(const char *path, char *co https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L594 fprintf(stderr, "\n"); } #ifdef _WIN32 - pid = spawnvp(_P_NOWAIT, path, argv); -#else { - /* - * Not calling non-async-signal-safe functions between vfork - * and execv for safety - */ - char fbuf[MAXPATHLEN]; - const char *abspath = dln_find_exe_r(path, 0, fbuf, sizeof(fbuf)); - int dev_null; + extern HANDLE rb_w32_start_process(const char *abspath, char *const *argv, int out_fd); + int out_fd = 0; + if (mjit_opts.verbose <= 1) { + /* Discard cl.exe's outputs like: + _ruby_mjit_p12u3.c + Creating library C:.../_ruby_mjit_p12u3.lib and object C:.../_ruby_mjit_p12u3.exp */ + out_fd = dev_null; + } - if (!abspath) { - verbose(1, "MJIT: failed to find `%s' in PATH\n", path); + pid = (pid_t)rb_w32_start_process(abspath, argv, out_fd); + if (pid == 0) { + verbose(1, "MJIT: Failed to create process: %s", dlerror()); return -1; } - dev_null = rb_cloexec_open(ruby_null_device, O_WRONLY, 0); - - if ((pid = vfork()) == 0) { - umask(0077); - if (mjit_opts.verbose == 0) { - /* CC can be started in a thread using a file which has been - already removed while MJIT is finishing. Discard the - messages about missing files. */ - dup2(dev_null, STDERR_FILENO); - dup2(dev_null, STDOUT_FILENO); - } - (void)close(dev_null); - pid = execv(abspath, argv); /* Pid will be negative on an error */ - /* Even if we successfully found CC to compile PCH we still can - fail with loading the CC in very rare cases for some reasons. - Stop the forked process in this case. */ - verbose(1, "MJIT: Error in execv: %s\n", abspath); - _exit(1); + } +#else + if ((pid = vfork()) == 0) { + umask(0077); + if (mjit_opts.verbose == 0) { + /* CC can be started in a thread using a file which has been + already removed while MJIT is finishing. Discard the + messages about missing files. */ + dup2(dev_null, STDERR_FILENO); + dup2(dev_null, STDOUT_FILENO); } (void)close(dev_null); + pid = execv(abspath, argv); /* Pid will be negative on an error */ + /* Even if we successfully found CC to compile PCH we still can + fail with loading the CC in very rare cases for some reasons. + Stop the forked process in this case. */ + verbose(1, "MJIT: Error in execv: %s", abspath); + _exit(1); } #endif + (void)close(dev_null); return pid; } COMPILER_WARNING_POP @@ -740,26 +750,7 @@ compile_c_to_so(const char *c_file, cons https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L750 if (args == NULL) return FALSE; - { - /* Discard cl.exe's outputs like: - _ruby_mjit_p12u3.c - Creating library C:.../_ruby_mjit_p12u3.lib and object C:.../_ruby_mjit_p12u3.exp */ - int stdout_fileno, orig_fd, dev_null; - if (mjit_opts.verbose <= 1) { - stdout_fileno = _fileno(stdout); - orig_fd = dup(stdout_fileno); - dev_null = rb_cloexec_open(ruby_null_device, O_WRONLY, 0); - - dup2(dev_null, stdout_fileno); - } - exit_code = exec_process(cc_path, args); - if (mjit_opts.verbose <= 1) { - dup2(orig_fd, stdout_fileno); - - close(orig_fd); - close(dev_null); - } - } + exit_code = exec_process(cc_path, args); free(args); if (exit_code == 0) { -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/