Valgrind testing, ideas

Valgrind testing, ideas

[Sebastian Gniazdowski]

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

Hello
The arrlen branch seems to faint. I had this idea of employing valgrind, to get certain about code in the branch. Currently I do this for myself, for my module, as follows:

diff --git a/module/ValTest/runtests.zsh b/module/ValTest/runtests.zsh
 integer success failure skipped retval
 for file in "${(f)ZTST_testlist}"; do
-  $ZTST_exe +Z -f $ZTST_srcdir/ztst.zsh $file
+  if (( ${+commands[colour-valgrind]} )); then
+      colour-valgrind --leak-check=full $ZTST_exe +Z -f $ZTST_srcdir/ztst.zsh $file
+  else
+      valgrind --leak-check=full $ZTST_exe +Z -f $ZTST_srcdir/ztst.zsh $file
+  fi
   retval=$?

Found 4 errors, instantly. The coloring may seem ridiculous, but the attached screenshot shows it is serious thing. It's like viewing data from XML, the data is logically divided, no need to parse with eyes.

The python module colour-valgrind should be rewritten to Zsh. With extended-glob, I see no problems with this, although it's a piece of work.

Next is the most important thing: known errors should be removed from output. Like the error from screenshot: array holding database-tied parameters should be left for OS to collect. Has anyone ideas of definition of an error, that could be used to remove blocks from valgrind output?

Also I get multiple PIDs from valgrind. I now know that single Zsh instance is used for whole test file. So the multiple PIDs are weird, not sure from where they come from.

Last thing, there should be much more test files. Single test per-file, I think. 

--
Sebastian Gniazdowski
psprint /at/ zdharma.org

[-- Attachment #2: colovalg.png --]
[-- Type: image/png, Size: 217618 bytes --]

Re: Valgrind testing, ideas

[Bart Schaefer]

On Thu, May 25, 2017 at 10:04 PM, Sebastian Gniazdowski
<psprint@zdharma.org> wrote:
>
> The arrlen branch seems to faint.

Sorry, what's the "arrlen branch"?

> Also I get multiple PIDs from valgrind.

Not all tests use the single zsh.  Some explicitly re-invoke
$ZTST_exe, others run in subshells.  In fact when I want to add
valgrind to a "make check", I insert
  local ZTST_exe=(valgrind $ZTST_exe)

> Last thing, there should be much more test files. Single test per-file, I think.

I'm not thrilled with this suggestion.  There are a lot of tests that
depend on sharing a single %prep (and corresponding %cleanup) section,
for example, and also a number of tests that depend on cascading one
after the other.  The amount of duplicated code that would result from
attempting to make these into singleton files would lead to other
problems.

Re: Valgrind testing, ideas

[Sebastian Gniazdowski]

On 26 maja 2017 at 07:50:59, Bart Schaefer (schaefer@brasslantern.com) wrote:
> On Thu, May 25, 2017 at 10:04 PM, Sebastian Gniazdowski
> wrote:
> >
> > The arrlen branch seems to faint.
> 
> Sorry, what's the "arrlen branch"?

Mikachu's badarrays branch

> Not all tests use the single zsh. Some explicitly re-invoke
> $ZTST_exe, others run in subshells. In fact when I want to add
> valgrind to a "make check", I insert
> local ZTST_exe=(valgrind $ZTST_exe)

Thanks

> > Last thing, there should be much more test files. Single test per-file, I think.
> 
> I'm not thrilled with this suggestion. There are a lot of tests that
> depend on sharing a single %prep (and corresponding %cleanup) section,
> for example, and also a number of tests that depend on cascading one
> after the other. The amount of duplicated code that would result from
> attempting to make these into singleton files would lead to other
> problems.

Yes it is problematic, I've blindly created 28 test files from redis-adapted-V11db_gdbm, and noted that they fail, because database contents is cascaded through test, I rely on some keys from previous test to exist, etc. So I run all-tests in single file too, through valgrind, currently. But if lessening of load for eyes is wanted, more automatic-pin-pointing-capable output is wanted, then single test case per file is rather the way to go.

--
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: Valgrind testing, ideas

[Daniel Shahaf]

Sebastian Gniazdowski wrote on Fri, 26 May 2017 09:57 +0200:
> But if lessening of load for eyes is wanted, more automatic-pin-pointing-capable output is wanted, then single test case per file is rather the way to go.

Just teach the test runner to only run a single test from a file.

Re: Valgrind testing, ideas

[Bart Schaefer]

On May 26, 10:56am, Daniel Shahaf wrote:
}
} Just teach the test runner to only run a single test from a file.

There are a number of tests that cascade off one another; it's no more
possible in the general case to run one test from a larger file than
to break the tests up in to separate files.  Running one test from a
file might solve the %prep/%cleanup problem, but not all the problems.

mikachu/badarrays (Re: Valgrind testing, ideas)

[Bart Schaefer]

On May 26,  7:04am, Sebastian Gniazdowski wrote:
}
} The arrlen branch seems to faint. I had this idea of employing
} valgrind, to get certain about code in the branch.
}
} Found 4 errors, instantly.

I took a stab at merging mikachu/badarrays into the current master,
and for the most part it was straightforward, but there are too many
conflicting changes to Src/params.c (different numbers of values in
the replacements for the whole suite of IPDEF macros, of which more
have been added in the meantime; bug fixes in getarrvalue() that
overlap the PM_CACHLEN changes).  I gave up on attempting to follow
what would need to be be edited to bring the latest code up to date.

I could push the current broken (won't compile) state up to the origin
(I branched my merge as schaefer/badarrays) if Mikachu is interested in
cleaning up params.c.  In the meantime it's probably not worthwhile to
attempt memory checking on mikchu/badarrays.

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Mikael Magnusson]

On Sun, May 28, 2017 at 9:43 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On May 26,  7:04am, Sebastian Gniazdowski wrote:
> }
> } The arrlen branch seems to faint. I had this idea of employing
> } valgrind, to get certain about code in the branch.
> }
> } Found 4 errors, instantly.
>
> I took a stab at merging mikachu/badarrays into the current master,
> and for the most part it was straightforward, but there are too many
> conflicting changes to Src/params.c (different numbers of values in
> the replacements for the whole suite of IPDEF macros, of which more
> have been added in the meantime; bug fixes in getarrvalue() that
> overlap the PM_CACHLEN changes).  I gave up on attempting to follow
> what would need to be be edited to bring the latest code up to date.
>
> I could push the current broken (won't compile) state up to the origin
> (I branched my merge as schaefer/badarrays) if Mikachu is interested in
> cleaning up params.c.  In the meantime it's probably not worthwhile to
> attempt memory checking on mikchu/badarrays.

Well, it never worked in the first place, that's why i put "bad" in
the name. It's basically just a documentation of the places I found
that would need to be updated to use a cached length value, but since
it doesn't work I guess I didn't find all of them, or I misunderstood
some parts of the code that I changed...

-- 
Mikael Magnusson

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Bart Schaefer]

On May 29,  3:21pm, Mikael Magnusson wrote:
}
} Well, it never worked in the first place, that's why i put "bad" in
} the name. It's basically just a documentation of the places I found
} that would need to be updated to use a cached length value, but since
} it doesn't work I guess I didn't find all of them, or I misunderstood
} some parts of the code that I changed...

It also appears we've run out of bits for PM_CACHELEN and PM_CHECKEN,
the 19 and 20 bit positions are are now occupied by PM_DONTIMPORT_SUID
and PM_SINGLE, though I suppose PM_KSHSTORED and PM_ZSHSTORED could do
double duty as they won't apply to arrays and functions at once.

With respect to "it doesn't work" -- in commits f4ab07b4 and b7c2ddf6
you say you're adding typeset -C and typeset -c to control PM_CACHELEN
and PM_CHECKLEN but looking at the diffs you only added the argument
parsing; I can't find anywhere that PM_CACHELEN or PM_CHECKLEN are ever
assigned to the parameter flags?

