[PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`

[PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`

[Kylie McClain]

From: Kylie McClain <somasis@exherbo.org>

nl, while specified in POSIX, is rather obscure and isn't provided by small
coreutils implementations such as `busybox`. This while loop works just as
well for our purposes.
---
 src/mkbuiltins | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/mkbuiltins b/src/mkbuiltins
index b4d6f4e..a47bce8 100644
--- a/src/mkbuiltins
+++ b/src/mkbuiltins
@@ -101,7 +101,8 @@ cat <<\!
  */
 
 !
-sed 's/	-[a-z]*//' $temp2 | nl -ba -v0 |
+sed 's/	-[a-z]*//' $temp2 | while read line;do \
+	i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |
 	LC_ALL= LC_COLLATE=C sort -u -k 3,3 |
 	tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ |
 	awk '{	printf "#define %s (builtincmd + %d)\n", $3, $1}'
-- 
2.9.0

[PATCH 2/2] histedit: Remove non-glibc fallback code

[Kylie McClain]

From: Kylie McClain <somasis@exherbo.org>

This fallback code seems to go back the import of the repository back in 2005,
it fails on musl libc, and there aren't any comments explaining why this
difference is needed. Regardless, any compatibility ifdefs should probably be
checking macros defined on systems that need a different code path, rather than
just having fallback code for non-glibc.
---
 src/histedit.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/histedit.c b/src/histedit.c
index 94465d7..4498cf4 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -214,11 +214,7 @@ histcmd(int argc, char **argv)
 	if (argc == 1)
 		sh_error("missing history argument");
 
-#ifdef __GLIBC__
 	optind = 0;
-#else
-	optreset = 1; optind = 1; /* initialize getopt */
-#endif
 	while (not_fcnumber(argv[optind]) &&
 	      (ch = getopt(argc, argv, ":e:lnrs")) != -1)
 		switch ((char)ch) {
-- 
2.9.0

Re: [PATCH 2/2] histedit: Remove non-glibc fallback code

[Jilles Tjoelker]

On Thu, Aug 04, 2016 at 01:54:11AM -0400, Kylie McClain wrote:
> From: Kylie McClain <somasis@exherbo.org>

> This fallback code seems to go back the import of the repository back
> in 2005, it fails on musl libc, and there aren't any comments
> explaining why this difference is needed. Regardless, any
> compatibility ifdefs should probably be checking macros defined on
> systems that need a different code path, rather than just having
> fallback code for non-glibc.

Setting optreset is required on FreeBSD and NetBSD which do not
recognize setting optind to 0. POSIX specifies neither of these methods
to reset getopt().

However, removing the dependency on this feature is simple. The shell
already features a function nextopt() that can be used to parse options
and is automatically set up for builtins.

From 23da600dcff616662a93f307420d9142598e2cae Mon Sep 17 00:00:00 2001
From: Jilles Tjoelker <jilles@stack.nl>
Date: Thu, 4 Aug 2016 17:51:12 +0200
Subject: [PATCH 1/2] [HISTEDIT] Stop depending on getopt reset feature.

Instead, use our own nextopt() function.
---
 src/histedit.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/histedit.c b/src/histedit.c
index 94465d7..ec45065 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -214,13 +214,8 @@ histcmd(int argc, char **argv)
 	if (argc == 1)
 		sh_error("missing history argument");
 
-#ifdef __GLIBC__
-	optind = 0;
-#else
-	optreset = 1; optind = 1; /* initialize getopt */
-#endif
 	while (not_fcnumber(argv[optind]) &&
-	      (ch = getopt(argc, argv, ":e:lnrs")) != -1)
+	      (ch = nextopt(":e:lnrs")) != '\0')
 		switch ((char)ch) {
 		case 'e':
 			editor = optionarg;
-- 
2.9.2

-- 
Jilles Tjoelker

Re: [PATCH 2/2] histedit: Remove non-glibc fallback code

[Jilles Tjoelker]

On Thu, Aug 04, 2016 at 05:59:08PM +0200, Jilles Tjoelker wrote:
> >From 23da600dcff616662a93f307420d9142598e2cae Mon Sep 17 00:00:00 2001
> From: Jilles Tjoelker <jilles@stack.nl>
> Date: Thu, 4 Aug 2016 17:51:12 +0200
> Subject: [PATCH 1/2] [HISTEDIT] Stop depending on getopt reset feature.

> Instead, use our own nextopt() function.
> ---
>  src/histedit.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/src/histedit.c b/src/histedit.c
> index 94465d7..ec45065 100644
> --- a/src/histedit.c
> +++ b/src/histedit.c
> @@ -214,13 +214,8 @@ histcmd(int argc, char **argv)
>  	if (argc == 1)
>  		sh_error("missing history argument");
>  
> -#ifdef __GLIBC__
> -	optind = 0;
> -#else
> -	optreset = 1; optind = 1; /* initialize getopt */
> -#endif
>  	while (not_fcnumber(argv[optind]) &&
> -	      (ch = getopt(argc, argv, ":e:lnrs")) != -1)
> +	      (ch = nextopt(":e:lnrs")) != '\0')
>  		switch ((char)ch) {
>  		case 'e':
>  			editor = optionarg;

This is clearly wrong; not_fcnumber() should be passed *argptr instead
of something bogus depending on optind.

The fixed version is what FreeBSD sh has as of SVN r240541 but I have
not tested it in dash.

In any case, a side effect of this change is a small code size
reduction.

-- 
Jilles Tjoelker

Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`

[Harald van Dijk]

On 04/08/2016 07:54, Kylie McClain wrote:
> From: Kylie McClain <somasis@exherbo.org>
>
> nl, while specified in POSIX, is rather obscure and isn't provided by small
> coreutils implementations such as `busybox`. This while loop works just as
> well for our purposes.
...
> -sed 's/	-[a-z]*//' $temp2 | nl -ba -v0 |
> +sed 's/	-[a-z]*//' $temp2 | while read line;do \
> +	i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |

$(( ... )) is mentioned in 
<https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html> 
as not universally supported, notably Solaris 10 /bin/sh does not have 
it. Given that dash was fairly recently changed to make it build on 
Solaris 9, it seems like a mistake to break that again.

Aside from that, i is such a common variable name that it seems risky to 
assume it is unset. I know I've set it myself in shell sessions that I 
ended up using for building dash. I never exported it, so it wouldn't 
break here, but it doesn't seem like a stretch that someone else does 
export it.

Cheers,
Harald van Dijk

Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`

[Seb]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Thu, Aug 04, 2016 at 07:17:47PM +0200, Harald van Dijk wrote:

> Given that dash was fairly recently changed to make it build on
> Solaris 9, it seems like a mistake to break that again.

Hello,

This is an attempt to simplify the current implementation. This
one does not require any temporary file anymore and relies only
on sort and (a rather basic usage of) awk.

I've checked the the Opensolaris' awk manual, trying to not
introduce some unsupported syntax. I still wonder about the
escaped newlines and the (currently commented) close()
statements, but both can be easily addressed. So, if someone
thinks it's worth testing...

++
Seb.

[-- Attachment #2: mkbuiltins --]
[-- Type: text/plain, Size: 2382 bytes --]

#!/bin/sh

LC_ALL=C      # Force the collate order of the builtins.
export LC_ALL # some shells may not support the "export FOO=bar" form.

awk '
(NF && ($1 !~ /^#/)) {
    # command [options] alias1 [[options] alias2] ...
    for (i = 2; i <= NF; i++) {
        mask = 0
        cmd = $1
        if ($i ~ /^-/) {
            if ($i ~ /n/)
                cmd = "NULL"
            if ($i ~ /s/)
                mask += 1
            if ($i ~ /[su]/)
                mask += 2
            if ($i ~ /a/)
                mask += 4
            i++
        }
        print $i, cmd, mask, $1
    }
}' $1 | sort -k1,1 | awk '
BEGIN {
    BUILTINS_H = "./builtins.h"
    BUILTINS_C = "./builtins.c"
    warn = "/*\n * This file was generated by the mkbuiltins program.\n */\n"
    print  warn >BUILTINS_H
    print  warn "\n#include \"shell.h\"" \
                "\n#include \"builtins.h\"\n" >BUILTINS_C
}
(!($NF in DEFINE)) {
    up = $NF # /bin/awk has no toupper() on Solaris.
    gsub(/a/, "A", up); gsub(/j/, "J", up); gsub(/s/, "S", up)
    gsub(/b/, "B", up); gsub(/k/, "K", up); gsub(/t/, "T", up)
    gsub(/c/, "C", up); gsub(/l/, "L", up); gsub(/u/, "U", up)
    gsub(/d/, "D", up); gsub(/m/, "M", up); gsub(/v/, "V", up)
    gsub(/e/, "E", up); gsub(/n/, "N", up); gsub(/w/, "W", up)
    gsub(/f/, "F", up); gsub(/o/, "O", up); gsub(/x/, "X", up)
    gsub(/g/, "G", up); gsub(/p/, "P", up); gsub(/y/, "Y", up)
    gsub(/h/, "H", up); gsub(/q/, "Q", up); gsub(/z/, "Z", up)
    gsub(/i/, "I", up); gsub(/r/, "R", up)


    print "#define "up" (builtincmd + "(NR-1)")" >BUILTINS_H
    print "int "$NF"(int, char **);" >BUILTINS_C
    DEFINE[$NF]
}
{
    CMD[NR] = "\""$1"\", "$2", "$3
}
END {
    print "\n#define NUMBUILTINS "NR"\n" \
          "\n#define BUILTIN_SPECIAL 0x1" \
          "\n#define BUILTIN_REGULAR 0x2" \
          "\n#define BUILTIN_ASSIGN  0x4\n" \
          "\nstruct builtincmd {" \
          "\n    const char *name;" \
          "\n    int (*builtin)(int, char **);" \
          "\n    unsigned flags;" \
          "\n};" \
          "\n\nextern const struct builtincmd builtincmd[];" >BUILTINS_H
    # close(BUILTINS_H) # not supported on Solaris ? 

    print "\nconst struct builtincmd builtincmd[] = {" >BUILTINS_C
    for (i = 1; i <= NR; i++)
        print "\t{ "CMD[i]" }," >BUILTINS_C
    print "};" >BUILTINS_C
    # close(BUILTINS_C)
}'

# EoF

Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`

[Seb]

On Sat, Aug 06, 2016 at 11:02:42AM +0200, Seb wrote:
> This is an attempt to simplify the current implementation [...]

BTW, I wonder if there is not a (virtual) bug with the current
mkbuiltins. Look at the TRUECMD macro in the generated builtins.h:

 #define TRUECMD (builtincmd + 1)

It doesn't point on "true" (mask=2, "special builtin") but on ":"
(mask=3, "standard builtin"), whatever the order of the aliases
you set in builtins.def.in. Maybe it's a mere problem with the
macro name ("TRUECMD" should rather be "COLONCMD"), but I don't
know if it is really an intended behaviour as the result is
finally a matter of sorting.

++
Seb.

Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`

[Harald van Dijk]

On 04/08/2016 19:17, Harald van Dijk wrote:
> On 04/08/2016 07:54, Kylie McClain wrote:
>> From: Kylie McClain <somasis@exherbo.org>
>>
>> nl, while specified in POSIX, is rather obscure and isn't provided by
>> small
>> coreutils implementations such as `busybox`. This while loop works
>> just as
>> well for our purposes.
> ...
>> -sed 's/    -[a-z]*//' $temp2 | nl -ba -v0 |
>> +sed 's/    -[a-z]*//' $temp2 | while read line;do \
>> +    i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |
>
> $(( ... )) is mentioned in
> <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html>
> as not universally supported, notably Solaris 10 /bin/sh does not have
> it. Given that dash was fairly recently changed to make it build on
> Solaris 9, it seems like a mistake to break that again.

I just realised that that particular change to make it build on Solaris 
9 involved avoiding running mkbuiltins using sh, choosing to use $SHELL 
instead, as $SHELL should already have been picked by configure as 
something more modern. So the use of $(( ... )) should probably not be a 
problem after all...

> Aside from that, i is such a common variable name that it seems risky to
> assume it is unset. I know I've set it myself in shell sessions that I
> ended up using for building dash. I never exported it, so it wouldn't
> break here, but it doesn't seem like a stretch that someone else does
> export it.

...as long as i is simply initialised before the loop.

Cheers,
Harald van Dijk

Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`

[Herbert Xu]

Harald van Dijk <harald@gigawatt.nl> wrote:
> On 04/08/2016 07:54, Kylie McClain wrote:
>> From: Kylie McClain <somasis@exherbo.org>
>>
>> nl, while specified in POSIX, is rather obscure and isn't provided by small
>> coreutils implementations such as `busybox`. This while loop works just as
>> well for our purposes.
> ...
>> -sed 's/      -[a-z]*//' $temp2 | nl -ba -v0 |
>> +sed 's/      -[a-z]*//' $temp2 | while read line;do \
>> +     i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |
> 
> $(( ... )) is mentioned in 
> <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html> 
> as not universally supported, notably Solaris 10 /bin/sh does not have 
> it. Given that dash was fairly recently changed to make it build on 
> Solaris 9, it seems like a mistake to break that again.
> 
> Aside from that, i is such a common variable name that it seems risky to 
> assume it is unset. I know I've set it myself in shell sessions that I 
> ended up using for building dash. I never exported it, so it wouldn't 
> break here, but it doesn't seem like a stretch that someone else does 
> export it.

nl has been around forever.  I'm not taking this patch.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt