[flang-commits] [flang] [flang] Fix execute_command_line cmdstat is not set when error occurs (PR #93023)

[Daniel Chen via flang-commits]

================
@@ -64,22 +64,40 @@ void CheckAndStoreIntToDescriptor(
 // the CMDSTAT variable is not present, error termination is initiated.
 int TerminationCheck(int status, const Descriptor *cmdstat,
     const Descriptor *cmdmsg, Terminator &terminator) {
-  if (status == -1) {
-    if (!cmdstat) {
-      terminator.Crash("Execution error with system status code: %d", status);
-    } else {
-      StoreIntToDescriptor(cmdstat, EXECL_ERR, terminator);
-      CheckAndCopyCharsToDescriptor(cmdmsg, "Execution error");
-    }
-  }
 #ifdef _WIN32
   // On WIN32 API std::system returns exit status directly
   int exitStatusVal{status};
-  if (exitStatusVal == 1) {
 #else
   int exitStatusVal{WEXITSTATUS(status)};
-  if (exitStatusVal == 127 || exitStatusVal == 126) {
+  if (exitStatusVal == 1) {
+    if (!cmdstat) {
+      terminator.Crash("General Error with exit status code: 1");
+    } else {
+      StoreIntToDescriptor(cmdstat, INVALID_CL_ERR, terminator);
+      CheckAndCopyCharsToDescriptor(cmdmsg,
+          "General Error: a catch-all exit code for a variety of general "
+          "errors.");
+    }
+  } else if (exitStatusVal == 126) {
+    if (!cmdstat) {
+      terminator.Crash("Command cannot execute with exit status code: 126");
+    } else {
+      StoreIntToDescriptor(cmdstat, INVALID_CL_ERR, terminator);
+      CheckAndCopyCharsToDescriptor(cmdmsg,
+          "Command cannot execute: command was found, but it could not be "
+          "executed.");
+    }
+  } else if (exitStatusVal == 127) {
+    if (!cmdstat) {
+      terminator.Crash("Command not found with exit status code: 127");
+    } else {
+      StoreIntToDescriptor(cmdstat, INVALID_CL_ERR, terminator);
+      CheckAndCopyCharsToDescriptor(cmdmsg,
+          "Command not found: command was not found in the system's PATH");
+    }
+  } else
 #endif
+  if (exitStatusVal != 0) {
----------------
DanielCChen wrote:

Thanks for the update!
It seems to me the "old" logic is

```
if (status == -1) { //  Case 1: common for both Linux and Windows
...
}
#ifdef windows
  if (exitStatusVal == 1) { // Case 2 for all errors on windows.
#else
  if (exitStatusVal == 126 or 127 { // Case 3 (126) and Case 4 (127) for Linux.
#end if
...
}
```

I think what is missing is there is no checking for `exitStatusVal == 1` to catch the case in my issue on Linux. 
I am not sure if we need `exitStatusVal != 0` case though. 
Based on your commits, may I suggest something like:
```
enum CMD_STAT {
  ASYNC_NO_SUPPORT_ERR = -2,
  NO_SUPPORT_ERR = -1,
  CMD_EXECUTED = 0,
  FORK_ERR = 1,
  COMMAND_EXECUTION_ERR = 2, // exit code 1
  COMMAND_CANNOT_EXECUTE_ERR = 3, // Linux exit code 126
  COMMAND_NOT_FOUND_ERR = 4, // Linux exit code 127
  SIGNAL_ERR = 5
};
...

  if (status == -1) {
    if (!cmdstat) {
      terminator.Crash("Command line execution is not supported with system status code: %d.", status);
    } else {
      StoreIntToDescriptor(cmdstat, NO_SUPPORT_ERR, terminator);
      CheckAndCopyCharsToDescriptor(cmdmsg, "Command line execution is not supported.");
    }
  }
#ifdef _WIN32
  // On WIN32 API std::system returns exit status directly
  int exitStatusVal{status};
  if (exitStatusVal == 1) {
    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."); // Since Windows doesn't have different return code for different errors, use a generic error message.
    }
  }
#else
  int 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.");
    }
  }
  ```
  
  Please note that I am not sure what case `status == -1` covers. I assumed it is for the case that a processor that doesn't support command line execution, so I changed the `CMD_STAT` of it from `EXECL_ERR` to `NO_SUPPORT_ERR`. Please correct me if I am wrong. BTW, 'NO_SUPPORT_ERR` seems is not used anywhere.

https://github.com/llvm/llvm-project/pull/93023