Example / partial fix for printf with math expressions

Example / partial fix for printf with math expressions

[Bart Schaefer]

No attachment, so beware line wrap.

This repairs (printf "%d" ...) but obviously hasn't touched any of the
several other places where bin_print() calls matheval() and friends.
Upshot, math expects to get metafied strings, bin_print() does not (or
at least not always) pass them that way.

I also did not investigate whether curarg needs a known length rather
than calling metafy with -1, but since math is internally going to
stop at nul bytes anyway, I don't think it matters.

Also because the strings are metafied, checkunary() needs some
MULTIBYTE tweaking to get the count right and not split characters in
the middle when showing an error.  Someone who works more with that
should investigate.

diff --git a/Src/builtin.c b/Src/builtin.c
index dd352c146..5fccbac6d 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5465,7 +5465,8 @@ bin_print(char *name, char **args, Options ops, int func)
                        *d++ = 'l';
 #endif
                        *d++ = 'l', *d++ = *c, *d = '\0';
-                       zlongval = (curarg) ? mathevali(curarg) : 0;
+                       zlongval = (curarg) ?
+                           mathevali(metafy(curarg, -1, META_HEAPDUP)) : 0;
                        if (errflag) {
                            zlongval = 0;
                            errflag &= ~ERRFLAG_ERROR;

Re: Example / partial fix for printf with math expressions

[Stephane Chazelas]

2024-02-23 15:23:52 -0800, Bart Schaefer:
> No attachment, so beware line wrap.
> 
> This repairs (printf "%d" ...) but obviously hasn't touched any of the
> several other places where bin_print() calls matheval() and friends.
> Upshot, math expects to get metafied strings, bin_print() does not (or
> at least not always) pass them that way.
> 
> I also did not investigate whether curarg needs a known length rather
> than calling metafy with -1, but since math is internally going to
> stop at nul bytes anyway, I don't think it matters.
[...]

Ah sorry, I hadn't seen that message when replying in the other
thread.

The math parser seems to work OK with NULs at least in:

$ typeset -A a
$ typeset -p a
typeset -A a=( [$'\C-@']=1 )
$ let $'b = (a = ##\0) + 32'; echo $a $b
0 32


With and without your example patch applied however:

$ printf '%d\n' $'a[\0]++'
zsh: invalid subscript
0

With the one below using curlen / len[argp-args]:

$ typeset -A a
$ printf '%d\n' $'a[\0]++'
0
$ typeset -p a
typeset -A a=( [$'\C-@']=1 )

diff --git a/Src/builtin.c b/Src/builtin.c
index dd352c146..f72d14da4 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5247,7 +5247,8 @@ bin_print(char *name, char **args, Options ops, int func)
 		    }
 		}
 		if (*argp) {
-		    width = (int)mathevali(*argp++);
+		    width = (int)mathevali(metafy(*argp, len[argp - args], META_USEHEAP));
+		    argp++;
 		    if (errflag) {
 			errflag &= ~ERRFLAG_ERROR;
 			ret = 1;
@@ -5281,7 +5282,8 @@ bin_print(char *name, char **args, Options ops, int func)
 		    }
 
 		    if (*argp) {
-			prec = (int)mathevali(*argp++);
+			prec = (int)mathevali(metafy(*argp, len[argp - args], META_USEHEAP));
+			argp++;
 			if (errflag) {
 			    errflag &= ~ERRFLAG_ERROR;
 			    ret = 1;
@@ -5465,7 +5467,7 @@ bin_print(char *name, char **args, Options ops, int func)
  		    	*d++ = 'l';
 #endif
 		    	*d++ = 'l', *d++ = *c, *d = '\0';
-			zlongval = (curarg) ? mathevali(curarg) : 0;
+			zlongval = (curarg) ? mathevali(metafy(curarg, curlen, META_HEAPDUP)) : 0;
 			if (errflag) {
 			    zlongval = 0;
 			    errflag &= ~ERRFLAG_ERROR;
@@ -5516,7 +5518,7 @@ bin_print(char *name, char **args, Options ops, int func)
 			if (!curarg)
 			    zulongval = (zulong)0;
 			else if (!zstrtoul_underscore(curarg, &zulongval))
-			    zulongval = mathevali(curarg);
+			    zulongval = mathevali(metafy(curarg, curlen, META_HEAPDUP));
 			if (errflag) {
 			    zulongval = 0;
 			    errflag &= ~ERRFLAG_ERROR;

Re: Example / partial fix for printf with math expressions

[Stephane Chazelas]

2024-02-24 14:40:41 +0000, Stephane Chazelas:
[...]
> The math parser seems to work OK with NULs at least in:
> 
> $ typeset -A a
> $ typeset -p a
> typeset -A a=( [$'\C-@']=1 )
[...]

Sorry, I messed up my copy-paste again. Should have been:

$ typeset -A a
$ let $'a[\0]++'
$ typeset -p a
typeset -A a=( [$'\C-@']=1 )

-- 
Stephane

Re: Example / partial fix for printf with math expressions

[Bart Schaefer]

Thanks for the builtin.c patch.  Just a couple of notes:

On Sat, Feb 24, 2024 at 6:40 AM Stephane Chazelas
<stephane@chazelas.org> wrote (with correction from a later message):
>
> The math parser seems to work OK with NULs at least in:
>
> $ typeset -A a
> $ let $'a[\0]++'

That's not exactly the math parser; mathparse() get as far as noticing
it's a parameter reference and then goes through
params.c:getnumvalue() which actually does the subscript parsing.
You're "cheating" with this construction (and of course "let" is doing
the metafication properly to pass the reference through).

> $ let $'b = (a = ##\0) + 32'; echo $a $b

For the ## operator the parse is reading just one byte, so you're
still not involving anything that might hit a nul-terminated string.

This is the math parser and only the math parser, but again with
proper metafication:

% let $'a\0b = 2'
zsh: bad math expression: illegal character: \M-C
% let $'ab \0 2'
zsh: bad math expression: illegal character: \M-C
% let $'ab \02'
zsh: bad math expression: illegal character: ^B
% let $'ab y\02'
zsh: bad math expression: operator expected at `y^B'
% let $'ab = \02'
zsh: bad math expression: operand expected at `^B'

I'm not sure it's actually very useful to display more than the first
character in the latter two cases, but we can try to improve what's
currently displayed in multibyte cases.

Compare math parser without metafication:

% a=42
% printf "%d\n" $'a\0b = 2'
42

This is what I meant by "stops at NUL".

> With and without your example patch applied however:
>
> $ printf '%d\n' $'a[\0]++'
> zsh: invalid subscript

That does demonstrate that the length is needed in bin_print() to
properly metafy the subscript, yes.

Re: Example / partial fix for printf with math expressions

[Stephane Chazelas]

2024-02-24 14:40:41 +0000, Stephane Chazelas:
[...]
> With the one below using curlen / len[argp-args]:
[...]
> +			zlongval = (curarg) ? mathevali(metafy(curarg, curlen, META_HEAPDUP)) : 0;
[...]

That incur a performance penalty:

$ time zsh -c 'repeat 1000 printf %d 123_456+{1..10000}' > /dev/null
zsh -c 'repeat 1000 printf %d 123_456+{1..10000}' > /dev/null  6.07s user 0.46s system 99% cpu 6.544 total
$ time ./Src/zsh -c 'repeat 1000 printf %d 123_456+{1..10000}' > /dev/null
./Src/zsh -c 'repeat 1000 printf %d 123_456+{1..10000}' > /dev/null  9.97s user 0.62s system 99% cpu 10.602 total

The numbers end up being metafied (earlier), unmetafied by
printf, metafied again by printf here and that metafication
processed by the math handler I guess.

Maybe the performance could be *improved* instead if the
unmetafication could be skipped altogether in printf in those
cases?

-- 
Stephane

Re: Example / partial fix for printf with math expressions

[Bart Schaefer]

On Sat, Feb 24, 2024 at 10:54 PM Stephane Chazelas
<stephane@chazelas.org> wrote:
>
> 2024-02-24 14:40:41 +0000, Stephane Chazelas:
> [...]
> > +                     zlongval = (curarg) ? mathevali(metafy(curarg, curlen, META_HEAPDUP)) : 0;
>
> That incur a performance penalty:
>
> The numbers end up being metafied (earlier), unmetafied by
> printf, metafied again by printf here and that metafication
> processed by the math handler I guess.

The reason this went unobserved for so long is that metafication only
matters to a couple of things in math:
1) proper handling of parameter references, like your a[x]++ example
2) those pesky error messages that started this whole thread

As far as I can tell there are no other circumstances in which
metafication is necessary for the syntax of math expressions.

> Maybe the performance could be *improved* instead if the
> unmetafication could be skipped altogether in printf in those
> cases?

That's certainly possible, but it would mean that every branch of the
big "case" statements for %-replacement would have to do their own
unmetafy(), rather than a single loop at the top covering the entire
argv array.  The -s/-S/-z options also reverse the unmetafy(), so some
heavy refactoring of bin_print() is needed to unroll this completely.

However, I think you could change META_HEAPDUP and META_USEHEAP to
META_NOALLOC, becuase you're re-metafying back into the same space
that was originally unmetafied?  That might cut the performance
penalty a lot.

Re: Example / partial fix for printf with math expressions

[Bart Schaefer]

On Sun, Feb 25, 2024 at 4:04 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> However, I think you could change META_HEAPDUP and META_USEHEAP to
> META_NOALLOC, becuase you're re-metafying back into the same space
> that was originally unmetafied?  That might cut the performance
> penalty a lot.

No metafication:
zsh -c 'repeat 1000 printf %d 123_456+{1..10000}' > /dev/null  7.51s
user 0.39s system 99% cpu 7.907 total

USEHEAP:
Src/zsh -c 'repeat 1000 printf %d 123_456+{1..10000}' > /dev/null
8.62s user 0.59s system 99% cpu 9.218 total

NOALLOC:
Src/zsh -c 'repeat 1000 printf %d 123_456+{1..10000}' > /dev/null
7.89s user 0.36s system 99% cpu 8.255 total

All of this is with full debugging, an optimized compile might change it.