PATCH: hist: remove wrong NULL terminator

PATCH: hist: remove wrong NULL terminator

[Mikael Magnusson]

This actually writes a NULL to some arbitrary location in the caller function's stack. Found by Coverity (Issue 1255746).

The start of the quote() function does char **str = tr; and is called like this,
quote(&sline);
sline in turn is just a char *sline;
The result of str[1] = NULL; is then, as far as I can tell, not anything good. I also can't see any other thing that might have been intended to be NULL-terminated here, so just remove it.

---
 Src/hist.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Src/hist.c b/Src/hist.c
index e65d78b..3dc0472 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -2179,7 +2179,6 @@ quote(char **tr)
 	    *rptr++ = *ptr;
     *rptr++ = '\'';
     *rptr++ = 0;
-    str[1] = NULL;
     return 0;
 }
 
-- 
2.2.0.GIT

Re: PATCH: hist: remove wrong NULL terminator

[Ray Andrews]

On 01/09/2015 04:43 AM, Mikael Magnusson wrote:
> This actually writes a NULL to some arbitrary location in the caller function's stack. Found by Coverity (Issue 1255746).
>
> The start of the quote() function does char **str = tr; and is called like this,
> quote(&sline);
> sline in turn is just a char *sline;
> The result of str[1] = NULL; is then, as far as I can tell, not anything good. I also can't see any other thing that might have been intended to be NULL-terminated here, so just remove it.

Holy Cow.  That's just been sitting there for god knows how long?

Re: PATCH: hist: remove wrong NULL terminator

[Lawrence Velázquez]

On Jan 9, 2015, at 12:45 PM, Ray Andrews <rayandrews@eastlink.ca> wrote:

> Holy Cow.  That's just been sitting there for god knows how long?

Since before zsh was under SourceForge version control, it looks like.

    % git blame -L 2179,+6 Src/hist.c(git:master)
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2179)      *rptr++ = *ptr;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2180)     *rptr++ = '\'';
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2181)     *rptr++ = 0;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2182)     str[1] = NULL;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2183)     return 0;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2184) }
    % git show --shortstat e74702b4(git:master)
    commit e74702b467171dbdafb56dfe354794a212e020d9
    Author: Tanaka Akira <akr@users.sourceforge.net>
    Date:   Thu Apr 15 18:05:38 1999 +0000

        Initial revision

     354 files changed, 66043 insertions(+)

vq

Re: PATCH: hist: remove wrong NULL terminator

[Lawrence Velázquez]

On Jan 9, 2015, at 1:33 PM, Lawrence Velázquez <vq@larryv.me> wrote:

>    % git blame -L 2179,+6 Src/hist.c(git:master)
>    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2179)      *rptr++ = *ptr;
>    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2180)     *rptr++ = '\'';
>    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2181)     *rptr++ = 0;
>    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2182)     str[1] = NULL;
>    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2183)     return 0;
>    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2184) }
>    % git show --shortstat e74702b4(git:master)
>    commit e74702b467171dbdafb56dfe354794a212e020d9
>    Author: Tanaka Akira <akr@users.sourceforge.net>
>    Date:   Thu Apr 15 18:05:38 1999 +0000
> 
>        Initial revision
> 
>     354 files changed, 66043 insertions(+)

Badly-edited paste. In the interests of correctness:

    % git blame -L 2179,+6 Src/hist.c
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2179)      *rptr++ = *ptr;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2180)     *rptr++ = '\'';
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2181)     *rptr++ = 0;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2182)     str[1] = NULL;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2183)     return 0;
    e74702b4 (Tanaka Akira 1999-04-15 18:05:38 +0000 2184) }
    % git show --shortstat e74702b4
    commit e74702b467171dbdafb56dfe354794a212e020d9
    Author: Tanaka Akira <akr@users.sourceforge.net>
    Date:   Thu Apr 15 18:05:38 1999 +0000

        Initial revision

     354 files changed, 66043 insertions(+)

vq

Re: PATCH: hist: remove wrong NULL terminator

[Mikael Magnusson]

On Fri, Jan 9, 2015 at 6:45 PM, Ray Andrews <rayandrews@eastlink.ca> wrote:
> On 01/09/2015 04:43 AM, Mikael Magnusson wrote:
>>
>> This actually writes a NULL to some arbitrary location in the caller
>> function's stack. Found by Coverity (Issue 1255746).
>>
>> The start of the quote() function does char **str = tr; and is called like
>> this,
>> quote(&sline);
>> sline in turn is just a char *sline;
>> The result of str[1] = NULL; is then, as far as I can tell, not anything
>> good. I also can't see any other thing that might have been intended to be
>> NULL-terminated here, so just remove it.
>
>
> Holy Cow.  That's just been sitting there for god knows how long?

Yes, but it's actually pretty harmless, most other variables on the
stack in that function are never used if we enter this codepath.

-- 
Mikael Magnusson

Re: PATCH: hist: remove wrong NULL terminator

[Ray Andrews]

On 01/09/2015 11:38 AM, Mikael Magnusson wrote:

> Yes, but it's actually pretty harmless, most other variables on the 
> stack in that function are never used if we enter this codepath. 

I guess so, since otherwise there'd be crashes all over the place. Still, it
has a lethal look about it.  Trying to understand the culture of the project
I find myself wondering how things like that pass unnoticed, given the
unquestioned skill level of the people involved.

