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

yi-wu-arm · 2024-05-22T11:46:31Z

DanielCChen · 2024-05-23T18:03:08Z

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.

yi-wu-arm · 2024-05-28T09:35:15Z

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 · 2024-05-28T19:15:40Z

@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 · 2024-05-29T13:54:53Z

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.

flang/unittests/Runtime/CommandTest.cpp

flang/runtime/execute.cpp

yi-wu-arm · 2024-05-30T14:06:58Z

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.

flang/runtime/execute.cpp

github-actions · 2024-05-30T18:34:18Z

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

yi-wu-arm · 2024-05-31T12:42:14Z

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

yi-wu-arm · 2024-06-10T12:51:33Z

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.

Fixes: llvm#92929

This reverts commit 3b285a1024f3115a6af6da1b0d7d637548a0bf1c.

for Windows, all three of them has a exit code of 1 so can't write cmdmsg for them

yi-wu-arm · 2024-06-19T09:07:07Z

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 · 2024-06-20T14:14:06Z

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!

…r occurs" (#96365) Reverts #93023 Reverting due to buildbot failure. https://lab.llvm.org/buildbot/#/builders/41/builds/227 test-suite :: Fortran/gfortran/regression/gfortran-regression-execute-regression__execute_command_line_3_f90

yi-wu-arm · 2024-06-24T14:23:00Z

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

…llvm#93023) Fixes: llvm#92929 Also added cmdstat for common linux return code 1, 126, 127

…r occurs" (llvm#96365) Reverts llvm#93023 Reverting due to buildbot failure. https://lab.llvm.org/buildbot/#/builders/41/builds/227 test-suite :: Fortran/gfortran/regression/gfortran-regression-execute-regression__execute_command_line_3_f90

kiranchandramohan requested a review from DavidTruby May 28, 2024 18:49

yi-wu-arm marked this pull request as ready for review May 28, 2024 19:15

llvmbot added flang:runtime flang Flang issues not falling into any other category labels May 28, 2024

DanielCChen reviewed May 29, 2024

View reviewed changes

flang/unittests/Runtime/CommandTest.cpp Outdated Show resolved Hide resolved

flang/runtime/execute.cpp Outdated Show resolved Hide resolved

yi-wu-arm commented May 30, 2024

View reviewed changes

flang/runtime/execute.cpp Outdated Show resolved Hide resolved

yi-wu-arm requested a review from DanielCChen May 31, 2024 12:40

yi-wu-arm force-pushed the ecl_cmdstat_fix branch from 800e5eb to 24e8e2e Compare June 10, 2024 12:48

yi-wu-arm added 10 commits June 14, 2024 20:04

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

d57fb62

Fixes: llvm#92929

cmdstat will set to 3 when error then set to specific value

ca86946

slightly more detailed cmdmsg

20ec036

Revert "slightly more detailed cmdmsg"

d9b347e

This reverts commit 3b285a1024f3115a6af6da1b0d7d637548a0bf1c.

add cmdmsg to common exit code 1, 126, 127 for Linux

7bd1b8d

for Windows, all three of them has a exit code of 1 so can't write cmdmsg for them

rewrite if-else flow

48a73ae

test fixes Windows

424a4d2

clang format

5344dc5

add cmdstat and cmdmsg for common linux exit code 1 126 127

c6d4ac0

test fixes

1eea4e1

yi-wu-arm force-pushed the ecl_cmdstat_fix branch from aee6729 to 1eea4e1 Compare June 14, 2024 19:04

move status==-1 back to its original place

9fe9f59

DanielCChen approved these changes Jun 20, 2024

View reviewed changes

yi-wu-arm merged commit 4232dd5 into llvm:main Jun 21, 2024
8 checks passed

kiranchandramohan mentioned this pull request Jun 21, 2024

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

Merged

yi-wu-arm mentioned this pull request Jun 26, 2024

update Fortran tests for execute_command_line llvm/llvm-test-suite#137

Merged

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024

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

5bbc952

…llvm#93023) Fixes: llvm#92929 Also added cmdstat for common linux return code 1, 126, 127

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Conversation

Choose a reason for hiding this comment