I believe I've now merged params.c ... I tried forcing both of the new
flags on for non-special PM_ARRAY params in createparam(), and all tests
seem to pass except those involving the $match array, which implies one
of us did something wrong in setarrvalue() for post_assignment_length.
I think.

-- 
Barton E. Schaefer

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Sebastian Gniazdowski]

On 31 maja 2017 at 07:47:59, Bart Schaefer (schaefer@brasslantern.com) wrote:
> With respect to "it doesn't work" -- in commits f4ab07b4 and b7c2ddf6
> you say you're adding typeset -C and typeset -c to control PM_CACHELEN
> and PM_CHECKLEN but looking at the diffs you only added the argument
> parsing; I can't find anywhere that PM_CACHELEN or PM_CHECKLEN are ever
> assigned to the parameter flags?
>  
> I believe I've now merged params.c ... I tried forcing both of the new
> flags on for non-special PM_ARRAY params in createparam(), and all tests
> seem to pass except those involving the $match array, which implies one
> of us did something wrong in setarrvalue() for post_assignment_length.
> I think.

I've got slightly demotivated after Mikachu said the code doesn't work, but you say you have merged this and it works?

As for my thesis on Valgrind testing, I was now running for a week the many-test-cases-in-single-test-run method, and I can now say this is a very plausible method. Valgrind messages appear, one just observes, scrolling terminal, and if a report appears, it doesn't have to be connected with distinct test case, it has its own context which is the source code and the single line pointed by Valgrind. I'm a happy Valgrind user with errors like pm->gsu.s = &stdscalar_gsu assigned instead of pm->gsu.a = &stdarray_gsu reported (interestingly this one indirectly, as memory leak). One tip maybe, I do:

if [[ 2 -nt 1 ]]; then
    colour-valgrind $ZTST_exe +Z -f $ZTST_srcdir/ztst.zsh $file
else
    colour-valgrind --leak-check=full $ZTST_exe +Z -f $ZTST_srcdir/ztst.zsh $file
fi

to easily switch by touch {1|2} between errors checking and leak checking. Actually have also file "3", to disable the "possibly lost" reports.

--
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Bart Schaefer]

On Jun 1,  6:31pm, Sebastian Gniazdowski wrote:
} Subject: Re: mikachu/badarrays (Re: Valgrind testing, ideas)
}
} I've got slightly demotivated after Mikachu said the code doesn't
} work, but you say you have merged this and it works?

I said it mostly passes tests.  There's still something screwy about
its interaction with the newer optimizations to re-use array storage
when growing/shrinking an array, which for purposes of the tests is
invoked whenever the $match array is changed by the pattern code; so
all the tests that require $match are failing.

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Sebastian Gniazdowski]

On 1 czerwca 2017 at 22:35:55, Bart Schaefer (schaefer@brasslantern.com) wrote:
> I said it mostly passes tests. There's still something screwy about
> its interaction with the newer optimizations to re-use array storage
> when growing/shrinking an array, which for purposes of the tests is
> invoked whenever the $match array is changed by the pattern code; so
> all the tests that require $match are failing.

Ah ok, mostly passes tests. I think the optimizations should agree with caching of arrlen(), the new array size is computed as before (the way it is computed didn't change), and cached array len can be stored.

I'm curious how the screen saver zmorpho would perform, as far as I remember it was ~360s with no optimizations, 40s with them, 25s with hashes used to store data under integer indices. I would test cached arrlen(), would it be 30, 20, 10 seconds, that's a puzzle, arrlen() was top function in callgrind run. Is there code available somewhere?

--
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Bart Schaefer]

On Jun 2,  3:40am, Sebastian Gniazdowski wrote:
}
} Is there code available somewhere?

I'd like there to be, but I'm having some trouble creating the branch on
the origin to which to push my local branch.  Might be that my version
of git is too old.

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Bart Schaefer]

On Jun 2,  3:40am, Sebastian Gniazdowski wrote:
}
} Is there code available somewhere?

This has been pushed to the zsh git server as schaefer/badarrays

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Sebastian Gniazdowski]

On 4 czerwca 2017 at 02:49:10, Bart Schaefer (schaefer@brasslantern.com) wrote:
> On Jun 2, 3:40am, Sebastian Gniazdowski wrote:
> }
> } Is there code available somewhere?
>  
> This has been pushed to the zsh git server as schaefer/badarrays

Tested it with the screensaver that has screen-buffer (24x105 in the test; pushed the code to https://github.com/zdharma/hacking-private/tree/master/array_tests).

It is 2 second speed up, from 10.15s to 8.01s. Expected more, but the numbers apparently just sum up this way, even though arrlen_le prevails in callgrind run (below). For hashes the speed is 7.91s, so it seems that hashes are already top-speed containers.

Badarrays:

3,143,167,205  itype_end
2,132,125,016  _platform_memmove$VARIANT$Nehalem [/usr/lib/system/libsystem_platform.dylib]
1,269,228,237  stringsubst
1,196,046,124  stringsubst'2
1,112,113,319  ingetc
  935,633,068  strcpy [/usr/lib/system/libsystem_c.dylib]
  928,176,451  strtod_l [/usr/lib/system/libsystem_c.dylib]
  877,921,877  dquote_parse

Normal (HEAD):

13,503,068,671  arrlen_le
 3,144,808,355  itype_end
 2,133,150,428  _platform_memmove$VARIANT$Nehalem [/usr/lib/system/libsystem_platform.dylib]
 1,487,072,023  arrlen
 1,268,544,885  stringsubst
 1,196,046,124  stringsubst'2
 1,112,113,371  ingetc
   935,739,112  strcpy [/usr/lib/system/libsystem_c.dylib]
   927,736,913  strtod_l [/usr/lib/system/libsystem_c.dylib]
   877,921,877  dquote_parse

-- 
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Mikael Magnusson]

On Wed, May 31, 2017 at 7:47 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On May 29,  3:21pm, Mikael Magnusson wrote:
> }
> } Well, it never worked in the first place, that's why i put "bad" in
> } the name. It's basically just a documentation of the places I found
> } that would need to be updated to use a cached length value, but since
> } it doesn't work I guess I didn't find all of them, or I misunderstood
> } some parts of the code that I changed...
>
> It also appears we've run out of bits for PM_CACHELEN and PM_CHECKEN,
> the 19 and 20 bit positions are are now occupied by PM_DONTIMPORT_SUID
> and PM_SINGLE, though I suppose PM_KSHSTORED and PM_ZSHSTORED could do
> double duty as they won't apply to arrays and functions at once.
>
> With respect to "it doesn't work" -- in commits f4ab07b4 and b7c2ddf6
> you say you're adding typeset -C and typeset -c to control PM_CACHELEN
> and PM_CHECKLEN but looking at the diffs you only added the argument
> parsing; I can't find anywhere that PM_CACHELEN or PM_CHECKLEN are ever
> assigned to the parameter flags?
>
> I believe I've now merged params.c ... I tried forcing both of the new
> flags on for non-special PM_ARRAY params in createparam(), and all tests
> seem to pass except those involving the $match array, which implies one
> of us did something wrong in setarrvalue() for post_assignment_length.
> I think.

The existing code probably does something fun like assigning these
flags based on the order of flags in the TYPESET_OPTSTR define. If you
don't use typeset -c/-C for anything, then none of the new code should
be activated and it is indeed expected that things would work.

-- 
Mikael Magnusson

Re: mikachu/badarrays (Re: Valgrind testing, ideas)

[Bart Schaefer]

On Jun 1, 11:22pm, Mikael Magnusson wrote:
} Subject: Re: mikachu/badarrays (Re: Valgrind testing, ideas)
}
} On Wed, May 31, 2017 at 7:47 AM, Bart Schaefer
} <schaefer@brasslantern.com> wrote:
} > It also appears we've run out of bits for PM_CACHELEN and PM_CHECKEN,
} > the 19 and 20 bit positions are are now occupied by PM_DONTIMPORT_SUID
} > and PM_SINGLE, though I suppose PM_KSHSTORED and PM_ZSHSTORED could do
} > double duty as they won't apply to arrays and functions at once.
} 
} The existing code probably does something fun like assigning these
} flags based on the order of flags in the TYPESET_OPTSTR define.