Re: PATCH: hist: remove wrong NULL terminator

[Peter Stephenson]

On Fri, 09 Jan 2015 13:39:06 -0800
Ray Andrews <rayandrews@eastlink.ca> wrote:
> On 01/09/2015 11:38 AM, Mikael Magnusson wrote:
> 
> > Yes, but it's actually pretty harmless, most other variables on the 
> > stack in that function are never used if we enter this codepath. 
> 
> I guess so, since otherwise there'd be crashes all over the place. Still, it
> has a lethal look about it.  Trying to understand the culture of the project
> I find myself wondering how things like that pass unnoticed, given the
> unquestioned skill level of the people involved.

Bart already answered this once.  There is no "magic away problems with
the force of my intellect" in programming any more than anywhere else.
There are things that happen to show up in real life; there's painstaking
use of tools as Mikael is doing; and there are things you happen to notice
because you were in the area.  The same is true of pretty much all
software apart from the very small subset that can be proved
mathematically correct (and even that's only necessarily true of the
representation of the software in the form you're looking at).  The next
best step is to use a better language than C.  I wasn't entirely joking
when I talked about D, but it's not an option for us.

pws

Re: PATCH: hist: remove wrong NULL terminator

[Ray Andrews]

On 01/09/2015 02:30 PM, Peter Stephenson wrote:


> Bart already answered this once. There is no "magic away problems with 
> the force of my intellect" in programming any more than anywhere else. 

Indeed not.
> There are things that happen to show up in real life; there's 
> painstaking use of tools as Mikael is doing; 

Yes, it seems like fantastic work to me.
> and there are things you happen to notice because you were in the 
> area. The same is true of pretty much all software apart from the very 
> small subset that can be proved mathematically correct (and even 
> that's only necessarily true of the representation of the software in 
> the form you're looking at). The next best step is to use a better 
> language than C. I wasn't entirely joking when I talked about D, but 
> it's not an option for us. pws 

Yeah, don't take it as me being bitchy Peter, it really is a question, 
not a veiled comment.  As I've learned, zsh is not a huge corporation 
(like I thought it would be), and there's always been more coders than 
testers.  And power seems to be valued above simplicity, and the code is 
'layered' very heavily which makes things difficult to understand.  
Still, seeing an assignment to what turns out to be a dangling pointer 
(if I'm saying that correctly) raises my eyebrows.  In the culture I 
come from, a good tester is valued above a good coder.  It's me who has 
to adapt of course.

Re: PATCH: hist: remove wrong NULL terminator

[Bart Schaefer]

On Fri, Jan 9, 2015 at 3:39 PM, Ray Andrews <rayandrews@eastlink.ca> wrote:
>
> Yeah, don't take it as me being bitchy Peter, it really is a question, not a
> veiled comment.

Except it *is* a veiled comment, because you ask how "the culture"
could allow this to "pass unnoticed" as if simply by having a
different attitude we could have emulated thousands of lines of code
in our heads to find all possible errors (even those that have no side
effects, like this one) during the years that sophisticated automatic
analysis tools were simply not available except possibly to
well-funded corporate teams.

Right.  Just a question.

Re: PATCH: hist: remove wrong NULL terminator

[Bart Schaefer]

On Jan 9,  1:43pm, Mikael Magnusson wrote:
} Subject: PATCH: hist: remove wrong NULL terminator
}
} The result of str[1] = NULL; is then, as far as I can tell, not
} anything good. I also can't see any other thing that might have been
} intended to be NULL-terminated here, so just remove it.

That whole function is a bit odd.  It's exported, but nothing calls it
except in this file.  My compiler doesn't complain if changed to static.

And both it and quotebreak() start like

    quote(char **tr)
    {
	char *ptr, *rptr, **str = (char **)tr;

what is the cast for if tr is already char** ?

Anyway I don't see any reason not to commit the patch from 34178.

Re: PATCH: hist: remove wrong NULL terminator

[Ray Andrews]

On 01/09/2015 04:39 PM, Bart Schaefer wrote:
> Except it *is* a veiled comment, because you ask how "the culture"
> could allow this to "pass unnoticed"

No sir, it is a question.  And I think the answer has been given, and it 
is that
there has tended to be a shortage of testers.
> as if simply by having a
> different attitude we could have emulated thousands of lines of code
> in our heads to find all possible errors

... and that's another part of it, that things are so complicated.
> (even those that have no side

... and that it's not serious anyway.  (In DOS, something like that would be
a crasher.)
> effects, like this one) during the years that sophisticated automatic
> analysis tools were simply not available except possibly to
> well-funded corporate teams.
... and that too: zsh is not a big project nor funded in any way. Quite
astonishing what has been achieved, given all that, I'd say.

And as to 'simply having a different attitude' ... I wish it was simple,
look at the trouble I'm having ;-)

I'm bugging people, better drop out of sight.
>
>

Re: PATCH: hist: remove wrong NULL terminator

[Lawrence Velázquez]

On Jan 10, 2015, at 2:45 AM, Ray Andrews <rayandrews@eastlink.ca> wrote:

> On 01/09/2015 04:39 PM, Bart Schaefer wrote:
>> Except it *is* a veiled comment, because you ask how "the culture"
>> could allow this to "pass unnoticed"
> 
> No sir, it is a question.

