From: Kyle Ackerman <kack@kyleackerman.net>
Subject: Re: WIP Otto's Malloc Regression Integration
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Thu, 12 Dec 2024 00:55:11 +0000






On Wednesday, December 11th, 2024 at 12:09 PM, Stefan Sperling <stsp@stsp.name> wrote:

> 
> 
> On Thu, Dec 05, 2024 at 08:26:18PM +0000, Kyle Ackerman wrote:
> 
> > This is a WIP diff to give an option for regression tests.
> > 
> > Here is an example once you apply the patch
> > 
> > `cd ./regress/cmdline/ MEM_CHECK=1 ./send.sh`
> > 
> > You can set MEM_CHECK to {1,2,3,4} which will be passed down to MALLOC_OPTIONS.
> > If a regression produces a memory leak, then the kdump is left in $testroot, otherwise
> > it is deleted with $testroot. The kdump is also printed.
> > I currently only have this implemented in ./add.sh and ./send.sh. Trying to enable
> > MEM_CHECK on a regress without it being implemented will cause it to fail.
> > The current pain point with the diff below is that ktrace(1) has a return value, so
> > it may cause issues as we check got's return value in some regressions.
> 
> 
> Thanks a lot! This is a great starting point.
> 
> Below is a diff against the main branch, which I based on yours.
> There are some simplications, and I would like to separate out the
> memory leak tests from the rest, for two reasons:
> 
> 1) We don't want to carry the burden of thinking about memleaks
> testing while writing tests for other bugs. That would just get
> in the way of regular bug fixing. And this solves the problem of
> dealing with ktrace-specific exit codes since the memleak tests
> can be written to take this into account.
> 
> 2) We can run memleak checks by default when they're separated out.
> 
> Attached is a patch relative to the one you sent, showing the tweaks
> I made on top of yours. Both diffs applied together produce the diff
> below.
> 
> If you're happy with this I would commit these 2 diffs in a series and
> we can keep tweaking it in-tree, adding more memleak tests over time.
> 
> diff refs/heads/main refs/heads/memleak
> commit - 75bd1d8cea00a4cbdbdf6bcea2eb272df2fef76e
> commit + f432cdf424b63d4701af6c0170531a713e82d83c
> blob - 8625a7dfe8434206263677cae6d47f88935f4a43
> blob + ccd394f290990926dc51bb4e6ef639915cb5eeb0
> --- regress/cmdline/Makefile
> +++ regress/cmdline/Makefile
> @@ -1,7 +1,7 @@
> REGRESS_TARGETS=checkout update status log add rm diff blame branch tag \
> ref commit revert cherrypick backout rebase init import histedit \
> integrate merge stage unstage cat clone fetch send tree patch pack \
> - cleanup dump load
> + cleanup dump load memleak
> NOOBJ=Yes
> 
> GOT_TEST_ROOT=/tmp
> @@ -105,4 +105,7 @@ dump:
> load:
> ./load.sh -q -r "$(GOT_TEST_ROOT)"
> 
> +memleak:
> + ./memleak.sh -q -r "$(GOT_TEST_ROOT)"
> +
> .include <bsd.regress.mk>
> 
> blob - 156415b56edfb4ef165638477f4d1a02d4b5d86f
> blob + 772f6211b358c90a8350c2c2703196dbbc2f3c13
> --- regress/cmdline/common.sh
> +++ regress/cmdline/common.sh
> @@ -32,7 +32,11 @@ export GOT_TEST_ALGO="${GOT_TEST_ALGO:-sha1}"
> 
> export LC_ALL=C
> 
> -export MALLOC_OPTIONS=S
> +if [ -n "$MEM_CHECK" ] && [ "$MEM_CHECK" -gt 0 ] && [ "$MEM_CHECK" -lt 5 ]; then
> + export MALLOC_OPTIONS="$MEM_CHECK"S
> +else
> + export MALLOC_OPTIONS=S
> +fi
> 
> git_init()
> {
> @@ -300,3 +304,33 @@ test_done()
> echo "test failed; leaving test data in $testroot"
> fi
> }
> +
> +test_memleak_done()
> +{
> + local testroot="$1"
> + local result="$2"
> +
> + if kdump -u malloc -f $testroot/ktrace.out | grep -q " got 0x"; then
> + kdump -u malloc -f $testroot/ktrace.out
> + result=1
> + fi
> +
> + test_done "$testroot" "$result"
> +}
> +
> +check_memleak()
> +{
> + local testroot="$1"
> +
> + if [ -z "$1" ]; then
> + echo "Testroot not passed"
> + exit 1
> + fi
> +
> + if [ ! -d "$testroot" ]; then
> + echo "Directory does not exist"
> + exit 1
> + fi
> +
> + echo "ktrace -dtu -f $testroot/ktrace.out"
> +}
> blob - /dev/null
> blob + a85fc92460f40b533e431fdbb513a5cc625a94c9 (mode 755)
> --- /dev/null
> +++ regress/cmdline/memleak.sh
> @@ -0,0 +1,86 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2024 Kyle Ackerman kack@kyleackerman.net
> 
> +# Copyright (c) 2024 Stefan Sperling stsp@openbsd.org
> 
> +#
> +# Permission to use, copy, modify, and distribute this software for any
> +# purpose with or without fee is hereby granted, provided that the above
> +# copyright notice and this permission notice appear in all copies.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> +
> +export MEM_CHECK=1
> +
> +. ./common.sh
> +
> +test_memleak_add_basic() {
> + local testroot=`test_init memleak_add_basic`
> +
> + got checkout $testroot/repo $testroot/wt > /dev/null
> 
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + test_done "$testroot" "$ret"
> + return 1
> + fi
> +
> + echo "new file" > $testroot/wt/foo
> 
> +
> + echo 'A foo' > $testroot/stdout.expected
> 
> + (cd $testroot/wt && $(check_memleak $testroot) \
> + got add foo > $testroot/stdout)
> 
> +
> + test_memleak_done "$testroot" "$ret"
> +}
> +
> +test_memleak_send_basic() {
> + local testroot=`test_init send_basic`
> + local testurl=ssh://127.0.0.1/$testroot
> + local commit_id=`git_show_head $testroot/repo`
> + got clone -q $testurl/repo $testroot/repo-clone
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + echo "got clone command failed unexpectedly" >&2
> 
> + test_done "$testroot" "$ret"
> + return 1
> + fi
> + cat > $testroot/repo/.git/got.conf <<EOF
> 
> +remote "origin" {
> + protocol ssh
> + server 127.0.0.1
> + repository "$testroot/repo-clone"
> +}
> +EOF
> + got tag -r $testroot/repo -m '1.0' 1.0 >/dev/null
> 
> + tag_id=`got ref -r $testroot/repo -l | grep "^refs/tags/1.0" \\ + | tr -d ' ' | cut -d: -f2`
> +
> + echo "modified alpha" > $testroot/repo/alpha
> 
> + git -C $testroot/repo rm -q beta
> + (cd $testroot/repo && ln -s epsilon/zeta symlink)
> + git -C $testroot/repo add symlink
> + echo "new file alpha" > $testroot/repo/new
> 
> + git -C $testroot/repo add new
> + git_commit $testroot/repo -m "modified alpha"
> + local commit_id2=`git_show_head $testroot/repo`
> +
> + $(check_memleak $testroot) got send -q -r $testroot/repo \
> + > $testroot/stdout 2> $testroot/stderr
> 
> + ret=$?
> + if [ $ret -ne 0 ]; then
> + echo "got send command failed unexpectedly" >&2
> 
> + test_done "$testroot" "$ret"
> + return 1
> + fi
> +
> + test_memleak_done "$testroot" "$ret"
> +}
> +
> +test_parseargs "$@"
> +run_test test_memleak_add_basic
> +run_test test_memleak_send_basic


Looks good to me, thanks!