Re: sysval and doc fixes

[Gary V. Vaughan]

Eric Blake wrote:
> Hi Gary,

Hi Eric!

>>>> And if system() fails with -1 (such as for E2BIG, when I exceed the 1MB
>>>> ARG_MAX limitation), Solaris still translated that to 127:
>>>> $ perl -e 'print "syscmd(/bin/echo ".("a "x2000000).")sysval"' |\
>>>> /usr/ccs/bin/m4 -B5000000
>>>> 127
> 
> So I guess in the above case, Solaris successfully did a vfork() but then
> failed to exec /bin/sh, explaining the 127.  I don't really know an easy
> way to force a fork() failure on Solaris, to see if I could make sysval
> display 255 or -1.  So maybe it's okay if sysval returns -1 if system()
> did likewise.

ACK.  Whichever provides the simpler implementation. :-)

>>> In theory, this means we can't tell the the difference between a system
>>> call failure and killed by signal 127 (TERMSIG is status&0x7f)... but
>>> this is definitely an improvement, so please commit after taking my comments
>>> below into consideration.
> 
> Except that most systems don't support 127 signals, and by the patch
> below, a signal of 127, if supported, would result in 32512.  But yes, it
> is not possible to tell between a program that exited normally with status
> 127 and a program that was unable to be executed.

Exactly.  I use the definition of "In theory" == "Not really".  On second
thoughts, it is a (theoretical) limitation that might be worth a note in
the user documentation.

>>> Please port forward too :-)
> 
> It's on my (growing) list.

Thanks.

>>> We can simplify the changes to src/builtin.c, and remove some redundancy.
>>> And fix an uninitialised variable if sysval is expanded before syscmd
>>> is used:
>>>
>>>
>>> #ifndef WEXITSTATUS
>>> # define WEXITSTATUS(status) (((status) >> 8) & 0xff)
>>> #endif
>>> #ifndef WTERMSIG
>>> # define WTERMSIG(status) ((status) & 0x7f)
>>> #endif
> 
> Except that POSIX states that WEXITSTATUS is undefined if WIFEXITED is
> false (likewise for WIFSIGNALED/WTERMSIG); we really do need all 4 to be
> portable.
> 
>>> /* Exit code from last "syscmd" command.  */
>>> static int sysval = 0;
> 
> That's redundant.  Uninitialized static variables live in .bss and are
> 0-initialized by the C runtime (it is only the heap and uninitialized
> local variables that have garbage values).  Even with 1.4.4:
> $ echo sysval|m4
> 0

That is implementation defined IMHO.  Better to outright declare the
zero initialiser for future readers of the code, and when m4 is ported
to a wacky embedded environment that doesn't have all the binary sections
we are used to on ELF systems.

> But it does beg the question - should frozen files preserve sysval?  It's
> not worth changing this for 1.4.x, but we should probably add another
> command to frozen file format 2 in m4-2.0 to preserve nonzero sysval over
> freezing.

Good point. Yes, it is part of the state that we purport to snapshot into
a frozen file.

>>> ...
>>>
>>> static void
>>> m4_sysval (struct obstack *obs, int argc, token_data **argv)
>>> {
>>>   /* If previous syscmd call exited, SYSVAL is the exit status;
>>>      if syscmd was killed by a signal, SYSVAL is signal number times 256,
>>>      if syscmd could not execute command, SYSVAL is 127,
>>>      else SYSVAL is 0 */
>>>   shipout_int (obs, sysval == -1
>>>                       ? 0x7f
>>>                       : ((WEXITSTATUS (sysval))|(WTERMSIG (sysval) << 8)));
>>> }
> 
> OK, I can keep the decode logic factored in m4_sysval, rather than
> duplicating in m4_e?syscmd.

Okay.  For future readability, maybe another pair of macros then:

#define M4SYSVAL_EXITBITS(x) \
            (WIFEXITED (x) ? WEXITSTATUS (x) : 0x0)
#define M4SYSVAL_TERMSIGBITS(x) \
            (WIFSIGNALLED (x) ? (WTERMSIG (x) << 8) : 0x0)

...

  shipout_int (obs, sysval == -1
                      ? 0x7f
                      : (M4SYSVAL_EXITBITS (sysval) | M4SYSVAL_TERMSIGBITS 
(sysval)));

Cheers,
        Gary.
-- 
Gary V. Vaughan      ())_.  address@hidden,gnu.org}
Research Scientist   ( '/   http://blog.azazil.net
GNU Hacker           / )=   http://trac.azazil.net/projects/libtool
Technical Author   `(_~)_   http://sources.redhat.com/autobook