Indeed, that's the case.  So that means it's now essentially impossible
to add any more options to typeset, because bit positions 19+ are used
by values that must be assigned in the IPDEF(...) structs for params
used/set by the shell.  In other words adding "cC" at the end of the
TYPEST_OPTSTR is now going to be broken by other things, unless we
add some special-case code.

} If you don't use typeset -c/-C for anything, then none of the new code
} should be activated and it is indeed expected that things would work.

As I mentioned:

} > I tried forcing both of the new
} > flags on for non-special PM_ARRAY params in createparam()

So they were getting used everywhere, even without "typeset -c".  What
remained broken after that was the code that resizes an existing array.

PM_ flags (Re: mikachu/badarrays ...)

[Oliver Kiddle]

On 30 May, Bart wrote:

> It also appears we've run out of bits for PM_CACHELEN and PM_CHECKEN,
> the 19 and 20 bit positions are are now occupied by PM_DONTIMPORT_SUID
> and PM_SINGLE, though I suppose PM_KSHSTORED and PM_ZSHSTORED could do
> double duty as they won't apply to arrays and functions at once.

Should we perhaps change the flags field in struct param to be a 64-bit
value to make room for more flags? Can we portably use something like
uint64_t or is it not that simple?

In addition to more overloading of function/parameter related flags
there are also some mutually exclusive combinations. For example, only
one of the PM_TYPE flags can be set so 3 bits could be used in place
of 5 but it'd need the PM_TYPE mask adding in lots of places and only
gain us 2 bits. There's also the 32nd bit available but to reliably and
portably use that do we need to change flags to be an unsigned int?

Oliver

Re: PM_ flags (Re: mikachu/badarrays ...)

[Mikael Magnusson]

On Wed, Jun 7, 2017 at 12:53 PM, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> On 30 May, Bart wrote:
>
>> It also appears we've run out of bits for PM_CACHELEN and PM_CHECKEN,
>> the 19 and 20 bit positions are are now occupied by PM_DONTIMPORT_SUID
>> and PM_SINGLE, though I suppose PM_KSHSTORED and PM_ZSHSTORED could do
>> double duty as they won't apply to arrays and functions at once.
>
> Should we perhaps change the flags field in struct param to be a 64-bit
> value to make room for more flags? Can we portably use something like
> uint64_t or is it not that simple?
>
> In addition to more overloading of function/parameter related flags
> there are also some mutually exclusive combinations. For example, only
> one of the PM_TYPE flags can be set so 3 bits could be used in place
> of 5 but it'd need the PM_TYPE mask adding in lots of places and only
> gain us 2 bits. There's also the 32nd bit available but to reliably and
> portably use that do we need to change flags to be an unsigned int?
>
> Oliver

Just to make this clear in case it wasn't, the flags I used in that
branch were only for debugging and not needed for the operation at
all, so there's no urgent need to fix this unless someone wants to add
another flag.

-- 
Mikael Magnusson

Re: PM_ flags (Re: mikachu/badarrays ...)

[Bart Schaefer]

On Jun 7, 12:53pm, Oliver Kiddle wrote:
}
} Should we perhaps change the flags field in struct param to be a 64-bit
} value to make room for more flags? Can we portably use something like
} uint64_t or is it not that simple?

The main problem is that there is assumed to be a mapping from bit
positions to indexes in the TYPESET_OPTSTR string.  At some point we
started adding PM_FLAGS at the opt of the range to mean things other
than what could be assigned via "typeset -C" for some value of C,
and now the stack has grown down to meet the heap, so to speak.

So we'd have to renumber everything to move to a 64-bit representation,
opening up that space in the middle again.

This, by the way, is an example of why it's very difficult to build zsh
modules outside the tree that can be re-used across zsh versions; there
are lots of cases (wordcode being another example) where values that are
built into a module at time T could be dangerously wrong for a shell
compiled at T+N.