system() returning the wrong process return code under windows

[Benjamin Lindner]

Hello,

using system() with two output arguments currently returns the wrong
process exit code.

 > [a,b]=system("echo foo & exit 2")
a =  127
b = foo

Under windows, pclose() returns the process exit code directly, but the
WIFEXITED and WEXITSTATUS macros currently do something different.

the attached chanegset fixes this and

 > [a,b]=system("echo foo & exit 2")
a =  2
b = foo

now works correctly.

I don't know whether this is also an issue under cygwin, the changeset
currently only checks for mingw platform.

Changeset applies to both 3.2.x branch as the development sources.

benjamin

# HG changeset patch
# User Benjamin Lindner <lindnerb@...>
# Date 1256408153 -7200
# Node ID 9202865ea0264a989fc30fd9a927147e5ea3b35e
# Parent  fed605a33cb5f261f55f9390c1392f7e7fe2dbf8
define WIFEXITED and WEXITSTATUS for windows platform

diff -r fed605a33cb5 -r 9202865ea026 liboctave/ChangeLog
--- a/liboctave/ChangeLog Sat Oct 24 20:15:53 2009 +0200
+++ b/liboctave/ChangeLog Sat Oct 24 20:15:53 2009 +0200
@@ -1,3 +1,8 @@
[phone]  Benjamin Lindner  <lindnerb@...>
+
+ * syswait.h: define WIFEXITED and WEXITSTATUS
+ for windows platform
+
 2009-09-06  Jaroslav Hajek  <highegg@...>
 
  * dColVector.h (operator *(const Matrix&, const ColumnVector)):
diff -r fed605a33cb5 -r 9202865ea026 liboctave/syswait.h
--- a/liboctave/syswait.h Sat Oct 24 20:15:53 2009 +0200
+++ b/liboctave/syswait.h Sat Oct 24 20:15:53 2009 +0200
@@ -41,6 +41,11 @@
 #include <sys/wait.h>
 #endif
 
+#if defined (__MINGW32__)
+#define WIFEXITED(stat_val) ( (stat_val) != -1 )
+#define WEXITSTATUS(stat_val) ((unsigned)(stat_val))
+#endif
+
 #if defined (NeXT)
 #define HAVE_WAITPID 1
 #define WAITPID(a, b, c) \

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

[Marco atzeri-3]

--- Sab 24/10/09, Benjamin Lindner <lindnerben@...> ha scritto:

> Hello,
>
> using system() with two output arguments currently returns
> the wrong process exit code.
>
> > [a,b]=system("echo foo & exit 2")
> a =  127
> b = foo
>
> Under windows, pclose() returns the process exit code
> directly, but the WIFEXITED and WEXITSTATUS macros currently
> do something different.
>
> the attached chanegset fixes this and
>
> > [a,b]=system("echo foo & exit 2")
> a =  2
> b = foo
>
> now works correctly.
>
> I don't know whether this is also an issue under cygwin,
> the changeset currently only checks for mingw platform.

on cygwin and ctave 3.2.3

[a,b]=system("echo foo & exit 2")
a =  2
b = foo

so no need of such changeset

>
> Changeset applies to both 3.2.x branch as the development
> sources.
>
> benjamin
>

Regards
Marco


     

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

[Michael Goffioul-2]

You can probably also include _MSC_VER in your patch,
as they use the same runtime code library.

Michael.


On Sat, Oct 24, 2009 at 7:18 PM, Benjamin Lindner <lindnerben@...> wrote:

> Hello,
>
> using system() with two output arguments currently returns the wrong process
> exit code.
>
>> [a,b]=system("echo foo & exit 2")
> a =  127
> b = foo
>
> Under windows, pclose() returns the process exit code directly, but the
> WIFEXITED and WEXITSTATUS macros currently do something different.
>
> the attached chanegset fixes this and
>
>> [a,b]=system("echo foo & exit 2")
> a =  2
> b = foo
>
> now works correctly.
>
> I don't know whether this is also an issue under cygwin, the changeset
> currently only checks for mingw platform.
>
> Changeset applies to both 3.2.x branch as the development sources.
>
> benjamin
>
> # HG changeset patch
> # User Benjamin Lindner <lindnerb@...>
> # Date 1256408153 -7200
> # Node ID 9202865ea0264a989fc30fd9a927147e5ea3b35e
> # Parent  fed605a33cb5f261f55f9390c1392f7e7fe2dbf8
> define WIFEXITED and WEXITSTATUS for windows platform
>
> diff -r fed605a33cb5 -r 9202865ea026 liboctave/ChangeLog
> --- a/liboctave/ChangeLog       Sat Oct 24 20:15:53 2009 +0200
> +++ b/liboctave/ChangeLog       Sat Oct 24 20:15:53 2009 +0200
> @@ -1,3 +1,8 @@
> [phone]  Benjamin Lindner  <lindnerb@...>
> +
> +       * syswait.h: define WIFEXITED and WEXITSTATUS
> +       for windows platform
> +
>  2009-09-06  Jaroslav Hajek  <highegg@...>
>
>        * dColVector.h (operator *(const Matrix&, const ColumnVector)):
> diff -r fed605a33cb5 -r 9202865ea026 liboctave/syswait.h
> --- a/liboctave/syswait.h       Sat Oct 24 20:15:53 2009 +0200
> +++ b/liboctave/syswait.h       Sat Oct 24 20:15:53 2009 +0200
> @@ -41,6 +41,11 @@
>  #include <sys/wait.h>
>  #endif
>
> +#if defined (__MINGW32__)
> +#define WIFEXITED(stat_val) ( (stat_val) != -1 )
> +#define WEXITSTATUS(stat_val) ((unsigned)(stat_val))
> +#endif
> +
>  #if defined (NeXT)
>  #define HAVE_WAITPID 1
>  #define WAITPID(a, b, c) \
>
> _______________________________________________
> Bug-octave mailing list
> Bug-octave@...
> https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave
>
>

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

[Benjamin Lindner]

Michael Goffioul wrote:
> You can probably also include _MSC_VER in your patch,
> as they use the same runtime code library.
>

thanks for the feedback.
I also included _MSC_VER in the changeset.

benjamin

# HG changeset patch
# User Benjamin Lindner <lindnerb@...>
# Date 1256408153 -7200
# Node ID 9202865ea0264a989fc30fd9a927147e5ea3b35e
# Parent  fed605a33cb5f261f55f9390c1392f7e7fe2dbf8
define WIFEXITED and WEXITSTATUS for windows platform

diff -r fed605a33cb5 -r 9202865ea026 liboctave/ChangeLog
--- a/liboctave/ChangeLog Sat Oct 24 20:15:53 2009 +0200
+++ b/liboctave/ChangeLog Sat Oct 24 20:15:53 2009 +0200
@@ -1,3 +1,8 @@
[phone]  Benjamin Lindner  <lindnerb@...>
+
+ * syswait.h: define WIFEXITED and WEXITSTATUS
+ for windows platform
+
 2009-09-06  Jaroslav Hajek  <highegg@...>
 
  * dColVector.h (operator *(const Matrix&, const ColumnVector)):
diff -r fed605a33cb5 -r 9202865ea026 liboctave/syswait.h
--- a/liboctave/syswait.h Sat Oct 24 20:15:53 2009 +0200
+++ b/liboctave/syswait.h Sat Oct 24 20:15:53 2009 +0200
@@ -41,6 +41,11 @@
 #include <sys/wait.h>
 #endif
 
+#if defined (__MINGW32__) || defined (_MSC_VER)
+#define WIFEXITED(stat_val) ( (stat_val) != -1 )
+#define WEXITSTATUS(stat_val) ((unsigned)(stat_val))
+#endif
+
 #if defined (NeXT)
 #define HAVE_WAITPID 1
 #define WAITPID(a, b, c) \

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

[John W. Eaton-3]

On 26-Oct-2009, Benjamin Lindner wrote:

| Michael Goffioul wrote:
| > You can probably also include _MSC_VER in your patch,
| > as they use the same runtime code library.
| >
|
| thanks for the feedback.
| I also included _MSC_VER in the changeset.
|
| benjamin
|
| ----------------------------------------------------------------------
| # HG changeset patch
| # User Benjamin Lindner <lindnerb@...>
| # Date 1256408153 -7200
| # Node ID 9202865ea0264a989fc30fd9a927147e5ea3b35e
| # Parent  fed605a33cb5f261f55f9390c1392f7e7fe2dbf8
| define WIFEXITED and WEXITSTATUS for windows platform
|
| diff -r fed605a33cb5 -r 9202865ea026 liboctave/ChangeLog
| --- a/liboctave/ChangeLog Sat Oct 24 20:15:53 2009 +0200
| +++ b/liboctave/ChangeLog Sat Oct 24 20:15:53 2009 +0200
| @@ -1,3 +1,8 @@
| [phone]  Benjamin Lindner  <lindnerb@...>
| +
| + * syswait.h: define WIFEXITED and WEXITSTATUS
| + for windows platform
| +
|  2009-09-06  Jaroslav Hajek  <highegg@...>
|  
|   * dColVector.h (operator *(const Matrix&, const ColumnVector)):
| diff -r fed605a33cb5 -r 9202865ea026 liboctave/syswait.h
| --- a/liboctave/syswait.h Sat Oct 24 20:15:53 2009 +0200
| +++ b/liboctave/syswait.h Sat Oct 24 20:15:53 2009 +0200
| @@ -41,6 +41,11 @@
|  #include <sys/wait.h>
|  #endif
|  
| +#if defined (__MINGW32__) || defined (_MSC_VER)
| +#define WIFEXITED(stat_val) ( (stat_val) != -1 )
| +#define WEXITSTATUS(stat_val) ((unsigned)(stat_val))
| +#endif
| +

