PATCH: Avoid \e in C code; building on Solaris 11

PATCH: Avoid \e in C code; building on Solaris 11

[Oliver Kiddle]

A couple of uses of the non-standard \e in C strings have crept into the
code in relatively recent patches. Most modern compilers support this
but the Solaris compiler doesn't and \033 should be used instead.

Incidentally, .sh.edmode doesn't appear to work on any system in
my testing, even on other platforms. I get an empty variable and
${#.sh.edmode} remains zero.

The change to makepro.awk in 44637 also means that AWK=gawk is needed
with the configure script on Solaris. Even before, I tended to use
AWK=nawk and as it silences an ugly warning on modern systems I wouldn't
back that patch out. Annoyingly, it does leave a size zero
Src/builtin.syms and make clean then doesn't delete that (it fails) and
a subsequent attempt with gawk then fails to compile builtin.c. For GNU
make there is a .DELETE_ON_ERROR: target that would solve that but I
can't see an equivalent for Sun make.

The test cases now expect diff to have a -a option which it doesn't on
Solaris. That was changed in ztst.zsh in 2017 and I don't see much need
to change this.

B03print fails on Tab expansion by print. This is because the test
expects tr '\0' Z to work which it doesn't. Neither does the full octal
'\000' or $'\0'. I'm not clear on what purpose that trailing null has
for the test but it is commented with "regression test for multibyte tab
expand".

/dev/fd tests are being skipped because it doesn't detect /dev/fd.
That test is: echo ok|(exec 3<&0; cat /dev/fd/3 2>/dev/null;)
It works with bash or zsh on Solaris 11 but not with sh or ksh
On Solaris 10, it does work with /bin/sh (just not with ksh)
Any idea why the use of fd 3 is needed in our test? It works with just:
  echo ok|(cat /dev/fd/0 2>/dev/null;)
I've tried a few variations such as (/bin/echo ok;sleep 1)|
without success.

Aside from that, there are a couple of failures in W02jobs which I can't
repdroduce outside of the test system.

Oliver

diff --git a/Src/Modules/ksh93.c b/Src/Modules/ksh93.c
index 51999dd71..9af5e1d69 100644
--- a/Src/Modules/ksh93.c
+++ b/Src/Modules/ksh93.c
@@ -171,7 +171,7 @@ ksh93_wrapper(Eprog prog, FuncWrap w, char *name)
 	/* bindkey -v forces VIMODE so this test is as good as any */
 	if (curkeymapname && isset(VIMODE) &&
 	    strcmp(curkeymapname, "main") == 0)
-	    strcpy(sh_edmode, "\e");
+	    strcpy(sh_edmode, "\033");
 	else
 	    strcpy(sh_edmode, "");
 	if (!sh_edchar)
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 51bb43339..e2b86e863 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -977,7 +977,7 @@ cuttext(ZLE_STRING_T line, int ct, int flags)
 	unmetafy(mbcut, &cutll);
 	mbcut = base64_encode(mbcut, cutll);
 
-	fprintf(shout, "\e]52;%c;%s\a", zmod.flags & MOD_CLIP ? 'c' : 'p',
+	fprintf(shout, "\033]52;%c;%s\a", zmod.flags & MOD_CLIP ? 'c' : 'p',
 		mbcut);
     } else if (zmod.flags & MOD_VIBUF) {
 	struct cutbuffer *b = &vibuf[zmod.vibuf];

Re: PATCH: Avoid \e in C code; building on Solaris 11

[Bart Schaefer]

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

On Fri, Dec 8, 2023 at 10:29 AM Oliver Kiddle <opk@zsh.org> wrote:

> A couple of uses of the non-standard \e in C strings have crept into the
> code in relatively recent patches. Most modern compilers support this
> but the Solaris compiler doesn't and \033 should be used instead.
>

Sorry about that.


> Incidentally, .sh.edmode doesn't appear to work on any system in
> my testing, even on other platforms. I get an empty variable and
> ${#.sh.edmode} remains zero.
>

The problem with .sh.edmode seems to be that isset(VIMODE) is false.  I
thought we'd rigged it so that "bindkey -v" would implicitly perform
"setopt vi"?

If you explicitly "set -o vi" (which would be the ksh way to get there)
then .sh.edmode works.


> Annoyingly, it does leave a size zero
> Src/builtin.syms and make clean then doesn't delete that (it fails) and
> a subsequent attempt with gawk then fails to compile builtin.c.
>

"(it fails)" means "make clean" actually returns error, or just that it
doesn't have a rule for removing .syms files?

Would it work to prefix the awk recipe with "-" to ignore that error, and
then append another line to that recipe to clean up on awk failure?


> B03print fails on Tab expansion by print. This is because the test
> expects tr '\0' Z to work which it doesn't. Neither does the full octal
> '\000' or $'\0'. I'm not clear on what purpose that trailing null has
> for the test but it is commented with "regression test for multibyte tab
> expand".
>

The test is confirming that zexpandtabs() doesn't infinite-loop on broken
multibyte input which might include nul bytes.  Could replace the "tr" with
something else.


> /dev/fd tests are being skipped because it doesn't detect /dev/fd.
> That test is: echo ok|(exec 3<&0; cat /dev/fd/3 2>/dev/null;)
>

(This refers to configure.ac, not Test/*)

This is testing that /dev/fd/ entries are created when new descriptors are
created.  The commit log says "work around /dev/fd problem on FreeBSD":
+dnl FreeBSD 5 only supports /dev/fd/0 to /dev/fd/2 without mounting
+dnl a special file system.  As zsh needs arbitrary /dev/fd (typically
+dnl >10) for its own use, we need to make sure higher fd's are available.

Aside from that, there are a couple of failures in W02jobs which I can't
> repdroduce outside of the test system.
>

W02jobs runs everything via zpty, there are some commented-out tests in
there marked with:
#### Races presumed to be associated with zpty mean that
#### tests involving suspending jobs are not safe.

It would be unsurprising if there are other races that need to be accounted
for.

[-- Attachment #2: Type: text/html, Size: 4086 bytes --]

Re: PATCH: Avoid \e in C code; building on Solaris 11

[Bart Schaefer]

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

On Fri, Dec 8, 2023 at 2:03 PM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> The test is confirming that zexpandtabs() doesn't infinite-loop on broken
> multibyte input which might include nul bytes.  Could replace the "tr" with
> something else.
>

E.g. (not bothering to worry about gmail wrapping here)

diff --git a/Test/B03print.ztst b/Test/B03print.ztst
index 4d2cf9764..93a9669b0 100644
--- a/Test/B03print.ztst
+++ b/Test/B03print.ztst
@@ -305,8 +305,9 @@
  foo+=$'\tone\ttwo\tthree\tfour\n'
  foo+=$'\t\tone\t\ttwo\t\tthree\t\tfour'
  foo+='\0' # regression test for multibyte tab expand
- print -x4 $foo | tr '\0' Z # avoid raw nul byte in expected output below
- print -X4 $foo | tr '\0' Z
+ # avoid raw nul byte in expected output below
+ print ${"$(print -x4 $foo)"/$'\0'/Z}
+ print ${"$(print -X4 $foo)"/$'\0'/Z}
 0:Tab expansion by print
 >one two three four
 >    one two three four

[-- Attachment #2: Type: text/html, Size: 1383 bytes --]

Re: PATCH: Avoid \e in C code; building on Solaris 11

[Oliver Kiddle]

Bart Schaefer wrote:
>     Annoyingly, it does leave a size zero
>     Src/builtin.syms and make clean then doesn't delete that (it fails) and
>     a subsequent attempt with gawk then fails to compile builtin.c.
>
> "(it fails)" means "make clean" actually returns error, or just that it doesn't
> have a rule for removing .syms files?

make clean returns an error. Looks like the Src/Makemod tries to recurse
into Src/Builtins and that doesn't contain a Makefile. It had never got
far enough to create it.

> Would it work to prefix the awk recipe with "-" to ignore that error, and then
> append another line to that recipe to clean up on awk failure?

If you use an initial -, I don't think a subsequent line can get the
return status. make uses separate shell instances for each line. The
best I can think of is to append || (rm $@ && false)
However, after taking another look at the awk script, I think the
following patch is the best solution - using the octal escape \075 for
the equals.
  awk '/=/'  is a syntax error
  gawk '/\=/' complains that `\=' is not a known regexp operator
And while it may be less readable, both are happy with '/\075/'

Oliver

diff --git a/Src/makepro.awk b/Src/makepro.awk
index f69660531..0d53c5850 100644
--- a/Src/makepro.awk
+++ b/Src/makepro.awk
@@ -121,7 +121,7 @@ BEGIN {
 		# initialiser.
 		dcltor = substr(line, 1, RLENGTH-1)
 		line = substr(line, RLENGTH+1)
-		sub(/=.*$/, "", dcltor)
+		sub(/\075.*$/, "", dcltor)
 		match(dcltor, /^([^_0-9A-Za-z]| const )*/)
 		dcltor = substr(dcltor, 1, RLENGTH) "@+" substr(dcltor, RLENGTH+1)
 		match(dcltor, /^.*@\+[_0-9A-Za-z]+/)

Re: PATCH: Avoid \e in C code; building on Solaris 11

[Oliver Kiddle]

Bart Schaefer wrote:
> On Fri, Dec 8, 2023 at 2:03 PM Bart Schaefer <[1]schaefer@brasslantern.com>
> wrote:
>
>     The test is confirming that zexpandtabs() doesn't infinite-loop on broken
>     multibyte input which might include nul bytes.  Could replace the "tr" with
>     something else.
>
>
> E.g. (not bothering to worry about gmail wrapping here)

That works. Thanks

Any objection to the following so that manual hacks are not needed to
run the test cases. This is just checking $OSTYPE for solairs.

Incidentally, I tried whether the new ${ print -x4 $foo } form might
also work here and it results in an extra trailing blank line in each
case. Is that difference expected?

Oliver

diff --git a/Test/ztst.zsh b/Test/ztst.zsh
index ea1b016d5..1d05baddf 100755
--- a/Test/ztst.zsh
+++ b/Test/ztst.zsh
@@ -326,6 +326,7 @@ ZTST_diff() {
   emulate -L zsh
   setopt extendedglob
 
+  local -a diff_arg
   local diff_out
   integer diff_pat diff_ret
 
@@ -342,6 +343,7 @@ ZTST_diff() {
     ;;
   esac
   shift
+  [[ $OSTYPE != solaris* ]] && diff_arg=( -a )
       
   if (( diff_pat )); then
     local -a diff_lines1 diff_lines2
@@ -382,7 +384,7 @@ ZTST_diff() {
       diff_ret=1
     fi
   else
-    diff_out=$(diff -a "$@")
+    diff_out=$(diff $diff_arg "$@")
     diff_ret="$?"
     if [[ "$diff_ret" != "0" ]]; then
       print -r -- "$diff_out"

Re: PATCH: Avoid \e in C code; building on Solaris 11

[Oliver Kiddle]

Bart Schaefer wrote:
>     /dev/fd tests are being skipped because it doesn't detect /dev/fd.
>     That test is: echo ok|(exec 3<&0; cat /dev/fd/3 2>/dev/null;)
>
> (This refers to [2]configure.ac, not Test/*)

Yes, sorry. However it was something that was only apparent when running
test cases because it caused some to get skipped.

It occurred to me that /bin/sh on Solaris 11 is ksh 93 and that test
consistently doesn't work with ksh (88 or 93). The same applies with
ksh93 on Linux.

> This is testing that /dev/fd/ entries are created when new descriptors are
> created.  The commit log says "work around /dev/fd problem on FreeBSD":
> +dnl FreeBSD 5 only supports /dev/fd/0 to /dev/fd/2 without mounting
> +dnl a special file system.  As zsh needs arbitrary /dev/fd (typically
> +dnl >10) for its own use, we need to make sure higher fd's are available.

While that talks about FreeBSD 5, that isn't a situation that has
changed at all. Annoyingly, most package builds take place in a jail
where fdescfs is not mounted so PATH_DEV_FD is probably undefined for
most users on FreeBSD even if they mount fdescfs.

Just doing test -e on /dev/fd/3 gives the same results as the existing
test on FreeBSD both with and without fdescfs. Do you see a problem with
this approach?

Oliver

diff --git a/configure.ac b/configure.ac
index a42758bf3..2871dcb7c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2016,7 +2016,7 @@ AH_TEMPLATE([PATH_DEV_FD],
 [Define to the path of the /dev/fd filesystem.])
 AC_CACHE_CHECK(for /dev/fd filesystem, zsh_cv_sys_path_dev_fd,
 [for zsh_cv_sys_path_dev_fd in /proc/self/fd /dev/fd no; do
-   test x`echo ok|(exec 3<&0; cat $zsh_cv_sys_path_dev_fd/3 2>/dev/null;)` = xok && break
+   (exec 3<&0; test -e $zsh_cv_sys_path_dev_fd/3;) && break
  done])
 if test x$zsh_cv_sys_path_dev_fd != xno; then
   AC_DEFINE_UNQUOTED(PATH_DEV_FD, "$zsh_cv_sys_path_dev_fd")

Re: PATCH: Avoid \e in C code; building on Solaris 11

[Bart Schaefer]

On Sat, Dec 9, 2023 at 3:04 PM Oliver Kiddle <opk@zsh.org> wrote:
>
> Incidentally, I tried whether the new ${ print -x4 $foo } form might
> also work here and it results in an extra trailing blank line in each
> case. Is that difference expected?

Yes, ${ ... } is defined to not strip trailing newlines the way $(...)
does.  I first had the new form too but decided $(...) was more
obvious than using "print -n".  If you don't need current-shell
semantics, it's a toss-up whether a subshell or a temp file read/write
is going to be more efficient.

No objection to the diff_arg patch on my part.

Re: PATCH: Avoid \e in C code; building on Solaris 11

[Bart Schaefer]

On Sat, Dec 9, 2023 at 3:43 PM Oliver Kiddle <opk@zsh.org> wrote:
>
> Just doing test -e on /dev/fd/3 gives the same results as the existing
> test on FreeBSD both with and without fdescfs. Do you see a problem with
> this approach?

I do not see a problem.