Revert "Revert "[flang] Fix execute_command_line cmdstat is not set when error occurs" (#96365)"

[yiwu0b11]

The fix broke llvm-test-suite, so it was reverted previously. With test fixes added in llvm/llvm-test-suite#137, it should now pass the tests

This reverts commit 4356356.

[llvmbot]

@llvm/pr-subscribers-flang-runtime

Author: Yi Wu (yi-wu-arm)

Changes

The fix broke llvm-test-suite, so it was reverted previously. With test fixes added in llvm/llvm-test-suite#137, it should now pass the tests

This reverts commit 4356356.

---

Full diff: https://github.com/llvm/llvm-project/pull/96774.diff

3 Files Affected:

• (modified) flang/docs/Intrinsics.md (+10-9)
• (modified) flang/runtime/execute.cpp (+96-25)
• (modified) flang/unittests/Runtime/CommandTest.cpp (+65-17)

diff --git a/flang/docs/Intrinsics.md b/flang/docs/Intrinsics.md
index 8853d4d9e1c79..d1f7cd8372e24 100644
--- a/flang/docs/Intrinsics.md
+++ b/flang/docs/Intrinsics.md
@@ -893,16 +893,17 @@ used in constant expressions have currently no folding support at all.
 ##### `CMDSTAT`:
 
 - Synchronous execution:
-  - -2: No error condition occurs, but `WAIT` is present with the value `false`, and the processor does not support asynchronous execution.
-  - -1: The processor does not support command line execution.
+  - -2: `ASYNC_NO_SUPPORT_ERR` - No error condition occurs, but `WAIT` is present with the value `false`, and the processor does not support asynchronous execution.
+  - -1: `NO_SUPPORT_ERR` - The processor does not support command line execution. (system returns -1 with errno `ENOENT`)
+  - 0: `CMD_EXECUTED` - Command executed with no error.
   - \+ (positive value): An error condition occurs.
-    - 1: Fork Error (occurs only on POSIX-compatible systems).
-    - 2: Execution Error (command exits with status -1).
-    - 3: Invalid Command Error (determined by the exit code depending on the system).
-      - On Windows: exit code is 1.
-      - On POSIX-compatible systems: exit code is 127 or 126.
-    - 4: Signal error (either stopped or killed by signal, occurs only on POSIX-compatible systems).
-  - 0: Otherwise.
+    - 1: `FORK_ERR` - Fork Error (occurs only on POSIX-compatible systems).
+    - 2: `EXECL_ERR` - Execution Error (system returns -1 with other errno).
+    - 3: `COMMAND_EXECUTION_ERR` - Invalid Command Error (exit code 1).
+    - 4: `COMMAND_CANNOT_EXECUTE_ERR` - Command Cannot Execute Error (Linux exit code 126).
+    - 5: `COMMAND_NOT_FOUND_ERR` - Command Not Found Error (Linux exit code 127).
+    - 6: `INVALID_CL_ERR` - Invalid Command Line Error (covers all other non-zero exit codes).
+    - 7: `SIGNAL_ERR` - Signal error (either stopped or killed by signal, occurs only on POSIX-compatible systems).
 - Asynchronous execution:
   - 0 will always be assigned.
 
diff --git a/flang/runtime/execute.cpp b/flang/runtime/execute.cpp
index 0f5bc5059e21d..c7f8f386d81f4 100644
--- a/flang/runtime/execute.cpp
+++ b/flang/runtime/execute.cpp
@@ -13,8 +13,10 @@
 #include "tools.h"
 #include "flang/Runtime/descriptor.h"
 #include <cstdlib>
+#include <errno.h>
 #include <future>
 #include <limits>
+
 #ifdef _WIN32
 #include "flang/Common/windows-include.h"
 #else
@@ -32,13 +34,16 @@ namespace Fortran::runtime {
 // and the processor does not support asynchronous execution. Otherwise it is
 // assigned the value 0
 enum CMD_STAT {
-  ASYNC_NO_SUPPORT_ERR = -2,
-  NO_SUPPORT_ERR = -1,
-  CMD_EXECUTED = 0,
-  FORK_ERR = 1,
-  EXECL_ERR = 2,
-  INVALID_CL_ERR = 3,
-  SIGNAL_ERR = 4
+  ASYNC_NO_SUPPORT_ERR = -2, // system returns -1 with ENOENT
+  NO_SUPPORT_ERR = -1, // Linux setsid() returns -1
+  CMD_EXECUTED = 0, // command executed with no error
+  FORK_ERR = 1, // Linux fork() returns < 0
+  EXECL_ERR = 2, // system returns -1 with other errno
+  COMMAND_EXECUTION_ERR = 3, // exit code 1
+  COMMAND_CANNOT_EXECUTE_ERR = 4, // Linux exit code 126
+  COMMAND_NOT_FOUND_ERR = 5, // Linux exit code 127
+  INVALID_CL_ERR = 6, // cover all other non-zero exit code
+  SIGNAL_ERR = 7
 };
 
 // Override CopyCharsToDescriptor in tools.h, pass string directly
@@ -62,24 +67,86 @@ void CheckAndStoreIntToDescriptor(
 
 // If a condition occurs that would assign a nonzero value to CMDSTAT but
 // the CMDSTAT variable is not present, error termination is initiated.
-int TerminationCheck(int status, const Descriptor *cmdstat,
+std::int64_t TerminationCheck(std::int64_t status, const Descriptor *cmdstat,
     const Descriptor *cmdmsg, Terminator &terminator) {
+  // On both Windows and Linux, errno is set when system returns -1.
   if (status == -1) {
-    if (!cmdstat) {
-      terminator.Crash("Execution error with system status code: %d", status);
+    // On Windows, ENOENT means the command interpreter can't be found.
+    // On Linux, system calls execl with filepath "/bin/sh", ENOENT means the
+    // file pathname does not exist.
+    if (errno == ENOENT) {
+      if (!cmdstat) {
+        terminator.Crash("Command line execution is not supported, system "
+                         "returns -1 with errno ENOENT.");
+      } else {
+        StoreIntToDescriptor(cmdstat, NO_SUPPORT_ERR, terminator);
+        CheckAndCopyCharsToDescriptor(cmdmsg,
+            "Command line execution is not supported, system returns -1 with "
+            "errno ENOENT.");
+      }
     } else {
-      StoreIntToDescriptor(cmdstat, EXECL_ERR, terminator);
-      CheckAndCopyCharsToDescriptor(cmdmsg, "Execution error");
+      char err_buffer[30];
+      char msg[]{"Execution error with system status code: -1, errno: "};
+#ifdef _WIN32
+      if (strerror_s(err_buffer, sizeof(err_buffer), errno) != 0)
+#else
+      if (strerror_r(errno, err_buffer, sizeof(err_buffer)) != 0)
+#endif
+        terminator.Crash("errno to char msg failed.");
+      char *newMsg{static_cast<char *>(AllocateMemoryOrCrash(
+          terminator, std::strlen(msg) + std::strlen(err_buffer) + 1))};
+      std::strcat(newMsg, err_buffer);
+
+      if (!cmdstat) {
+        terminator.Crash(newMsg);
+      } else {
+        StoreIntToDescriptor(cmdstat, EXECL_ERR, terminator);
+        CheckAndCopyCharsToDescriptor(cmdmsg, newMsg);
+      }
+      FreeMemory(newMsg);
     }
   }
+
 #ifdef _WIN32
   // On WIN32 API std::system returns exit status directly
-  int exitStatusVal{status};
-  if (exitStatusVal == 1) {
+  std::int64_t exitStatusVal{status};
+  if (exitStatusVal != 0) {
+    if (!cmdstat) {
+      terminator.Crash(
+          "Invalid command quit with exit status code: %d", exitStatusVal);
+    } else {
+      StoreIntToDescriptor(cmdstat, INVALID_CL_ERR, terminator);
+      CheckAndCopyCharsToDescriptor(cmdmsg, "Invalid command line");
+    }
+  }
 #else
-  int exitStatusVal{WEXITSTATUS(status)};
-  if (exitStatusVal == 127 || exitStatusVal == 126) {
-#endif
+  std::int64_t exitStatusVal{WEXITSTATUS(status)};
+  if (exitStatusVal == 1) {
+    if (!cmdstat) {
+      terminator.Crash("Command line execution failed with exit code: 1.");
+    } else {
+      StoreIntToDescriptor(cmdstat, COMMAND_EXECUTION_ERR, terminator);
+      CheckAndCopyCharsToDescriptor(
+          cmdmsg, "Command line execution failed with exit code: 1.");
+    }
+  } else if (exitStatusVal == 126) {
+    if (!cmdstat) {
+      terminator.Crash("Command cannot be executed with exit code: 126.");
+    } else {
+      StoreIntToDescriptor(cmdstat, COMMAND_CANNOT_EXECUTE_ERR, terminator);
+      CheckAndCopyCharsToDescriptor(
+          cmdmsg, "Command cannot be executed with exit code: 126.");
+    }
+  } else if (exitStatusVal == 127) {
+    if (!cmdstat) {
+      terminator.Crash("Command not found with exit code: 127.");
+    } else {
+      StoreIntToDescriptor(cmdstat, COMMAND_NOT_FOUND_ERR, terminator);
+      CheckAndCopyCharsToDescriptor(
+          cmdmsg, "Command not found with exit code: 127.");
+    }
+    // capture all other nonzero exit code
+  } else if (exitStatusVal != 0) {
     if (!cmdstat) {
       terminator.Crash(
           "Invalid command quit with exit status code: %d", exitStatusVal);
@@ -88,23 +155,26 @@ int TerminationCheck(int status, const Descriptor *cmdstat,
       CheckAndCopyCharsToDescriptor(cmdmsg, "Invalid command line");
     }
   }
+#endif
+
 #if defined(WIFSIGNALED) && defined(WTERMSIG)
   if (WIFSIGNALED(status)) {
     if (!cmdstat) {
-      terminator.Crash("killed by signal: %d", WTERMSIG(status));
+      terminator.Crash("Killed by signal: %d", WTERMSIG(status));
     } else {
       StoreIntToDescriptor(cmdstat, SIGNAL_ERR, terminator);
-      CheckAndCopyCharsToDescriptor(cmdmsg, "killed by signal");
+      CheckAndCopyCharsToDescriptor(cmdmsg, "Killed by signal");
     }
   }
 #endif
+
 #if defined(WIFSTOPPED) && defined(WSTOPSIG)
   if (WIFSTOPPED(status)) {
     if (!cmdstat) {
-      terminator.Crash("stopped by signal: %d", WSTOPSIG(status));
+      terminator.Crash("Stopped by signal: %d", WSTOPSIG(status));
     } else {
       StoreIntToDescriptor(cmdstat, SIGNAL_ERR, terminator);
-      CheckAndCopyCharsToDescriptor(cmdmsg, "stopped by signal");
+      CheckAndCopyCharsToDescriptor(cmdmsg, "Stopped by signal");
     }
   }
 #endif
@@ -134,8 +204,9 @@ void RTNAME(ExecuteCommandLine)(const Descriptor &command, bool wait,
 
   if (wait) {
     // either wait is not specified or wait is true: synchronous mode
-    int status{std::system(newCmd)};
-    int exitStatusVal{TerminationCheck(status, cmdstat, cmdmsg, terminator)};
+    std::int64_t status{std::system(newCmd)};
+    std::int64_t exitStatusVal{
+        TerminationCheck(status, cmdstat, cmdmsg, terminator)};
     // If sync, assigned processor-dependent exit status. Otherwise unchanged
     CheckAndStoreIntToDescriptor(exitstat, exitStatusVal, terminator);
   } else {
@@ -173,7 +244,7 @@ void RTNAME(ExecuteCommandLine)(const Descriptor &command, bool wait,
         terminator.Crash(
             "CreateProcess failed with error code: %lu.", GetLastError());
       } else {
-        StoreIntToDescriptor(cmdstat, (uint32_t)GetLastError(), terminator);
+        StoreIntToDescriptor(cmdstat, ASYNC_NO_SUPPORT_ERR, terminator);
         CheckAndCopyCharsToDescriptor(cmdmsg, "CreateProcess failed.");
       }
     }
@@ -201,7 +272,7 @@ void RTNAME(ExecuteCommandLine)(const Descriptor &command, bool wait,
         }
         exit(EXIT_FAILURE);
       }
-      int status{std::system(newCmd)};
+      std::int64_t status{std::system(newCmd)};
       TerminationCheck(status, cmdstat, cmdmsg, terminator);
       exit(status);
     }
diff --git a/flang/unittests/Runtime/CommandTest.cpp b/flang/unittests/Runtime/CommandTest.cpp
index 08daa4ba37f26..20bd7a5b5ff35 100644
--- a/flang/unittests/Runtime/CommandTest.cpp
+++ b/flang/unittests/Runtime/CommandTest.cpp
@@ -311,8 +311,8 @@ TEST_F(ZeroArguments, GetCommand) { CheckCommandValue(commandOnlyArgv, 1); }
 TEST_F(ZeroArguments, ECLValidCommandAndPadSync) {
   OwningPtr<Descriptor> command{CharDescriptor("echo hi")};
   bool wait{true};
-  OwningPtr<Descriptor> exitStat{EmptyIntDescriptor()};
-  OwningPtr<Descriptor> cmdStat{EmptyIntDescriptor()};
+  OwningPtr<Descriptor> exitStat{IntDescriptor(404)};
+  OwningPtr<Descriptor> cmdStat{IntDescriptor(202)};
   OwningPtr<Descriptor> cmdMsg{CharDescriptor("No change")};
 
   RTNAME(ExecuteCommandLine)
@@ -339,40 +339,91 @@ TEST_F(ZeroArguments, ECLValidCommandStatusSetSync) {
   CheckDescriptorEqStr(cmdMsg.get(), "No change");
 }
 
-TEST_F(ZeroArguments, ECLInvalidCommandErrorSync) {
-  OwningPtr<Descriptor> command{CharDescriptor("InvalidCommand")};
+TEST_F(ZeroArguments, ECLGeneralErrorCommandErrorSync) {
+  OwningPtr<Descriptor> command{CharDescriptor("cat GeneralErrorCommand")};
+  bool wait{true};
+  OwningPtr<Descriptor> exitStat{IntDescriptor(404)};
+  OwningPtr<Descriptor> cmdStat{IntDescriptor(202)};
+  OwningPtr<Descriptor> cmdMsg{CharDescriptor("cmd msg buffer XXXXXXXXXXXXXX")};
+
+  RTNAME(ExecuteCommandLine)
+  (*command.get(), wait, exitStat.get(), cmdStat.get(), cmdMsg.get());
+#ifdef _WIN32
+  CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 1);
+  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 6);
+  CheckDescriptorEqStr(cmdMsg.get(), "Invalid command lineXXXXXXXXX");
+#else
+  CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 1);
+  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 3);
+  CheckDescriptorEqStr(cmdMsg.get(), "Command line execution failed");
+#endif
+}
+
+TEST_F(ZeroArguments, ECLNotExecutedCommandErrorSync) {
+  OwningPtr<Descriptor> command{CharDescriptor(
+      "touch NotExecutedCommandFile && chmod -x NotExecutedCommandFile && "
+      "./NotExecutedCommandFile")};
+  bool wait{true};
+  OwningPtr<Descriptor> exitStat{IntDescriptor(404)};
+  OwningPtr<Descriptor> cmdStat{IntDescriptor(202)};
+  OwningPtr<Descriptor> cmdMsg{CharDescriptor("cmd msg buffer XXXXXXXX")};
+
+  RTNAME(ExecuteCommandLine)
+  (*command.get(), wait, exitStat.get(), cmdStat.get(), cmdMsg.get());
+#ifdef _WIN32
+  CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 1);
+  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 6);
+  CheckDescriptorEqStr(cmdMsg.get(), "Invalid command lineXXX");
+#else
+  CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 126);
+  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 4);
+  CheckDescriptorEqStr(cmdMsg.get(), "Command cannot be execu");
+  // removing the file only on Linux (file is not created on Win)
+  OwningPtr<Descriptor> commandClean{
+      CharDescriptor("rm -f NotExecutedCommandFile")};
+  OwningPtr<Descriptor> cmdMsgNoErr{CharDescriptor("No Error")};
+  RTNAME(ExecuteCommandLine)
+  (*commandClean.get(), wait, exitStat.get(), cmdStat.get(), cmdMsgNoErr.get());
+  CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 0);
+  CheckDescriptorEqStr(cmdMsgNoErr.get(), "No Error");
+  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 0);
+#endif
+}
+
+TEST_F(ZeroArguments, ECLNotFoundCommandErrorSync) {
+  OwningPtr<Descriptor> command{CharDescriptor("NotFoundCommand")};
   bool wait{true};
   OwningPtr<Descriptor> exitStat{IntDescriptor(404)};
   OwningPtr<Descriptor> cmdStat{IntDescriptor(202)};
-  OwningPtr<Descriptor> cmdMsg{CharDescriptor("Message ChangedXXXXXXXXX")};
+  OwningPtr<Descriptor> cmdMsg{CharDescriptor("unmodified buffer XXXXXXXXX")};
 
   RTNAME(ExecuteCommandLine)
   (*command.get(), wait, exitStat.get(), cmdStat.get(), cmdMsg.get());
 #ifdef _WIN32
-  CheckDescriptorEqInt(exitStat.get(), 1);
+  CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 1);
+  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 6);
+  CheckDescriptorEqStr(cmdMsg.get(), "Invalid command lineXXXXXXX");
 #else
   CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 127);