I hesitate to add more checks for systems/versions instead of
features, so I would prefer it if this check were done with a
configure test instaead.  Then we would know from the configure test
instead of having to enumerate the list of systems, and if the bug is
fixed in the future, Octave will not have to change in order to work
correctly.  Maybe something that defines BROKEN_SYSTEM_EXIT_STATUS or
similar?  Maybe there is already a test for this in the autoconf macro
archive.

Or, is there really no bug in Windows here?  Are we just using
WIFEXITED and WEXITSTATUS incorrectly?

jwe
_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

[Benjamin Lindner]

> I hesitate to add more checks for systems/versions instead of
> features, so I would prefer it if this check were done with a
> configure test instaead.  Then we would know from the configure test
> instead of having to enumerate the list of systems, and if the bug is
> fixed in the future, Octave will not have to change in order to work
> correctly.  Maybe something that defines BROKEN_SYSTEM_EXIT_STATUS or
> similar?  Maybe there is already a test for this in the autoconf macro
> archive.
>
> Or, is there really no bug in Windows here?  Are we just using
> WIFEXITED and WEXITSTATUS incorrectly?

Hmm, as far as I can see, the WIFEXITED and WEXITSTATUS are not
available as a standard on a windows system.
So one has to define them for windows platform, and the current
definition in syswait.h is incorrect for the windows platform.

Now maybe my patch is not correct either, it just worked for me.

On windows, _cwait (the implementation of the waitpid() macro in
syswait.h) stores the return code of the process, without any
transformation. And returns -1 if the process did not successfully exit.
So does pclose.
So the default macro implementation of checking the low-byte as status
and returning only the high as exit code does not work on windows.

( http://msdn.microsoft.com/en-us/library/zb9ehy71%28VS.80%29.aspx )
( http://msdn.microsoft.com/en-us/library/25xdhsd2%28VS.80%29.aspx )

benjamin
_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

[John W. Eaton-3]

On 26-Oct-2009, Benjamin Lindner wrote:

| > I hesitate to add more checks for systems/versions instead of
| > features, so I would prefer it if this check were done with a
| > configure test instaead.  Then we would know from the configure test
| > instead of having to enumerate the list of systems, and if the bug is
| > fixed in the future, Octave will not have to change in order to work
| > correctly.  Maybe something that defines BROKEN_SYSTEM_EXIT_STATUS or
| > similar?  Maybe there is already a test for this in the autoconf macro
| > archive.
| >
| > Or, is there really no bug in Windows here?  Are we just using
| > WIFEXITED and WEXITSTATUS incorrectly?
|
| Hmm, as far as I can see, the WIFEXITED and WEXITSTATUS are not
| available as a standard on a windows system.
| So one has to define them for windows platform, and the current
| definition in syswait.h is incorrect for the windows platform.
|
| Now maybe my patch is not correct either, it just worked for me.
|
| On windows, _cwait (the implementation of the waitpid() macro in
| syswait.h) stores the return code of the process, without any
| transformation. And returns -1 if the process did not successfully exit.
| So does pclose.
| So the default macro implementation of checking the low-byte as status
| and returning only the high as exit code does not work on windows.
|
| ( http://msdn.microsoft.com/en-us/library/zb9ehy71%28VS.80%29.aspx )
| ( http://msdn.microsoft.com/en-us/library/25xdhsd2%28VS.80%29.aspx )

Once we switch to automake and start using gnulib, we can fix this
problem with a gnulib module:

  http://www.gnu.org/software/gnulib/MODULES.html#module=sys_wait

The gnulib sys_wait.h.in file includes these lines:

  #else
  /* Native Windows API.  */

  # include <process.h>

  # define waitpid(pid,statusp,options) _cwait (statusp, pid, WAIT_CHILD)

  /* The following macros apply to an argument x, that is a status of a process,
     as returned by waitpid() or, equivalently, _cwait() or GetExitCodeProcess().
     This value is simply an 'int', not composed of bit fields.  */

  /* When an unhandled fatal signal terminates a process, the exit code is 3.  */
  # define WIFSIGNALED(x) ((x) == 3)
  # define WIFEXITED(x) ((x) != 3)
  # define WIFSTOPPED(x) 0

  /* The signal that terminated a process is not known posthum.  */
  # define WTERMSIG(x) SIGTERM

  # define WEXITSTATUS(x) (x)

  /* There are no core dumps.  */
  # define WCOREDUMP(x) 0

  #endif

This is one of the big reasons I want to start using gnulib in Octave.
There is a lot of portability experience expressed in the gnulib
sources, and I don't think it makes a lot of sense for us to continue
reinventing and rediscovering all these things ourselves, and then
only having that experience reflected in the Octave sources.  So I
expect we will have some immediate benefits if we start using
gnulib.  If there are problems with it, then we can help to improve
gnulib and everyone benefits, not just Octave users.

jwe
_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave