Re: workers/40626 (commit 6c476c22) causes multiple test failures

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Sebastian Gniazdowski]

On 26 February 2017 at 20:42, Bart Schaefer <schaefer@brasslantern.com>
wrote:
> I suspect this is what comes of some attempt to optimize assignments.

It's impossible, hashes weren't optimized. BTW options+=( ), etc.
still segfaults. Added:

    fprintf( stderr, "Pointer of old: %p, pointer of new: %p\n",
pm->u.hash, ht );
    fflush( stderr );

to setpmoptions(). Don't know how to obtain the same pointer of which
you mention, but you can revert to 5.1.1, apply your patch to which I
reply, and see it will be still the same. Any problem is impossible
because hashes weren't optimized, and optimizations are guarded by:
v->pm->gsu.a->setfn == arrsetfn.

-- 
  Sebastian Gniazdowski
  psprint3@fastmail.com

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Bart Schaefer]

On Feb 27,  1:41am, Sebastian Gniazdowski wrote:
} Subject: Re: workers/40626 (commit 6c476c22) causes multiple test failures
}
} On 26 February 2017 at 20:42, Bart Schaefer <schaefer@brasslantern.com>
} wrote:
} > I suspect this is what comes of some attempt to optimize assignments.
} 
} It's impossible, hashes weren't optimized.

Sorry, didn't mean to imply it was the *recent* attempts.

} BTW options+=( ), etc. still segfaults.

I am not able to reproduce that.  The functions+= test in V06paramter
passes for me.  Also BEFORE my patch valgrind would report errors:

torch% options+=()
torch% options+=()
==2255== Invalid read of size 4
==2255==    at 0x80FF148: setpmoptions (parameter.c:949)
==2255==    by 0x80C06BA: arrhashsetfn (params.c:3638)
==2255==    by 0x80BDAB2: setarrvalue (params.c:2668)
==2255==    by 0x80BF2FA: assignaparam (params.c:3133)
==2255==    by 0x807C542: addvars (exec.c:2498)
==2255==    by 0x807D92E: execcmd_exec (exec.c:3004)
==2255==    by 0x807AED8: execpline2 (exec.c:1872)
==2255==    by 0x8079D42: execpline (exec.c:1602)
==2255==    by 0x8079367: execlist (exec.c:1360)
==2255==    by 0x8078B75: execode (exec.c:1141)
==2255==    by 0x809819D: loop (init.c:208)
==2255==    by 0x809BB75: zsh_main (init.c:1692)
==2255==  Address 0x29283D is not stack'd, malloc'd or (recently) free'd

But subsequent to the patch:

schaefer[1031] valgrind Src/zsh -f
==8816== Memcheck, a memory error detector.
==8816== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al.
==8816== Using LibVEX rev 1575, a library for dynamic binary translation.
==8816== Copyright (C) 2004-2005, and GNU GPL'd, by OpenWorks LLP.
==8816== Using valgrind-3.1.1, a dynamic binary instrumentation framework.
==8816== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al.
==8816== For more details, rerun with: -v
==8816== 
torch% print $options
off on off off off off off off off on off on off off off off off on off off off
on on off off off off on off off off off off off off off off off off off on off
off on off off off on on on on off off off on off off on off off on off off on
off on off off on off on off off off on off off off on off off on off off off
off off off off off on off on off off on off off off off off off off on off off
off off on off off off on off on off off on off off off off on on on off on off
on on on on off off off off on on off off on off off off off off on off off on
off off on on off off off off off on off off off off on on off on off off on
off off on off on off off off off off off off off on on off on off off off
torch% options+=()
torch% options+=()
torch% 
==8816== 
==8816== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 24 from 2)
==8816== malloc/free: in use at exit: 0 bytes in 0 blocks.
==8816== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==8816== For counts of detected errors, rerun with: -v
==8816== All heap blocks were freed -- no leaks are possible.

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Sebastian Gniazdowski]

On Mon, Feb 27, 2017, at 08:42 AM, Bart Schaefer wrote:
> On Feb 27,  1:41am, Sebastian Gniazdowski wrote:
> } On 26 February 2017 at 20:42, Bart Schaefer <schaefer@brasslantern.com>
> } wrote:
> } > I suspect this is what comes of some attempt to optimize assignments.
> } 
> } It's impossible, hashes weren't optimized.
> 
> Sorry, didn't mean to imply it was the *recent* attempts.

Ah realized that slightly after a while when thought about special
hashes and how internal "ht" is.

> } BTW options+=( ), etc. still segfaults.
> 
> I am not able to reproduce that.  The functions+= test in V06paramter
> passes for me.  Also BEFORE my patch valgrind would report errors:
> torch% options+=()
> torch% options+=()
> ==2255== Invalid read of size 4

I cannot reproduce too, I might undergo some chaos debugging, sorry.
Tried Daniel's patch, HEAD, both with and without my zrealloc hanging
around and nothing. However I can crash zsh-5.2-dev-2 that I'm normally
using. Have 3 cores in /cores from morning, their bts:

 * frame #0: 0x000000010bc02aeb zsh`createparam + 75
    frame #1: 0x000000010bc069ca zsh`arrhashsetfn + 394
    frame #2: 0x000000010bc071e7 zsh`assignaparam + 1031
    frame #3: 0x000000010bbcd14c zsh`addvars + 924
    frame #4: 0x000000010bbcf8f5 zsh`execcmd_exec + 5109

 * frame #0: 0x000000010a987d9b zsh`getarg + 2235
    frame #1: 0x000000010a98720e zsh`getindex + 286
    frame #2: 0x000000010a98954b zsh`fetchvalue + 715
    frame #3: 0x000000010a9ae897 zsh`paramsubst + 5735
    frame #4: 0x000000010a9aba81 zsh`stringsubst + 1777
    frame #5: 0x000000010a9aafa4 zsh`prefork + 180

 * frame #0: 0x00007fc86cae5530
    frame #1: 0x000000010a63020e zsh`getindex + 286
    frame #2: 0x000000010a63254b zsh`fetchvalue + 715
    frame #3: 0x000000010a657897 zsh`paramsubst + 5735
    frame #4: 0x000000010a654a81 zsh`stringsubst + 1777
    frame #5: 0x000000010a653fa4 zsh`prefork + 180

I could think it's zsh-5.2-dev-2 after all, not 5.3.1, because 5.2-dev-2
yielded now:

 * frame #0: 0x00007fdf704459f0
    frame #1: 0x000000010ed1dfde zsh-5.2-dev-2`getindex + 286
    frame #2: 0x000000010ed2031b zsh-5.2-dev-2`fetchvalue + 715
    frame #3: 0x000000010ed45ce2
    zsh-5.2-dev-2`___lldb_unnamed_function208$$zsh-5.2-dev-2 + 5442
    frame #4: 0x000000010ed43011
    zsh-5.2-dev-2`___lldb_unnamed_function204$$zsh-5.2-dev-2 + 1777
    frame #5: 0x000000010ed42534 zsh-5.2-dev-2`prefork + 180
    frame #6: 0x000000010ed432b2 zsh-5.2-dev-2`singsub + 66

However lldb "image list" from the 3 first cores shows:
[ 53] CD26917F-349B-359F-A44C-E9021703D9B5 0x000000010bf4b000
/usr/local/lib/zsh/5.3.1-dev-0/zsh/sched.so
...
[ 53] CD26917F-349B-359F-A44C-E9021703D9B5 0x000000010accf000
/usr/local/lib/zsh/5.3.1-dev-0/zsh/sched.so
...
[ 53] CD26917F-349B-359F-A44C-E9021703D9B5 0x000000010a978000
/usr/local/lib/zsh/5.3.1-dev-0/zsh/sched.so

But if valgrind doesn't report, I've must have some pre-Daniel patch
checkout, sorry again.

PS. Ah, I probably didn't do make install. Ran Src/zsh, but modules were
from /usr/local.

-- 
Sebastian Gniazdowski
psprint3@fastmail.com

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Daniel Shahaf]

Sebastian Gniazdowski wrote on Mon, Feb 27, 2017 at 10:48:12 -0800:
> PS. Ah, I probably didn't do make install. Ran Src/zsh, but modules were
> from /usr/local.

That's precisely why I compile the modules statically...

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Frank Terbeck]

Daniel Shahaf wrote:
> Sebastian Gniazdowski wrote on Mon, Feb 27, 2017 at 10:48:12 -0800:
>> PS. Ah, I probably didn't do make install. Ran Src/zsh, but modules were
>> from /usr/local.
>
> That's precisely why I compile the modules statically...

Here is something that will do the required setup to run zsh with all
its modules from its source repository:

  https://github.com/ft/zsh-test


Regards, Frank
-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Bart Schaefer]

On Feb 28,  9:51pm, Daniel Shahaf wrote:
}
} I noticed that they're equivalent, but I think it's more readable to use
} += when setting values of some options and = when setting values of all
} options.  That's because using = without specifying all options violates
} the following invariant of hashes:
} 
}     local -a a=(...)
}     local -A hash=( $a )
}     (( $#a == $#hash ))

I'm presuming you mean (( $#a == 2 * $#hash )) ... but that's not an
invariant in the first place.

    local -a a=( a 1 b 2 a 3 b 4 )
    local -A h=( $a )

Anwyay there are several special hashes that don't obey normal rules
for hashes (which is why they're special), but I will concede your
readability point.

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Daniel Shahaf]

Bart Schaefer wrote on Tue, Feb 28, 2017 at 11:04:24 -0800:
> On Feb 28,  5:03pm, Daniel Shahaf wrote:
> }
> } > So here's my question ... why would you ever attempt to append to the
> } > options parameter, empty append or otherwise?
> } 
> } To set multiple options at once:
> } 
> }     local -a options_to_set=( printexitvalue on warncreateglobal off )
> }     options+=( $options_to_set )
> 
> My point is you don't need append for that.
> 
>     options=( $options_to_set )
> 
> will do exactly the same thing, because it's not possible to delete any
> of the hash keys even if you don't appear to be assigning them.

I noticed that they're equivalent, but I think it's more readable to use
+= when setting values of some options and = when setting values of all
options.  That's because using = without specifying all options violates
the following invariant of hashes:

    local -a a=(...)
    local -A hash=( $a )
    (( $#a == $#hash ))

I suppose that's an argument in favour of turning "using = without
specifying all options on the RHS" into a runtime error. :-)

Jokes aside, backwards compatibility argues that we should make neither
case an error.

Cheers,

Daniel

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Bart Schaefer]

On Feb 28,  5:03pm, Daniel Shahaf wrote:
}
} > So here's my question ... why would you ever attempt to append to the
} > options parameter, empty append or otherwise?
} 
} To set multiple options at once:
} 
}     local -a options_to_set=( printexitvalue on warncreateglobal off )
}     options+=( $options_to_set )

My point is you don't need append for that.

    options=( $options_to_set )

will do exactly the same thing, because it's not possible to delete any
of the hash keys even if you don't appear to be assigning them.

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Daniel Shahaf]

Bart Schaefer wrote on Tue, Feb 28, 2017 at 08:38:55 -0800:
> On Feb 28,  7:45am, Daniel Shahaf wrote:
> } Subject: Re: workers/40626 (commit 6c476c22) causes multiple test failures
> }
> } Bart Schaefer wrote on Sun, Feb 26, 2017 at 11:42:16 -0800:
> } > I suspect this is what comes of some attempt to optimize assignments.
> 
> It occurs to me that $options et al. predate the += syntax, so this may
> just have been overlooked when adding array-append.
> 
> } For future reference, James (who reported the original bug offlist) has
> } since reported another symptom:
> } 
> } % autoload -Uz compinit; compinit; setopt listambiguous; options+=()
> 
> So here's my question ... why would you ever attempt to append to the
> options parameter, empty append or otherwise?

To set multiple options at once:

    local -a options_to_set=( printexitvalue on warncreateglobal off )
    options+=( $options_to_set )

I'm sure you can imagine how $options_to_set might be empty in one
codepath but not in another.

(The actual reason I used an empty append in the reproduction recipe
was for the sake of minimality, but that's somewhat tangential.)

> The options hash always
> contains all possible valid keys; you can't add/delete a key; you can't
> duplicate a key; so it absolutely never makes sense to append options.
> 
> I was half of a mind to flat out make it an error, as these are ...
> 
> torch% options[bogus]=on      
> zsh: no such option: bogus
> torch% options[shwordsplit]=
> zsh: invalid value: 
> 

The append syntax doesn't accept these either:

    % options+=( bogus on )
    zsh: no such option: bogus
    % options+=( shwordsplit '' ) 
    zsh: invalid value: 
    % 

I don't think a change is needed.

> ... until I realized that all the other zsh/parameter hashes had the
> same problem and some of them *could* sensibly be appended.

Cheers,

Daniel

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Bart Schaefer]

