fixes #10273 execShellCmd now returns nonzero when child killed with signal + other fixes (#10274)

* s/exitStatus(...)/exitStatusLikeShell(...)/
* fix #10273 execShellCmd now returns nonzero when child exits with signal
* test case for #10249 and explanation for the bug
* fix test failure
* add tests/nim.cfg

.gitignore

@@ -32,7 +32,7 @@ doc/*.html
 doc/*.pdf
 doc/*.idx
 /web/upload
-build/*
+/build/*
 bin/*
 
 # iOS specific wildcards.

lib/pure/os.nim

@@ -1287,6 +1287,17 @@ proc moveFile*(source, dest: string) {.rtl, extern: "nos$1",
         discard tryRemoveFile(dest)
         raise
 
+proc exitStatusLikeShell*(status: cint): cint =
+  ## converts exit code from `c_system` into a shell exit code
+  when defined(posix) and not weirdTarget:
+    if WIFSIGNALED(status):
+      # like the shell!
+      128 + WTERMSIG(status)
+    else:
+      WEXITSTATUS(status)
+  else:
+    status
+
 proc execShellCmd*(command: string): int {.rtl, extern: "nos$1",
   tags: [ExecIOEffect], noNimScript.} =
   ## Executes a `shell command`:idx:.
@@ -1295,14 +1306,8 @@ proc execShellCmd*(command: string): int {.rtl, extern: "nos$1",
   ## line arguments given to program. The proc returns the error code
   ## of the shell when it has finished. The proc does not return until
   ## the process has finished. To execute a program without having a
-  ## shell involved, use the `execProcess` proc of the `osproc`
+  ## shell involved, use `osproc.execProcess`.
-  ## module.
+  result = exitStatusLikeShell(c_system(command))
-  when defined(macosx):
-    result = c_system(command) shr 8
-  elif defined(posix):
-    result = WEXITSTATUS(c_system(command))
-  else:
-    result = c_system(command)
 
 # Templates for filtering directories and files
 when defined(windows) and not weirdTarget:

lib/pure/osproc.nim

@@ -724,13 +724,6 @@ elif not defined(useNimRtl):
   proc isExitStatus(status: cint): bool =
     WIFEXITED(status) or WIFSIGNALED(status)
 
-  proc exitStatus(status: cint): cint =
-    if WIFSIGNALED(status):
-      # like the shell!
-      128 + WTERMSIG(status)
-    else:
-      WEXITSTATUS(status)
-
   proc envToCStringArray(t: StringTableRef): cstringArray =
     result = cast[cstringArray](alloc0((t.len + 1) * sizeof(cstring)))
     var i = 0
@@ -1054,7 +1047,7 @@ elif not defined(useNimRtl):
 
     proc waitForExit(p: Process, timeout: int = -1): int =
       if p.exitFlag:
-        return exitStatus(p.exitStatus)
+        return exitStatusLikeShell(p.exitStatus)
 
       if timeout == -1:
         var status: cint = 1
@@ -1109,7 +1102,7 @@ elif not defined(useNimRtl):
         finally:
           discard posix.close(kqFD)
 
-      result = exitStatus(p.exitStatus)
+      result = exitStatusLikeShell(p.exitStatus)
   else:
     import times
 
@@ -1142,7 +1135,7 @@ elif not defined(useNimRtl):
         s.tv_nsec = b.tv_nsec
 
       if p.exitFlag:
-        return exitStatus(p.exitStatus)
+        return exitStatusLikeShell(p.exitStatus)
 
       if timeout == -1:
         var status: cint = 1
@@ -1220,20 +1213,20 @@ elif not defined(useNimRtl):
             if sigprocmask(SIG_UNBLOCK, nmask, omask) == -1:
               raiseOSError(osLastError())
 
-      result = exitStatus(p.exitStatus)
+      result = exitStatusLikeShell(p.exitStatus)
 
   proc peekExitCode(p: Process): int =
     var status = cint(0)
     result = -1
     if p.exitFlag:
-      return exitStatus(p.exitStatus)
+      return exitStatusLikeShell(p.exitStatus)
 
     var ret = waitpid(p.id, status, WNOHANG)
     if ret > 0:
       if isExitStatus(status):
         p.exitFlag = true
         p.exitStatus = status
-        result = exitStatus(status)
+        result = exitStatusLikeShell(status)
 
   proc createStream(stream: var Stream, handle: var FileHandle,
                     fileMode: FileMode) =
@@ -1265,7 +1258,7 @@ elif not defined(useNimRtl):
   proc execCmd(command: string): int =
     when defined(linux):
       let tmp = csystem(command)
-      result = if tmp == -1: tmp else: exitStatus(tmp)
+      result = if tmp == -1: tmp else: exitStatusLikeShell(tmp)
     else:
       result = csystem(command)
 

testament/lib/stdtest/specialpaths.nim

@@ -0,0 +1,31 @@
+#[
+todo: move findNimStdLibCompileTime, findNimStdLib here
+]#
+
+import os
+
+# Note: all the const paths defined here are known at compile time and valid
+# so long Nim repo isn't relocated after compilation.
+# This means the binaries they produce will embed hardcoded paths, which
+# isn't appropriate for some applications that need to be relocatable.
+
+const sourcePath = currentSourcePath()
+  # robust way to derive other paths here
+  # We don't depend on PATH so this is robust to having multiple nim
+  # binaries
+
+const nimRootDir* = sourcePath.parentDir.parentDir.parentDir.parentDir
+  ## root of Nim repo
+
+const stdlibDir* = nimRootDir / "lib"
+  # todo: make nimeval.findNimStdLibCompileTime use this
+
+const systemPath* = stdlibDir / "system.nim"
+
+const buildDir* = nimRootDir / "build"
+  ## refs #10268: all testament generated files should go here to avoid
+  ## polluting .gitignore
+
+static:
+  # sanity check
+  doAssert fileExists(systemPath)

tests/nim.cfg

@@ -0,0 +1 @@
+--path:"../testament/lib" # so we can `import stdtest/foo` in this dir

tests/stdlib/t10231.nim

@@ -6,6 +6,8 @@ discard """
 
 import os
 
+# consider moving this inside tosproc (taking care that it's for cpp mode)
+
 if paramCount() == 0:
   # main process
   doAssert execShellCmd(getAppFilename().quoteShell & " test") == 1

tests/stdlib/tos.nim

@@ -263,3 +263,5 @@ block splitFile:
   doAssert splitFile("abc/.") == ("abc", ".", "")
   doAssert splitFile("..") == ("", "..", "")
   doAssert splitFile("a/..") == ("a", "..", "")
+
+# execShellCmd is tested in tosproc

tests/stdlib/tosproc.nim

@@ -1,23 +1,96 @@
-discard """
-  output: ""
-"""
 # test the osproc module
 
-import os, osproc
+import stdtest/specialpaths
+import "../.." / compiler/unittest_light
 
-block execProcessTest:
+when defined(case_testfile): # compiled test file for child process
-  let dir = parentDir(currentSourcePath())
+  from posix import exitnow
-  let (outp, err) = execCmdEx("nim c " & quoteShell(dir / "osproctest.nim"))
+  proc c_exit2(code: c_int): void {.importc: "_exit", header: "<unistd.h>".}
-  doAssert err == 0
+  import os
-  let exePath = dir / addFileExt("osproctest", ExeExt)
+  var a = 0
-  let outStr1 = execProcess(exePath, workingDir=dir, args=["foo", "b A r"], options={})
+  proc fun(b = 0) =
-  doAssert outStr1 == dir & "\nfoo\nb A r\n"
+    a.inc
+    if a mod 10000000 == 0: # prevents optimizing it away
+      echo a
+    fun(b+1)
 
-  const testDir = "t e st"
+  proc main() =
-  createDir(testDir)
+    let args = commandLineParams()
-  doAssert dirExists(testDir)
+    echo (msg: "child binary", pid: getCurrentProcessId())
-  let outStr2 = execProcess(exePath, workingDir=testDir, args=["x yz"], options={})
+    let arg = args[0]
-  doAssert outStr2 == absolutePath(testDir) & "\nx yz\n"
+    echo (arg: arg)
+    case arg
+    of "exit_0":
+      if true: quit(0)
+    of "exitnow_139":
+      if true: exitnow(139)
+    of "c_exit2_139":
+      if true: c_exit2(139)
+    of "quit_139":
+      # `exitStatusLikeShell` doesn't distinguish between a process that
+      # exit(139) and a process that gets killed with `SIGSEGV` because
+      # 139 = 11 + 128 = SIGSEGV + 128.
+      # However, as #10249 shows, this leads to bad debugging experience
+      # when a child process dies with SIGSEGV, leaving no trace of why it
+      # failed. The shell (and lldb debugger) solves that by inserting a
+      # helpful msg: `segmentation fault` when it detects a signal killed
+      # the child.
+      # todo: expose an API that will show more diagnostic, returing
+      # (exitCode, signal) instead of just `shellExitCode`.
+      if true: quit(139)
+    of "exit_recursion": # stack overflow by infinite recursion
+      fun()
+      echo a
+    of "exit_array": # bad array access
+      echo args[1]
+  main()
 
-  removeDir(testDir)
+else:
-  removeFile(exePath)
+
+  import os, osproc, strutils, posix
+
+  block execShellCmdTest:
+    ## first, compile child program
+    const nim = getCurrentCompilerExe()
+    const sourcePath = currentSourcePath()
+    let output = buildDir / "D20190111T024543".addFileExt(ExeExt)
+    let cmd = "$# c -o:$# -d:release -d:case_testfile $#" % [nim, output,
+        sourcePath]
+    # we're testing `execShellCmd` so don't rely on it to compile test file
+    # note: this should be exported in posix.nim
+    proc c_system(cmd: cstring): cint {.importc: "system",
+      header: "<stdlib.h>".}
+    assertEquals c_system(cmd), 0
+
+    ## use it
+    template runTest(arg: string, expected: int) =
+      echo (arg2: arg, expected2: expected)
+      assertEquals execShellCmd(output & " " & arg), expected
+
+    runTest("exit_0", 0)
+    runTest("exitnow_139", 139)
+    runTest("c_exit2_139", 139)
+    runTest("quit_139", 139)
+    runTest("exit_array", 1)
+    when defined(posix): # on windows, -1073741571
+      runTest("exit_recursion", SIGSEGV.int + 128) # bug #10273: was returning 0
+      assertEquals exitStatusLikeShell(SIGSEGV), SIGSEGV + 128.cint
+
+  block execProcessTest:
+    let dir = parentDir(currentSourcePath())
+    let (outp, err) = execCmdEx("nim c " & quoteShell(dir / "osproctest.nim"))
+    doAssert err == 0
+    let exePath = dir / addFileExt("osproctest", ExeExt)
+    let outStr1 = execProcess(exePath, workingDir = dir, args = ["foo",
+        "b A r"], options = {})
+    doAssert outStr1 == dir & "\nfoo\nb A r\n"
+
+    const testDir = "t e st"
+    createDir(testDir)
+    doAssert dirExists(testDir)
+    let outStr2 = execProcess(exePath, workingDir = testDir, args = ["x yz"],
+        options = {})
+    doAssert outStr2 == absolutePath(testDir) & "\nx yz\n"
+
+    removeDir(testDir)
+    removeFile(exePath)