Surely you are familiar with the concept of connotation, whether
intentional or not. Compare this…

    "This bug seems like it could have been caught sooner with
    better testing and code auditing. Is there a reason you don't do
    more of that? Here are some suggestions for making testing
    easier and more robust."

…with…

    "WOW, this is a REALLY DUMB MISTAKE. How could a bunch of people
    who are *so smart* make such a _boneheaded_ blunder? It confuses
    me because I'm used to a development culture that puts software
    quality *first*. I'm not commenting on anything, just asking
    a question."

If you think I'm exaggerating, go back and read what you wrote. It's not
much of an exaggeration.

vq

Re: PATCH: hist: remove wrong NULL terminator

[Ray Andrews]

On 01/10/2015 02:04 PM, Lawrence Velázquez wrote:
> Surely you are familiar with the concept of connotation, whether
> intentional or not. Compare this…
>
>      "This bug seems like it could have been caught sooner with
>      better testing and code auditing. Is there a reason you don't do
>      more of that? Here are some suggestions for making testing
>      easier and more robust."
>
> …with…
>
>      "WOW, this is a REALLY DUMB MISTAKE. How could a bunch of people
>      who are *so smart* make such a _boneheaded_ blunder? It confuses
>      me because I'm used to a development culture that puts software
>      quality *first*. I'm not commenting on anything, just asking
>      a question."
>
...with...

"In what environment does that sort of thing happen? Is it really
that important anyway? How should I view this? We'd have considered
that serious in DOS, but maybe it's a detail here. Looks like we need
more testers? That's me! OTOH, it seems that zsh/linux code is just
naturally more robust, maybe?  Seems so. Is this 'bad' or just par for
the course, I wonder. So many added layers!  Must it be so? Perhaps
I need a complete change in how I view a code defect."

Re: PATCH: hist: remove wrong NULL terminator

[Bart Schaefer]

On Jan 10,  2:50pm, Ray Andrews wrote:
}
} Looks like we need more testers? That's me!

OK, I'll bite.  Exactly what test would you have applied that would have
found that stray assignment?

Not that it wouldn't be good to have someone doing testing / writing new
cases for Test/*.ztst.

Re: PATCH: hist: remove wrong NULL terminator

[Ray Andrews]

On 01/10/2015 05:58 PM, Bart Schaefer wrote:
> On Jan 10,  2:50pm, Ray Andrews wrote:
> }
> } Looks like we need more testers? That's me!
>
> OK, I'll bite.  Exactly what test would you have applied that would have
> found that stray assignment?
>
> Not that it wouldn't be good to have someone doing testing / writing new
> cases for Test/*.ztst.
>
I'm speaking in the most general terms, not about anything in
particular.  As I become more competent, I'll test the hell out
of everything. I'd like to know how this Coverity thing works
it seems fantastic.

Speaking of testing, I can't get modulus to work nohow.

      $ echo $(( 2.5 ));
      2.5

      $ echo $(( 2.5 % 2  ));
      0

... If I'm doing something stupid there ...
That's with force_float.  Modulus with integers is fine. Other
operators fine.

Floating point modulus

[Bart Schaefer]

On Jan 10,  9:46pm, Ray Andrews wrote:
}
} Speaking of testing, I can't get modulus to work nohow.
} 
}       $ echo $(( 2.5 ));
}       2.5
} 
}       $ echo $(( 2.5 % 2  ));
}       0
} 
} ... If I'm doing something stupid there ...

Zsh implements the C modulus operator (%) directly, and it's defined
only on integers.  If you try that in C it won't even compile:

    error: invalid operands to binary %

Consequently the operands are silently converted to integers, instead
of the shell throwing an error.

The expectation is that you will know that zsh math is C math (same
as for why things are integer by default) and that you must do:

    $ zmodload zsh/mathfunc
    $ echo $(( fmod(2.5, 2) ))
    0.5

The following patch would instead silently substitute the fmod() call
when using the % operator on a float.  I won't commit it without some
further discussion from the group.  With more effort it could be made
to do this only when FORCE_FLOAT is in effect, though I'm not sure
how many such special-cases we want to manage.


diff --git a/Src/math.c b/Src/math.c
index 438a170..6d096e0 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -288,11 +288,11 @@ static int type[TOKCOUNT] =
 {
 /*  0 */  LR, LR|OP_OP|OP_OPF, RL, RL, RL|OP_OP|OP_OPF,
 /*  5 */  RL|OP_OP|OP_OPF, RL, RL, LR|OP_A2IO, LR|OP_A2IO,
-/* 10 */  LR|OP_A2IO, LR|OP_A2, LR|OP_A2, LR|OP_A2IO, LR|OP_A2,
+/* 10 */  LR|OP_A2IO, LR|OP_A2, LR|OP_A2, LR|OP_A2, LR|OP_A2,
 /* 15 */  LR|OP_A2, LR|OP_A2IO, LR|OP_A2IO, LR|OP_A2IR, LR|OP_A2IR,
 /* 20 */  LR|OP_A2IR, LR|OP_A2IR, LR|OP_A2IR, LR|OP_A2IR, BOOL|OP_A2IO,
 /* 25 */  BOOL|OP_A2IO, LR|OP_A2IO, RL|OP_OP, RL|OP_OP, RL|OP_E2,