On Feb 28,  7:45am, Daniel Shahaf wrote:
} Subject: Re: workers/40626 (commit 6c476c22) causes multiple test failures
}
} Bart Schaefer wrote on Sun, Feb 26, 2017 at 11:42:16 -0800:
} > I suspect this is what comes of some attempt to optimize assignments.

It occurs to me that $options et al. predate the += syntax, so this may
just have been overlooked when adding array-append.

} For future reference, James (who reported the original bug offlist) has
} since reported another symptom:
} 
} % autoload -Uz compinit; compinit; setopt listambiguous; options+=()

So here's my question ... why would you ever attempt to append to the
options parameter, empty append or otherwise?  The options hash always
contains all possible valid keys; you can't add/delete a key; you can't
duplicate a key; so it absolutely never makes sense to append options.

I was half of a mind to flat out make it an error, as these are ...

torch% options[bogus]=on      
zsh: no such option: bogus
torch% options[shwordsplit]=
zsh: invalid value: 

... until I realized that all the other zsh/parameter hashes had the
same problem and some of them *could* sensibly be appended.

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Feb 26, 2017 at 11:42:16 -0800:
> On Feb 26,  6:16am, Daniel Shahaf wrote:
> } Subject: Re: workers/40626 (commit 6c476c22) causes multiple test failures
> }
> } Bart Schaefer wrote on Sat, Feb 25, 2017 at 16:04:55 -0800:
> } > This is just the first one that dies:
> } 
> } I could swear 'make check' had passed before I pushed this, but I can
> } reproduce this.  No time to debug, so reverted.  Sorry for the breakage.
> 
> workers/40508 always seemed questionable to me.
> 
> I suspect this is what comes of some attempt to optimize assignments.
> There may be cases where the recopy operation in these set*() functions
> is redundant, but I don't feel like tracking that down just now; it is
> definitely necessary for $options, so ...

Thanks for fixing this.

For future reference, James (who reported the original bug offlist) has
since reported another symptom:

% autoload -Uz compinit; compinit; setopt listambiguous; options+=()
% <accept-line>
% a<TAB>
zsh: Segmentation fault  Src/zsh -f

The latest patch fixes this one, too.

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Sebastian Gniazdowski]

On Sunday, February 26, 2017 8:42 PM, Bart Schaefer <schaefer@brasslantern.com> wrote:

> I suspect this is what comes of some attempt to optimize assignments.

It's impossible, hashes weren't optimized. BTW options+=( ), etc. still segfault. Added:

fprintf( stderr, "Pointer of old: %p, pointer of new: %p\n", pm->u.hash, ht );
fflush( stderr );

to setpmoptions(). Shows nicely what you've described in comment in code, don't know how to obtain the same pointer of which you mention, but you can revert to 5.1.1, apply above fprintf, and see it will be still the same. Any problem is impossible because hashes weren't optimized, and optimizations are guarded by: v->pm->gsu.a->setfn == arrsetfn.

-- 

Sebastian Gniazdowski
psprint@yahoo.com

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Bart Schaefer]

On Feb 26,  6:16am, Daniel Shahaf wrote:
} Subject: Re: workers/40626 (commit 6c476c22) causes multiple test failures
}
} Bart Schaefer wrote on Sat, Feb 25, 2017 at 16:04:55 -0800:
} > This is just the first one that dies:
} 
} I could swear 'make check' had passed before I pushed this, but I can
} reproduce this.  No time to debug, so reverted.  Sorry for the breakage.

workers/40508 always seemed questionable to me.

I suspect this is what comes of some attempt to optimize assignments.
There may be cases where the recopy operation in these set*() functions
is redundant, but I don't feel like tracking that down just now; it is
definitely necessary for $options, so ...


diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index c251e4f..10c47d2 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -167,7 +167,7 @@ unsetpmcommand(Param pm, UNUSED(int exp))
 
 /**/
 static void
-setpmcommands(UNUSED(Param pm), HashTable ht)
+setpmcommands(Param pm, HashTable ht)
 {
     int i;
     HashNode hn;
@@ -190,7 +190,15 @@ setpmcommands(UNUSED(Param pm), HashTable ht)
 
 	    cmdnamtab->addnode(cmdnamtab, ztrdup(hn->nam), &cn->node);
 	}
