[flang] Fix execute_command_line cmdstat is not set when error occurs

[yiwu0b11]

Fixes: #92929

[DanielCChen]

Nit:
With the patch, the code in PR #92929 now issues

> a.out
 No error!
cat: no: No such file or directory
 exitstat= 1
 cmdstat= 3
 Invalid command line

Both exitstat and cmdstat are correct now, but the error message Invalid command line seems not quite accurate.
For instance, if I changed the command to a random fakecommand, it will give the same message.

[yiwu0b11]

but the error message Invalid command line seems not quite accurate.

Correct, but I'm not sure if its a good idea to use popen to capture the output (and only store it to cmdstat when error occurs)
It would be better if there is a detailed cmdmsg to match the actual meaning of the error. but in case of an error, the error message would shows up in the console, maybe adding "Invalid command line: consider checking for exitstat and console printout"

[llvmbot]

@llvm/pr-subscribers-flang-runtime

Author: Yi Wu (yi-wu-arm)

Changes

Fixes: #92929

---

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

3 Files Affected:

• (modified) flang/docs/Intrinsics.md (-2)
• (modified) flang/runtime/execute.cpp (+17-15)
• (modified) flang/unittests/Runtime/CommandTest.cpp (+4-2)

diff --git a/flang/docs/Intrinsics.md b/flang/docs/Intrinsics.md
index 41129b10083b1..7fa718626217b 100644
--- a/flang/docs/Intrinsics.md
+++ b/flang/docs/Intrinsics.md
@@ -899,8 +899,6 @@ used in constant expressions have currently no folding support at all.
     - 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.
 - Asynchronous execution:
diff --git a/flang/runtime/execute.cpp b/flang/runtime/execute.cpp
index 0f5bc5059e21d..c937a688a3bb1 100644
--- a/flang/runtime/execute.cpp
+++ b/flang/runtime/execute.cpp
@@ -64,47 +64,49 @@ 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) {
 #endif
+  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");
+      CheckAndCopyCharsToDescriptor(cmdmsg,
+          "Invalid command line: check for exitstat and console printout");
+    }
+  }
+  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");
     }
   }
+
 #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
diff --git a/flang/unittests/Runtime/CommandTest.cpp b/flang/unittests/Runtime/CommandTest.cpp
index 08daa4ba37f26..591911d1df2c7 100644
--- a/flang/unittests/Runtime/CommandTest.cpp
+++ b/flang/unittests/Runtime/CommandTest.cpp
@@ -344,7 +344,8 @@ TEST_F(ZeroArguments, ECLInvalidCommandErrorSync) {
   bool wait{true};
   OwningPtr<Descriptor> exitStat{IntDescriptor(404)};
   OwningPtr<Descriptor> cmdStat{IntDescriptor(202)};
-  OwningPtr<Descriptor> cmdMsg{CharDescriptor("Message ChangedXXXXXXXXX")};
+  OwningPtr<Descriptor> cmdMsg{CharDescriptor(
+      "cmdmsg will not modify the remaining buffer XXXXXXXXXXXXXXXXXXXXX")};
 
   RTNAME(ExecuteCommandLine)
   (*command.get(), wait, exitStat.get(), cmdStat.get(), cmdMsg.get());
@@ -354,7 +355,8 @@ TEST_F(ZeroArguments, ECLInvalidCommandErrorSync) {
   CheckDescriptorEqInt<std::int64_t>(exitStat.get(), 127);
 #endif
   CheckDescriptorEqInt<std::int64_t>(cmdStat.get(), 3);
-  CheckDescriptorEqStr(cmdMsg.get(), "Invalid command lineXXXX");
+  CheckDescriptorEqStr(cmdMsg.get(),
+      "Invalid command line: check for exitstat and console printoutXXXX");
 }
 
 TEST_F(ZeroArguments, ECLInvalidCommandTerminatedSync) {

[DanielCChen]

Correct, but I'm not sure if its a good idea to use popen to capture the output (and only store it to cmdstat when error occurs)
It would be better if there is a detailed cmdmsg to match the actual meaning of the error. but in case of an error, the error message would shows up in the console, maybe adding "Invalid command line: consider checking for exitstat and console printout"

My intention was to separate the error messages of a failed command execution from the command doesn't exist at all. It is a minor issue though.

[yiwu0b11]

separate the error messages

Done, but only on Linux. cd nowhere and fakecommand would all have a exitstat code of 1 on Windows so I can't seperate them. I thought about using errno, but it turns out that errno is set to 0 for the above two commands.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[yiwu0b11]

Win CI build fail: fatal error C1060: compiler is out of heap space (build and pass all tests on my local build)
Linux CI build and pass all tests

[yiwu0b11]

Did a rebase on main
Hi @DanielCChen do you mind to re-review the code? I've added cmdmsg for common exit code: 127, 126, 1.

Unfortunately, this doesn't work on Windows, so it's an Linux only feature.

[yiwu0b11]

Sorry for the pin @DanielCChen , do you mind take a look at the changes I made. In short, added cmdstat for 1, 126, 127, added NO_SUPPORT_ERR based on return value and errno.

[DanielCChen]

Thanks for the update! It looks good to me.
The only minor suggestion is to move the code that checks status == =1 prior to the code that checks exitStatusVal because it was the order before, so it would be easier to compare the changes.

[DanielCChen]

LGTM. Thanks for fixing this!

[yiwu0b11]

This has cause test failure on the llvm-test-suite, because we provide different cmdstat and more detailed cmdmsg.
Test would fail on:
execute_command_line_1.f90: (Line31): call execute_command_line ("ls *.doesnotexist", .true., i) should be terminated but the test expected not. (if a non-zero value is assigned to cmdstat but cmdstat is not present, terminate)
execute_command_line_3.f90: we provide different cmdstat and better cmdmsg
No affect on:
https://github.com/llvm/llvm-test-suite/blob/6a38b2969dab9a0c98c6de3c792294f9e118b8de/Fortran/gfortran/regression/execute_command_line_2.f90#L4