-/* 30 */  RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2IO,
+/* 30 */  RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2,
 /* 35 */  RL|OP_E2IO, RL|OP_E2IO, RL|OP_E2IO, RL|OP_E2IO, RL|OP_E2IO,
 /* 40 */  BOOL|OP_E2IO, BOOL|OP_E2IO, RL|OP_A2IO, RL|OP_A2, RL|OP_OP,
 /* 45 */  RL, RL, LR|OP_OPF, LR|OP_OPF, RL|OP_A2,
@@ -1133,7 +1133,9 @@ op(int what)
 		 * Any integer mod -1 is the same as any integer mod 1
 		 * i.e. zero.
 		 */
-		if (b.u.l == -1)
+		if (c.type == MN_FLOAT)
+		    c.u.d = fmod(a.u.d, b.u.d);
+		else if (b.u.l == -1)
 		    c.u.l = 0;
 		else
 		    c.u.l = a.u.l % b.u.l;

Re: Floating point modulus

[Peter Stephenson]

On Sat, 10 Jan 2015 23:10:17 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> The following patch would instead silently substitute the fmod() call
> when using the % operator on a float.

Seems OK --- it avoids the very unusual implicit (at the shell rather
than C level) cast from float back to integer.  The only query is
whether we need to test for HAVE_FMOD, though the manual page I can see
suggests only the fmodf and fmodl variants we don't need are likely to be
problematic.

pws

Re: Floating point modulus

[Ray Andrews]

On 01/10/2015 11:10 PM, Bart Schaefer wrote:
> Consequently the operands are silently converted to integers, instead
> of the shell throwing an error.
>
> The expectation is that you will know that zsh math is C math (same
> as for why things are integer by default) and that you must do:

Expectation you say.  Wouldn't it be polite for the docs to at least
mention that? Modulo is in a list with other standard operators,
no indication whatsoever that it has different limitations. And,
given that modulo, even with floating point, is (dare I say)
near the simplest of  arithmetic operations, I'd have thought
that floating point modulo would have been near the first thing
implemented.

    mod-you-loh ()
    {
    setopt force_float
    echo "Your number is: $1"
    echo "Your modulator (what is the proper word for that?) is: $2"
    remainder=$1
    while [[ $remainder > $2 ]]; do
         let remainder=$remainder-$2
         echo "Chewing away ... $remainder"
         done
    echo "All done, the remainder is: $remainder"
    }

    $ mod-you-loh 7.5 2

    Your number is: 7.5
    Your modulator (what is the proper word for that?) is: 2
    Chewing away ... 5.5
    Chewing away ... 3.5
    Chewing away ... 1.5
    All done, the remainder is: 1.5

... even I can do it in three minutes. But it's not built in?

<anecdote>
Having zsh math brought to mind, I was thinking where I could use it
in my own  functions, and the first thing that came to mind is a place
where modulo would be just the thing.  (I am currently using
something like my pretend function above).  How nice to be able
to forget the function and have all that power in one symbol:  " % ".
Nope.  Integers only. :(
</>
>      $ zmodload zsh/mathfunc
>      $ echo $(( fmod(2.5, 2) ))
>      0.5
>
> The following patch would instead silently substitute the fmod() call
> when using the % operator on a float.  I won't commit it without some
> further discussion from the group.  With more effort it could be made
> to do this only when FORCE_FLOAT is in effect, though I'm not sure
> how many such special-cases we want to manage.
>
>
> diff --git a/Src/math.c b/Src/math.c
> index 438a170..6d096e0 100644
> --- a/Src/math.c
> +++ b/Src/math.c
> @@ -288,11 +288,11 @@ static int type[TOKCOUNT] =
>   {
>   /*  0 */  LR, LR|OP_OP|OP_OPF, RL, RL, RL|OP_OP|OP_OPF,
>   /*  5 */  RL|OP_OP|OP_OPF, RL, RL, LR|OP_A2IO, LR|OP_A2IO,
> -/* 10 */  LR|OP_A2IO, LR|OP_A2, LR|OP_A2, LR|OP_A2IO, LR|OP_A2,
> +/* 10 */  LR|OP_A2IO, LR|OP_A2, LR|OP_A2, LR|OP_A2, LR|OP_A2,
>   /* 15 */  LR|OP_A2, LR|OP_A2IO, LR|OP_A2IO, LR|OP_A2IR, LR|OP_A2IR,
>   /* 20 */  LR|OP_A2IR, LR|OP_A2IR, LR|OP_A2IR, LR|OP_A2IR, BOOL|OP_A2IO,
>   /* 25 */  BOOL|OP_A2IO, LR|OP_A2IO, RL|OP_OP, RL|OP_OP, RL|OP_E2,
> -/* 30 */  RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2IO,
> +/* 30 */  RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2,
>   /* 35 */  RL|OP_E2IO, RL|OP_E2IO, RL|OP_E2IO, RL|OP_E2IO, RL|OP_E2IO,
>   /* 40 */  BOOL|OP_E2IO, BOOL|OP_E2IO, RL|OP_A2IO, RL|OP_A2, RL|OP_OP,
>   /* 45 */  RL, RL, LR|OP_OPF, LR|OP_OPF, RL|OP_A2,

I need to go and have a little cry.



> @@ -1133,7 +1133,9 @@ op(int what)
>   		 * Any integer mod -1 is the same as any integer mod 1
>   		 * i.e. zero.
>   		 */
> -		if (b.u.l == -1)
> +		if (c.type == MN_FLOAT)
> +		    c.u.d = fmod(a.u.d, b.u.d);
> +		else if (b.u.l == -1)
>   		    c.u.l = 0;
>   		else
>   		    c.u.l = a.u.l % b.u.l;

Gentlemen.  Perhaps it's presumptuous of me to discuss this, but could a 
time not come when zsh is no longer chained to past practice?  When 
decisions are based on merit, not on what ksh '79 did? Just for 
argument's sake, supposing zsh 6.0 was the breakout version.  Everything 
in it is there on merit. Floating point math there by default!  1/2 + 
1/2 = 1! Shocking, unprecedented, revolutionary!

No rc compatibility.  Mind, it's not that good things learned from rc 
would go away, it's just that they'd be there simply because they are 
good things and they might be modified to be even better, *even* if that 
was no longer rc compatible.  Instead of having to drive forward while 
looking backwards out the rear window, imagine driving forward looking 
forward. Never forgetting the past, but not being limited by it either.

Honestly now:  how many converts do we get from csh anymore? Haven't 
most of them already come in from the cold?  I suggest that the future 
of zsh is not with welcoming in the few remaining csh long beards, but 
by making the shell more usable for brand new people.  People who expect 
1/2 + 1/2 = 1.  People familiar with electronic calculators.  "Zsh 6.0 
... the shell of the future!"  No?

Even bash converts--we might think they come over because zsh is 
compatible, but perhaps even more would come over because it *isn't* 
compatible, it's better.  Which is to say that if bash has some 
silliness, zsh would pointedly *not* have it no matter how venerable the 
silliness may be.  Zsh already does that on several points. Ah have a 
dream today.  People say the future is with Python and it's 'modern' 
cousins.  Maybe, but shells will always be there, so why don't we make a 
forward looking one?  I'm not saying that the ancient scrolls should be 
burned just that they no longer 'control' or limit or enforce 
counterproductive traditions on us.  Battle cry: " 1/2 + 1/2 = 1 ".

Re: Floating point modulus

[Bart Schaefer]

On Jan 11,  5:33pm, Peter Stephenson wrote:
}
} The only query is whether we need to test for HAVE_FMOD, though the
} manual page I can see suggests only the fmodf and fmodl variants we
} don't need are likely to be problematic.