+  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 5);
+  CheckDescriptorEqStr(cmdMsg.get(), "Command not found with exit");
 #endif
-  CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 3);
-  CheckDescriptorEqStr(cmdMsg.get(), "Invalid command lineXXXX");
 }
 
 TEST_F(ZeroArguments, ECLInvalidCommandTerminatedSync) {
   OwningPtr<Descriptor> command{CharDescriptor("InvalidCommand")};
   bool wait{true};
-  OwningPtr<Descriptor> exitStat{IntDescriptor(404)};
   OwningPtr<Descriptor> cmdMsg{CharDescriptor("No Change")};
 
 #ifdef _WIN32
   EXPECT_DEATH(RTNAME(ExecuteCommandLine)(
-                   *command.get(), wait, exitStat.get(), nullptr, cmdMsg.get()),
+                   *command.get(), wait, nullptr, nullptr, cmdMsg.get()),
       "Invalid command quit with exit status code: 1");
 #else
   EXPECT_DEATH(RTNAME(ExecuteCommandLine)(
-                   *command.get(), wait, exitStat.get(), nullptr, cmdMsg.get()),
-      "Invalid command quit with exit status code: 127");
+                   *command.get(), wait, nullptr, nullptr, cmdMsg.get()),
+      "Command not found with exit code: 127.");
 #endif
-  CheckDescriptorEqInt(exitStat.get(), 404);
   CheckDescriptorEqStr(cmdMsg.get(), "No Change");
 }
 
@@ -394,13 +445,10 @@ TEST_F(ZeroArguments, ECLValidCommandAndExitStatNoChangeAndCMDStatusSetAsync) {
 TEST_F(ZeroArguments, ECLInvalidCommandParentNotTerminatedAsync) {
   OwningPtr<Descriptor> command{CharDescriptor("InvalidCommand")};
   bool wait{false};
-  OwningPtr<Descriptor> exitStat{IntDescriptor(404)};
   OwningPtr<Descriptor> cmdMsg{CharDescriptor("No change")};
 
   EXPECT_NO_FATAL_FAILURE(RTNAME(ExecuteCommandLine)(
-      *command.get(), wait, exitStat.get(), nullptr, cmdMsg.get()));
-
-  CheckDescriptorEqInt(exitStat.get(), 404);
+      *command.get(), wait, nullptr, nullptr, cmdMsg.get()));
   CheckDescriptorEqStr(cmdMsg.get(), "No change");
 }
 

[DanielCChen]

Thanks for updating the tests.
The code LGTM.
I am not familiar with the RT testing, so I will defer it to other reviewers.

[kiranchandramohan]

LG.