-    deleteparamtable(ht);
+    /*
+     * On full-array assignment ht is a temporary hash with the default
+     * get/set functions, whereas pm->u.hash has the special $commands
+     * get/set functions.  Do not assign ht to pm, just delete it.
+     *
+     * On append, ht and pm->u.hash are the same table, don't delete.
+     */
+    if (ht != pm->u.hash)
+	deleteparamtable(ht);
 }
 
 static const struct gsu_scalar pmcommand_gsu =
@@ -349,7 +357,9 @@ setfunctions(Param pm, HashTable ht, int dis)
 
 	    setfunction(hn->nam, ztrdup(getstrvalue(&v)), dis);
 	}
-    hashsetfn(pm, ht);
+    /* See setpmcommands() above */
+    if (ht != pm->u.hash)
+	deleteparamtable(ht);
 }
 
 /**/
@@ -937,7 +947,7 @@ unsetpmoption(Param pm, UNUSED(int exp))
 
 /**/
 static void
-setpmoptions(UNUSED(Param pm), HashTable ht)
+setpmoptions(Param pm, HashTable ht)
 {
     int i;
     HashNode hn;
@@ -962,7 +972,9 @@ setpmoptions(UNUSED(Param pm), HashTable ht)
 			      (val && strcmp(val, "off")), 0, opts))
 		zwarn("can't change option: %s", hn->nam);
 	}
-    deleteparamtable(ht);
+    /* See setpmcommands() above */
+    if (ht != pm->u.hash)
+	deleteparamtable(ht);
 }
 
 static const struct gsu_scalar pmoption_gsu =
@@ -1501,7 +1513,7 @@ unsetpmnameddir(Param pm, UNUSED(int exp))
 
 /**/
 static void
-setpmnameddirs(UNUSED(Param pm), HashTable ht)
+setpmnameddirs(Param pm, HashTable ht)
 {
     int i;
     HashNode hn, next, hd;
@@ -1543,7 +1555,9 @@ setpmnameddirs(UNUSED(Param pm), HashTable ht)
 
     i = opts[INTERACTIVE];
     opts[INTERACTIVE] = 0;
-    deleteparamtable(ht);
+    /* See setpmcommands() above */
+    if (ht != pm->u.hash)
+	deleteparamtable(ht);
     opts[INTERACTIVE] = i;
 }
 
@@ -1724,7 +1738,7 @@ unsetpmsalias(Param pm, UNUSED(int exp))
 
 /**/
 static void
-setaliases(HashTable alht, UNUSED(Param pm), HashTable ht, int flags)
+setaliases(HashTable alht, Param pm, HashTable ht, int flags)
 {
     int i;
     HashNode hn, next, hd;
@@ -1760,7 +1774,9 @@ setaliases(HashTable alht, UNUSED(Param pm), HashTable ht, int flags)
 		alht->addnode(alht, ztrdup(hn->nam),
 			      createaliasnode(ztrdup(val), flags));
 	}
-    deleteparamtable(ht);
+    /* See setpmcommands() above */
+    if (ht != pm->u.hash)
+	deleteparamtable(ht);
 }
 
 /**/

Re: workers/40626 (commit 6c476c22) causes multiple test failures

[Daniel Shahaf]

Bart Schaefer wrote on Sat, Feb 25, 2017 at 16:04:55 -0800:
> This is just the first one that dies:

I could swear 'make check' had passed before I pushed this, but I can
reproduce this.  No time to debug, so reverted.  Sorry for the breakage.

Cheers,

Daniel

workers/40626 (commit 6c476c22) causes multiple test failures

[Bart Schaefer]

This is just the first one that dies:

../../zsh-5.0/Test/A03quoting.ztst: starting.
--- /tmp/zsh.ztst.20553/ztst.out        2017-02-25 15:58:53.000000000 -0800
+++ /tmp/zsh.ztst.20553/ztst.tout       2017-02-25 15:58:53.000000000 -0800
@@ -1 +1 @@
-'
+
Test ../../zsh-5.0/Test/A03quoting.ztst failed: output differs from expected as shown above for:
  print -r ''''
  unsetopt rcquotes


**************************************
33 successful test scripts, 14 failures, 1 skipped
**************************************

Reverting 6c476c22 makes all tests pass again.