Wikipedia seems to agree with you.  I'll commit as is and we can find
out ...

Re: Floating point modulus

[Bart Schaefer]

On Jan 10, 11:10pm, Bart Schaefer wrote:
}
} The following patch would instead silently substitute the fmod() call
} when using the % operator on a float.

Incidentally, what's a reliable test for this that could be added to
C01arith.ztst?  I worry about rounding error differing by platform:

torch% print $(( 29.1 % 13.0 ))
3.1000000000000014

Re: Floating point modulus

[Bart Schaefer]

On Jan 11, 11:25am, Ray Andrews wrote:
}
} Expectation you say.  Wouldn't it be polite for the docs to at least
} mention that?

The documentation has long been a sore spot, because it was originally
written to only cover things that were different about zsh (from other
shells) because it was expected to be the second shell adopted by an
experienced user.  There are a huge number of nooks and crannies to
fill to make it comprehensively about everything zsh in a vacuum, and
no dedicated documentation project/author.

} Perhaps it's presumptuous of me to discuss this, but could a 
} time not come when zsh is no longer chained to past practice?  When 
} decisions are based on merit, not on what ksh '79 did?

Assuming I agree (which I don't) with your implication that decisions are
not already based on merit:  so far you haven't convinced me that this
would result in anything other than a lot of arguments about "merit."

Re: Floating point modulus

[Peter Stephenson]

On Sun, 11 Jan 2015 11:36:01 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 10, 11:10pm, Bart Schaefer wrote:
> } The following patch would instead silently substitute the fmod() call
> } when using the % operator on a float.
> 
> Incidentally, what's a reliable test for this that could be added to
> C01arith.ztst?  I worry about rounding error differing by platform:
> 
> torch% print $(( 29.1 % 13.0 ))
> 3.1000000000000014

diff --git a/Test/V03mathfunc.ztst b/Test/V03mathfunc.ztst
index ab383db..8761252 100644
--- a/Test/V03mathfunc.ztst
+++ b/Test/V03mathfunc.ztst
@@ -142,3 +142,7 @@ F:This test fails if your math library doesn't have erand48().
   print $g, $l
 0:Test Gamma function gamma and lgamma
 >1.00000, 0.00000
+
+  print $(( int((29.1 % 13.0 * 10) + 0.5) )) 
+0:Test floating point modulo function
+>31

pws

Re: Floating point modulus

[Bart Schaefer]

On Jan 11,  8:01pm, Peter Stephenson wrote:
}
} diff --git a/Test/V03mathfunc.ztst b/Test/V03mathfunc.ztst
} index ab383db..8761252 100644
} --- a/Test/V03mathfunc.ztst
} +++ b/Test/V03mathfunc.ztst

Thanks, but V03mathfunc is skipped if the zsh/mathfunc module is not
loadable -- so this test should be in C01arith.ztst.

Re: Floating point modulus

[Peter Stephenson]

On Sun, 11 Jan 2015 12:04:23 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 11,  8:01pm, Peter Stephenson wrote:
> }
> } diff --git a/Test/V03mathfunc.ztst b/Test/V03mathfunc.ztst
> } index ab383db..8761252 100644
> } --- a/Test/V03mathfunc.ztst
> } +++ b/Test/V03mathfunc.ztst
> 
> Thanks, but V03mathfunc is skipped if the zsh/mathfunc module is not
> loadable -- so this test should be in C01arith.ztst.

I'll leave that to you, but instead of an explicit rounding you could do
basically the same calculation but assigned to a variable declared as an
integer and output that.xs

pws

Re: Floating point modulus

[Ray Andrews]

On 01/11/2015 12:00 PM, Bart Schaefer wrote:
> On Jan 11, 11:25am, Ray Andrews wrote:
> }
> } Expectation you say.  Wouldn't it be polite for the docs to at least
> } mention that?
>
> The documentation has long been a sore spot, because it was originally
> written to only cover things that were different about zsh (from other
> shells)

You know, that is one of those sentences you make every now and then
that turns an entire fog bank into clear air.  *now* I understand how to
understand why the doc seems so faulty ... it was/is an addendum!
And it's been patched up ever since, but never was a 'full document'
from it's beginning, so of course it's full of gaps.
> } Perhaps it's presumptuous of me to discuss this, but could a
> } time not come when zsh is no longer chained to past practice?  When
> } decisions are based on merit, not on what ksh '79 did?
>
> Assuming I agree (which I don't) with your implication that decisions are
> not already based on merit:  so far you haven't convinced me that this
> would result in anything other than a lot of arguments about "merit."
Well, no doubt there would be some judgment calls on merit, no
avoiding that.  But since I've been involved ... actually the current
issue is a small but probably perfect test case. If one ignored past
practice and was concerned with maximum utility and most intuitive
implementation, and one was designing a shell that had a built in
calculator, and one was to decide what the answer should be to this
expression:

1/2 + 1/2 =

... I think we'd all agree the answer is '1'.  Why isn't it? Because
of past practice.  Thus Google's Library of Babel is plugged up with
people asking over and over again why shell arithmetic seems to be
busted. It's busted because of past practice.  The justification for
leaving it busted is just that it has always been busted, so should
always remain busted.  Is that merit?  I'll bet anyone 50 cents
that for every person who wants 1/2 + 1/2 = 0, there are a thousand
people who want it to equal one.

Re: Floating point modulus

[Peter Stephenson]

On Sun, 11 Jan 2015 12:58:23 -0800
Ray Andrews <rayandrews@eastlink.ca> wrote:
> 1/2 + 1/2 =
> 
> ... I think we'd all agree the answer is '1'.  Why isn't it? Because
> of past practice.

This is complete nonsense and Bart already took the time to explain at
length what is going on here and why.  I'm not going to repeat it.  The
idea that "we'd all agree that..." implies (i) you don't realise that
many of the people round here have been doing some of programming, shell
or whatever, for a long time, and don't agree anything of the sort (ii)
you simply can't even be bothered to read the explanations repeatedly
given of why the world isn't the way you want it to be and why it can't
be coerced by shouting at it.

I'd strongly suggest at this point you take a couple of months out to
learn a bit about programming languages, of which zsh is an example (and
not actually a very good introduction in many ways as a bit of a hybrid)
by actually doing something with them and a good book of exmples.
Otherwise the sheer wasted time is getting to be quite ridiculous.

pws

Re: Floating point modulus

[Ray Andrews]

On 01/11/2015 12:25 PM, Peter Stephenson wrote:

> I'll leave that to you, but instead of an explicit rounding you could 
> do basically the same calculation but assigned to a variable declared 
> as an integer and output that.xs pws remainder=$1

Ignore this question if it can't be explained simply, since
I don't have the knowledge probably to even understand the
answer, but why is it more complicated than (for example) this:

while [[ $remainder > $2 ]]; do
let remainder=$remainder-$2
done

... can't that sorta be attached to  " % " without dozens of lines of
changed code?  Seems not :(

And:

  /* 25 */  BOOL|OP_A2IO, LR|OP_A2IO, RL|OP_OP, RL|OP_OP, RL|OP_E2,
-/* 30 */  RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2IO,

... are those 'metas'?  IOW, is that the internal representation of
some command syntax?   Metafied? Tokenized?  ...

>      

Re: Floating point modulus

[Ray Andrews]

On 01/11/2015 01:34 PM, Peter Stephenson wrote:
> Otherwise the sheer wasted time is getting to be quite ridiculous. pws 

As you wish sir.  I'm gone.  I'll continue to do a daily build and report
anything that seems to have broken, but I will neither ask questions
nor make comments.

Re: Floating point modulus

[Bart Schaefer]

On Jan 11,  4:02pm, Ray Andrews wrote:
} Subject: Re: Floating point modulus
}
} On 01/11/2015 12:25 PM, Peter Stephenson wrote:
} 
} > I'll leave that to you, but instead of an explicit rounding you could 
} > do basically the same calculation but assigned to a variable declared 
} > as an integer and output that.xs pws remainder=$1
} 
} Ignore this question if it can't be explained simply, since
} I don't have the knowledge probably to even understand the
} answer, but why is it more complicated

You're conflating two only loosely related things.  Above, Peter is
talking about the regression test shell code; you seem to be asking
about the C code change, which already works fine -- except that on
different hardware/OS platforms, different implementations of double-
precision arithmetic may cause the same floating-point operation to
return results that differ in the less-significant decimal places.

Since the tests rely on being able to (textually) compare an actual
result to an expected result, you can't test just any floating point
expression, you have to choose a floating point expression that will
be the same everywhere to however many decimal places.

} And:
} 
}   /* 25 */  BOOL|OP_A2IO, LR|OP_A2IO, RL|OP_OP, RL|OP_OP, RL|OP_E2,
} -/* 30 */  RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2, RL|OP_E2IO,
} 
} ... are those 'metas'?  IOW, is that the internal representation of
} some command syntax?   Metafied? Tokenized?  ...

There are two tables in math.c, one with symbolic names of all the
operators, and one with symbolic (represented as bitfield) binding and
precedence of those same operators.  The tables line up positionally.
Yes, this should probably have been done with structs instead of with
two parallel arrays, but this code was written by a college sophomore
in 1993 or so and barely changed since.

LR and RL are binding, OP_A2 and OP_E2 are assignment or expression
and the number of operands, and a suffix of IO means the operator is
"integer only".  This information is used by the math parser.  So my
patch removed the "integer only" bit from the map for % and %= and
changed the actual implemention to call fmod() for non-integers.  The
change is very small compared to the diff context produced.

-- 
Barton E. Schaefer

Re: Floating point modulus

[Bart Schaefer]

On Jan 11,  8:25pm, Peter Stephenson wrote:
}
} I'll leave that to you, but instead of an explicit rounding you could do
} basically the same calculation but assigned to a variable declared as an
} integer and output that.

Hm.  This seems like a bug:

% integer rnd
% print -- $(( rnd = ((29.1 % 13.0 * 10) + 0.5) ))
31.500000000000014

Shouldn't the result of the $(( ... )) be an integer because rnd has been
declared as an integer?

I had to do this instead:

diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index 59d182a..8da17f7 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -18,6 +18,12 @@
 0:basic floating point arithmetic
 >31415.
 
+  integer rnd
+  (( rnd = ((29.1 % 13.0 * 10) + 0.5) ))
+  print $rnd
+0:Test floating point modulo function
+>31
+
   print $(( 0x10 + 0X01 + 2#1010 ))
 0:base input
 >27

-- 
Barton E. Schaefer

Re: Floating point modulus

[Peter Stephenson]

On Sun, 11 Jan 2015 18:46:32 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 11,  8:25pm, Peter Stephenson wrote:
> }
> } I'll leave that to you, but instead of an explicit rounding you could do
> } basically the same calculation but assigned to a variable declared as an
> } integer and output that.
> 
> Hm.  This seems like a bug:
> 
> % integer rnd
> % print -- $(( rnd = ((29.1 % 13.0 * 10) + 0.5) ))
> 31.500000000000014
> 
> Shouldn't the result of the $(( ... )) be an integer because rnd has been
> declared as an integer?

The result above is how I always thought it worked, actually.  The type
of the result isn't necessarily the type of the variable assigned to;
there's never been any code to propagate types back from assignments,
unlike in C.

Whether or not it should be is another question.  Looking at the doc,
with its references to C, you'd probably be entitled to expect assignment
types propagated the way they do in C.

In particular:

% integer foo
% (( foo )); print $?
1                                     # foo is zero, status is non-zero
% (( foo = 3.5/5.5 )); print $?
0                                     # foo is zero but
                                      # status is also zero as
				      # result is non zero
% (( foo )); print $?
1                                     # different result

You might think the status at least went with the assignment.

> I had to do this instead:

That's what I was expecting you to do.

pws

Re: Floating point modulus

[Peter Stephenson]

On Sun, 11 Jan 2015 16:18:22 -0800
Ray Andrews <rayandrews@eastlink.ca> wrote:
> On 01/11/2015 01:34 PM, Peter Stephenson wrote:
> > Otherwise the sheer wasted time is getting to be quite ridiculous. pws 
> 
> As you wish sir.  I'm gone.  I'll continue to do a daily build and report
> anything that seems to have broken, but I will neither ask questions
> nor make comments.

Questions are fine, and might well point to real issues.  The problem is
what's been ensuing when the answer comes back.  I won't labour the
point, though.

pws

Re: Floating point modulus

[Vincent Lefevre]

On 2015-01-11 11:36:01 -0800, Bart Schaefer wrote:
> On Jan 10, 11:10pm, Bart Schaefer wrote:
> }
> } The following patch would instead silently substitute the fmod() call
> } when using the % operator on a float.
> 
> Incidentally, what's a reliable test for this that could be added to
> C01arith.ztst?  I worry about rounding error differing by platform:
> 
> torch% print $(( 29.1 % 13.0 ))
> 3.1000000000000014

The rounding error is due to the decimal-to-binary conversions, not to
the % operation, which, if I'm not mistaken, is necessarily exact in a
floating-point system: considering x % y, if |x| < |y|, then the result
is x, which is exact; otherwise the result is a multiple of ulp(y) and
less than |y| in absolute value, thus exactly representable.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: Floating point modulus

[Peter Stephenson]

On Mon, 12 Jan 2015 09:56:28 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Sun, 11 Jan 2015 18:46:32 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > On Jan 11,  8:25pm, Peter Stephenson wrote:
> > }
> > } I'll leave that to you, but instead of an explicit rounding you could do
> > } basically the same calculation but assigned to a variable declared as an
> > } integer and output that.
> > 
> > Hm.  This seems like a bug:
> > 
> > % integer rnd
> > % print -- $(( rnd = ((29.1 % 13.0 * 10) + 0.5) ))
> > 31.500000000000014
> > 
> > Shouldn't the result of the $(( ... )) be an integer because rnd has been
> > declared as an integer?
>
> Whether or not it should be is another question.  Looking at the doc,
> with its references to C, you'd probably be entitled to expect assignment
> types propagated the way they do in C.

This appears to be straightforward.

It looks like we were actually assuming this behaviour in one existing
test, at least in the code, but then treating the result as floating
point anway.  Duh.

I've added some notes to the README about the change.

pws

diff --git a/README b/README
index e3ccc70..977f3fa 100644
--- a/README
+++ b/README
@@ -35,6 +35,54 @@ Zsh is a shell with lots of features.  For a list of some of these, see the
 file FEATURES, and for the latest changes see NEWS.  For more
 details, see the documentation.
 
+Incompatibilites between 5.0.7 and 5.0.8
+----------------------------------------
+
+A couple of arithmetic operations have changed: the new behaviour
+is intended to be more consistent, but is not compatible with the old.
+
+Previously, the modulus operation, `%', implicitly converted the
+operation to integer and output an integer result, even if one
+or both of the arguments were floating point.  Now, the C math
+library fmod() operator is used to implement the operation where
+one of the arguments is floating point.  For example:
+
+Old behavour:
+
+% print $(( 5.5 % 2 ))
+1
+
+New behaviour:
+
+% print $(( 5.5 % 2 ))
+1.5
+
+Previously, assignments to variables assigned the correct type to
+variables declared as floating point or integer, but this type was
+not propagated to the value of the expression, as a C programmer
+would naturally expect.  Now, the type of the variable is propagated
+so long as the variable is declared as a numeric type (however this
+happened, e.g. the variable may have been implicitly typed by a
+previous assignment).  For example:
+
+Old behaviour:
+
+% integer var
+% print $(( var = 5.5 / 2.0 ))
+2.2000000000000002
+% print $var
+2
+
+(the actual rounding error may vary).
+
+New behaviour:
+
+% integer var
+% print $(( var = 5.5 / 2.0 ))
+2
+% print $var
+2
+
 Incompatibilities between 5.0.2 and 5.0.5
 -----------------------------------------
 
diff --git a/Src/math.c b/Src/math.c
index 6d096e0..db42d0f 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -880,6 +880,8 @@ getcvar(char *s)
 static mnumber
 setmathvar(struct mathvalue *mvp, mnumber v)
 {
+    Param pm;
+
     if (mvp->pval) {
 	/*
 	 * This value may have been hanging around for a while.
@@ -909,7 +911,32 @@ setmathvar(struct mathvalue *mvp, mnumber v)
     if (noeval)
 	return v;
     untokenize(mvp->lval);
-    setnparam(mvp->lval, v);
+    pm = setnparam(mvp->lval, v);
+    if (pm) {
+	/*
+	 * If we are performing an assignment, we return the
+	 * number with the same type as the parameter we are
+	 * assigning to, in the spirit of the way assignments
+	 * in C work.  Note this was a change to long-standing
+	 * zsh behaviour.
+	 */
+	switch (PM_TYPE(pm->node.flags)) {
+	case PM_INTEGER:
+	    if (v.type != MN_INTEGER) {
+		v.u.l = (zlong)v.u.d;
+		v.type = MN_INTEGER;
+	    }
+	    break;
+
+	case PM_EFLOAT:
+	case PM_FFLOAT:
+	    if (v.type != MN_FLOAT) {
+		v.u.d = (double)v.u.l;
+		v.type = MN_FLOAT;
+	    }
+	    break;
+	}
+    }
     return v;
 }
 
diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index 8da17f7..8e0730d 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -16,7 +16,7 @@
   print -- $(( rnd = there * 10000 ))
 # save rounding problems by converting to integer
 0:basic floating point arithmetic
->31415.
+>31415
 
   integer rnd
   (( rnd = ((29.1 % 13.0 * 10) + 0.5) ))
@@ -300,3 +300,11 @@
   print $(( 0b2 ))
 1:Binary numbers don't tend to have 2's in
 ?(eval):1: bad math expression: operator expected at `2 '
+
+  integer varassi
+  print $(( varassi = 5.5 / 2.0 ))
+  print $varassi
+0:Integer variable assignment converts result to integer
+>2
+>2
+# It's hard to test for integer to float.

Re: Floating point modulus

[Bart Schaefer]

On Jan 12,  9:56am, Peter Stephenson wrote:
}
} > I had to do this instead:
} 
} That's what I was expecting you to do.

I see you've already done something with this, but just to close the
thread:  I thought you were expecting me to do what was done in the
immediately preceding test [ print -- $(( rnd = ... )) ] and was
surprised when it didn't work.

Re: Floating point modulus

[Peter Stephenson]

On Mon, 12 Jan 2015 08:35:57 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 12,  9:56am, Peter Stephenson wrote:
> }
> } > I had to do this instead:
> } 
> } That's what I was expecting you to do.
> 
> I see you've already done something with this, but just to close the
> thread:  I thought you were expecting me to do what was done in the
> immediately preceding test [ print -- $(( rnd = ... )) ] and was
> surprised when it didn't work.

Yes, that's the test I noted we (probably I) had screwed up over when I
wrote the last patch, so the previous situation was indeed a bit
confused.  Should all be consistent now.

pws