[PATCH 0/4] test-lib: improve LSAN + ASAN stack traces

[PATCH 0/4] test-lib: improve LSAN + ASAN stack traces

[Ævar Arnfjörð Bjarmason]

This small series provides a much needed UX improvement for
SANITIZE=leak stack traces. As noted in 4/4 this makes LSAN around 10%
slower, but in some cases the stack traces we show now are useless, so
I think it's worth it.

This also changes the stack traces to strip the absolute path to the
build directory from them. See 3/4 for how much better that looks.

This series is a result of a suggestion by Jeff King in [1], when some
of these bad stack traces (which for anyone re-reading that, I had the
wrong idea about, we just needed fast_unwind_on_malloc=0) were
discussed.

1. https://lore.kernel.org/git/YXxh%2FGMuy+sBViVY@coredump.intra.peff.net/

Ævar Arnfjörð Bjarmason (4):
  test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
  test-lib: make $GIT_BUILD_DIR an absolute path
  test-lib: add "strip_path_prefix" to XSAN_OPTIONS
  test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

 t/test-lib.sh | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

-- 
2.35.1.1031.g277d4562d2e

[PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
variables, rather than punting out entirely if we already have them in
the environment.

We do want to take any user-provided settings over our own, but we can
do do that by prepending our defaults to the variable. The
libsanitizer options parsing has "last option wins" semantics.

It's now possible to do e.g.:

    LSAN_OPTIONS=report_objects=1 ./t0006-date.sh

And not have the "report_objects=1" setting overwrite our sensible
default of "abort_on_error=1", but by prepending to the list we ensure
that:

    LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh

Will take the desired "abort_on_error=0" over our default.

See b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)
for the original pattern being altered here, and
85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,
2017-09-05) for when LSAN_OPTIONS was added in addition to the
then-existing ASAN_OPTIONS.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index e4716b0b867..3212966a82f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -36,17 +36,31 @@ then
 fi
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 
+# Prepend a string to a VAR using an arbitrary ":" delimiter, not
+# adding the delimiter if VAR is empty. I.e. a generalized:
+#
+#	VAR=$1${VAR:+$VAR}
+#
+# Usage (using ":" as a delimiter):
+#
+#	prepend_var VAR : $1
+prepend_var () {
+	eval "$1=$3\${$1:+$2\$$1}"
+}
+
+# If [AL]SAN is in effect we want to abort so that we notice problems.
+prepend_var XSAN_OPTIONS : abort_on_error=1
+
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
 # the noise level. This needs to happen at the start of the script,
 # before we even do our "did we build git yet" check (since we don't
 # want that one to complain to stderr).
-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
+prepend_var ASAN_OPTIONS : $XSAN_OPTIONS
+prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
-# If LSAN is in effect we _do_ want leak checking, but we still
-# want to abort so that we notice the problems.
-: ${LSAN_OPTIONS=abort_on_error=1}
+prepend_var LSAN_OPTIONS : $XSAN_OPTIONS
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-- 
2.35.1.1031.g277d4562d2e

[PATCH 2/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to
"/path/to/build". The "TEST_DIRECTORY" here is already made an
absolute path a few lines above this.

This will be helpful to LSAN_OPTIONS which will want to strip the
build directory path from filenames, which we couldn't do if we had a
"/.." in there.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3212966a82f..4f523b82ce5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -34,7 +34,7 @@ then
 	# elsewhere
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
-GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
 
 # Prepend a string to a VAR using an arbitrary ":" delimiter, not
 # adding the delimiter if VAR is empty. I.e. a generalized:
-- 
2.35.1.1031.g277d4562d2e

Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
> variables, rather than punting out entirely if we already have them in
> the environment.
>
> We do want to take any user-provided settings over our own, but we can
> do do that by prepending our defaults to the variable. The
> libsanitizer options parsing has "last option wins" semantics.
>
> It's now possible to do e.g.:
>
>     LSAN_OPTIONS=report_objects=1 ./t0006-date.sh
>
> And not have the "report_objects=1" setting overwrite our sensible
> default of "abort_on_error=1", but by prepending to the list we ensure
> that:
>
>     LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh
>
> Will take the desired "abort_on_error=0" over our default.

Makes sense.

> +# Prepend a string to a VAR using an arbitrary ":" delimiter, not
> +# adding the delimiter if VAR is empty. I.e. a generalized:
> +#
> +#	VAR=$1${VAR:+$VAR}

This reads: "Begin with the first parameter, and if VAR is set to a
non-empty string, append $VAR immediately after it without any
delimiter".  I would understand if it were

    VAR=$1${VAR:+":$VAR"}

(or without the first colon, allowing an empty string as a valid
member of colon-separated list).

> +# Usage (using ":" as a delimiter):
> +#
> +#	prepend_var VAR : $1
> +prepend_var () {
> +	eval "$1=$3\${$1:+$2\$$1}"

This one is correct; the above sample, when passed ":" and "VAR" to
$1 and $2, will specialize into the above example.

> +}
> +
> +# If [AL]SAN is in effect we want to abort so that we notice problems.
> +prepend_var XSAN_OPTIONS : abort_on_error=1

XSAN_OPTIONS stands for "options that are common to all ?SAN", I
guess.

>  # If we were built with ASAN, it may complain about leaks
>  # of program-lifetime variables. Disable it by default to lower
>  # the noise level. This needs to happen at the start of the script,
>  # before we even do our "did we build git yet" check (since we don't
>  # want that one to complain to stderr).
> -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
> +prepend_var ASAN_OPTIONS : $XSAN_OPTIONS
> +prepend_var ASAN_OPTIONS : detect_leaks=0

This makes me wonder if you want to generalize prepend_var even
further to notice when "$3" is an empty string.  It already avoids
spending an extra colon (and introducing an empty element in the
colon delimited list) by paying attention to the current value of
$1, so it would make sense to do the same for the incoming value.

IOW, the current prepend_var implementation relies on $XSAN_OPTIONS
and detect_leaks=0 both being non-empty string.

>  export ASAN_OPTIONS
>  
> -# If LSAN is in effect we _do_ want leak checking, but we still
> -# want to abort so that we notice the problems.
> -: ${LSAN_OPTIONS=abort_on_error=1}
> +prepend_var LSAN_OPTIONS : $XSAN_OPTIONS

But other than that, I think this step makes quite a lot of sense.

>  export LSAN_OPTIONS
>  
>  if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS

Re: [PATCH 2/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to
> "/path/to/build". The "TEST_DIRECTORY" here is already made an
> absolute path a few lines above this.
>
> This will be helpful to LSAN_OPTIONS which will want to strip the
> build directory path from filenames, which we couldn't do if we had a
> "/.." in there.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/test-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 3212966a82f..4f523b82ce5 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -34,7 +34,7 @@ then
>  	# elsewhere
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
> +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"

This makes perfect sense in the normal case, but the provision the
code that precedes this part has, i.e.

    if test -z "$TEST_DIRECTORY"
    then
            # We allow tests to override this, in case they want to run tests
            # outside of t/, e.g. for running tests on the test library
            # itself.
            TEST_DIRECTORY=$(pwd)
    else
            # ensure that TEST_DIRECTORY is an absolute path so that it
            # is valid even if the current working directory is changed
            TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
    fi

to allow TEST_DIRECTORY to be set externally robs the guarantee that
you can sensibly strip "/t" from its tail and expect everything to
work correctly.  The only thing the original requires on such an
externally given TEST_DIRECTORY is that one level above it is usable
as GIT_BUILD_DIR.

IOW,

	GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)"

would give you what you want to achieve in either code path, as long
as the original was working correctly for whatever value that is
given to TEST_DIRECTORY externally.

So, perhaps

	if test -z "$TEST_DIRECTORY"
	then
		TEST_DIRECTORY=$(pwd)
		GIT_BUILD_DIR=${TEST_DIRECTORY%/t}
	else
		TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) &&
		GIT_BUILD_DIR=$(cd "$TEST_DIRECTORY/.." && pwd)
	fi

or something like that?  I dunno.

Re: [PATCH 2/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 18 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to
>> "/path/to/build". The "TEST_DIRECTORY" here is already made an
>> absolute path a few lines above this.
>>
>> This will be helpful to LSAN_OPTIONS which will want to strip the
>> build directory path from filenames, which we couldn't do if we had a
>> "/.." in there.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/test-lib.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 3212966a82f..4f523b82ce5 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -34,7 +34,7 @@ then
>>  	# elsewhere
>>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>>  fi
>> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>> +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>
> This makes perfect sense in the normal case, but the provision the
> code that precedes this part has, i.e.
>
>     if test -z "$TEST_DIRECTORY"
>     then
>             # We allow tests to override this, in case they want to run tests
>             # outside of t/, e.g. for running tests on the test library
>             # itself.
>             TEST_DIRECTORY=$(pwd)
>     else
>             # ensure that TEST_DIRECTORY is an absolute path so that it
>             # is valid even if the current working directory is changed
>             TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
>     fi
>
> to allow TEST_DIRECTORY to be set externally robs the guarantee that
> you can sensibly strip "/t" from its tail and expect everything to
> work correctly.  The only thing the original requires on such an
> externally given TEST_DIRECTORY is that one level above it is usable
> as GIT_BUILD_DIR.
>
> IOW,
>
> 	GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)"
>
> would give you what you want to achieve in either code path, as long
> as the original was working correctly for whatever value that is
> given to TEST_DIRECTORY externally.
>
> So, perhaps
>
> 	if test -z "$TEST_DIRECTORY"
> 	then
> 		TEST_DIRECTORY=$(pwd)
> 		GIT_BUILD_DIR=${TEST_DIRECTORY%/t}
> 	else
> 		TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) &&
> 		GIT_BUILD_DIR=$(cd "$TEST_DIRECTORY/.." && pwd)
> 	fi
>
> or something like that?  I dunno.

I think you're being led astray by the "we allow tests to override this"
comment, which is something I added in 62f539043c7 (test-lib: Allow
overriding of TEST_DIRECTORY, 2010-08-19). I'm having some trouble
understanding what I meant at the time.

But it's not the case that we support a TEST_DIRECTORY pointing to
something that isn't inside the "t/" directory in our tree, as looking
at uses of the two shows:
    
    $ git grep -E '\$(GIT_BUILD_DIR|TEST_DIRECTORY)' -- t/test-lib.sh
    t/test-lib.sh:if test -z "$TEST_DIRECTORY"
    t/test-lib.sh:  TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
    t/test-lib.sh:  TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
    t/test-lib.sh:GIT_BUILD_DIR="$TEST_DIRECTORY"/..
    t/test-lib.sh:if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
    t/test-lib.sh:. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
    t/test-lib.sh:"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
    t/test-lib.sh:. "$TEST_DIRECTORY/test-lib-functions.sh"
    t/test-lib.sh:                  $(printf '%s\n' "$1" | sed -f "$GIT_BUILD_DIR/t/chainlint.sed" | grep -q '?![A-Z][A-Z]*?!')
    t/test-lib.sh:                  symlink_target="$GIT_BUILD_DIR/t/helper/$base"
    t/test-lib.sh:                  symlink_target="$GIT_BUILD_DIR/$base"
    t/test-lib.sh:  GIT_VALGRIND=$TEST_DIRECTORY/valgrind
    t/test-lib.sh:  for file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/t/helper/test-*
    t/test-lib.sh:  make_symlink "$GIT_BUILD_DIR"/mergetools "$GIT_VALGRIND/bin/mergetools"
    t/test-lib.sh:  PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
    t/test-lib.sh:          git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
    t/test-lib.sh:  GIT_EXEC_PATH=$GIT_BUILD_DIR
    t/test-lib.sh:          PATH="$GIT_BUILD_DIR:$GIT_BUILD_DIR/t/helper:$PATH"
    t/test-lib.sh:GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
    t/test-lib.sh:GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
    t/test-lib.sh:test -d "$GIT_BUILD_DIR"/templates/blt || {
    t/test-lib.sh:if ! test -x "$GIT_BUILD_DIR"/t/helper/test-tool$X

I.e. we already depend on the build dir being one-above the
TEST_DIRECTORY, and per test-lib.sh we load test-lib-functions.sh from
$TEST_DIRECTORY, and refer to $GIT_BUILD_DIR/t/... for various things
(e.g. t/helper).

That being said it was a bit of a micro-optimization of mine to avoid a
"&& pwd" there, and perhaps it was too clever.

But I do think we can 100% rely on just stripping the "/t" from the end
of the string.

Re-reading it again, what that comment really meant to say is that we
allow overriding TEST_DIRECTORY from $(pwd) because when we run that
test-lib.sh we may be in another directory.

But that overriding code still sets us to the same directory, which ends
in a "/t". See my subsequent 7b905119703 (t/t0000-basic.sh: Run the
passing TODO test inside its own test-lib, 2010-08-19) for the first use
of it.

I.e. it's for the t0000-basic.sh testing where we run sub-tests, which
now live in t/lib-subtest.sh.

So I could keep that code as-is for a re-roll, but adjust the misleading
comment while I'm at it, how does that sound?

Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Taylor Blau]

On Fri, Feb 18, 2022 at 03:20:41PM -0800, Junio C Hamano wrote:
> > +# Usage (using ":" as a delimiter):
> > +#
> > +#	prepend_var VAR : $1
> > +prepend_var () {
> > +	eval "$1=$3\${$1:+$2\$$1}"
>
> This one is correct; the above sample, when passed ":" and "VAR" to
> $1 and $2, will specialize into the above example.

> > +}
> > +
> > +# If [AL]SAN is in effect we want to abort so that we notice problems.
> > +prepend_var XSAN_OPTIONS : abort_on_error=1
>
> XSAN_OPTIONS stands for "options that are common to all ?SAN", I
> guess.

I was also unclear on this. Looking around in the google/sanitizers
repository, I don't see something called "XSAN_OPTIONS" mentioned
anywhere (neither in documentation nor in the actual source code).

Is this a convenience variable that we use to store options that are
common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the
extra confusion is worth it, since it only contains abort_on_error=1.

I guess it makes it (along with the prepend_var function introduced by
this patch) possible for a caller to write XSAN_OPTIONS=... into their
environment and then run a test script and have their settings feed into
ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find
this super useful (or, at least not dramatically more convenient than
setting both variables).

I could be wrong, and I'm obviously biased towards my own usage of the
ASAN/LSAN builds, but to me this patch might be clearer without the
extra variable.

> >  # If we were built with ASAN, it may complain about leaks
> >  # of program-lifetime variables. Disable it by default to lower
> >  # the noise level. This needs to happen at the start of the script,
> >  # before we even do our "did we build git yet" check (since we don't
> >  # want that one to complain to stderr).
> > -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
> > +prepend_var ASAN_OPTIONS : $XSAN_OPTIONS
> > +prepend_var ASAN_OPTIONS : detect_leaks=0
>
> This makes me wonder if you want to generalize prepend_var even
> further to notice when "$3" is an empty string.  It already avoids
> spending an extra colon (and introducing an empty element in the
> colon delimited list) by paying attention to the current value of
> $1, so it would make sense to do the same for the incoming value.
>
> IOW, the current prepend_var implementation relies on $XSAN_OPTIONS
> and detect_leaks=0 both being non-empty string.

Yes, agreed.

Thanks,
Taylor

Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 18 2022, Taylor Blau wrote:

> On Fri, Feb 18, 2022 at 03:20:41PM -0800, Junio C Hamano wrote:
>> > +# Usage (using ":" as a delimiter):
>> > +#
>> > +#	prepend_var VAR : $1
>> > +prepend_var () {
>> > +	eval "$1=$3\${$1:+$2\$$1}"
>>
>> This one is correct; the above sample, when passed ":" and "VAR" to
>> $1 and $2, will specialize into the above example.
>
>> > +}
>> > +
>> > +# If [AL]SAN is in effect we want to abort so that we notice problems.
>> > +prepend_var XSAN_OPTIONS : abort_on_error=1
>>
>> XSAN_OPTIONS stands for "options that are common to all ?SAN", I
>> guess.
>
> I was also unclear on this. Looking around in the google/sanitizers
> repository, I don't see something called "XSAN_OPTIONS" mentioned
> anywhere (neither in documentation nor in the actual source code).
>
> Is this a convenience variable that we use to store options that are
> common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the
> extra confusion is worth it, since it only contains abort_on_error=1.
>
> I guess it makes it (along with the prepend_var function introduced by
> this patch) possible for a caller to write XSAN_OPTIONS=... into their
> environment and then run a test script and have their settings feed into
> ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find
> this super useful (or, at least not dramatically more convenient than
> setting both variables).
>
> I could be wrong, and I'm obviously biased towards my own usage of the
> ASAN/LSAN builds, but to me this patch might be clearer without the
> extra variable.

Sorry, yes it's just a git.git invention to refer to "common LSAN and
ASAN' things. Perhaps GIT_ASAN_AND_ASAN_COMMON or something would be
much less confusing & better? Bikeshedding most welcome :)

Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Taylor Blau]

On Sat, Feb 19, 2022 at 03:48:42AM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> > +}
> >> > +
> >> > +# If [AL]SAN is in effect we want to abort so that we notice problems.
> >> > +prepend_var XSAN_OPTIONS : abort_on_error=1
> >>
> >> XSAN_OPTIONS stands for "options that are common to all ?SAN", I
> >> guess.
> >
> > I was also unclear on this. Looking around in the google/sanitizers
> > repository, I don't see something called "XSAN_OPTIONS" mentioned
> > anywhere (neither in documentation nor in the actual source code).
> >
> > Is this a convenience variable that we use to store options that are
> > common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the
> > extra confusion is worth it, since it only contains abort_on_error=1.
> >
> > I guess it makes it (along with the prepend_var function introduced by
> > this patch) possible for a caller to write XSAN_OPTIONS=... into their
> > environment and then run a test script and have their settings feed into
> > ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find
> > this super useful (or, at least not dramatically more convenient than
> > setting both variables).
> >
> > I could be wrong, and I'm obviously biased towards my own usage of the
> > ASAN/LSAN builds, but to me this patch might be clearer without the
> > extra variable.
>
> Sorry, yes it's just a git.git invention to refer to "common LSAN and
> ASAN' things. Perhaps GIT_ASAN_AND_ASAN_COMMON or something would be
> much less confusing & better? Bikeshedding most welcome :)

Yeah, I would be fine with something like GIT_ASAN_AND_LSAN_COMMON if it
makes things easier. I think it definitely is clearer, and does make it
easier to add new options to both.

I probably wouldn't ever tweak its value myself, so I don't think I have
a strong opinion here. If it were me, I'd probably just as soon
duplicate setting `abort_on_error=1` in the ASan- and LSan-specific
variables. But I don't have strong feelings here, so whatever makes the
most sense to you is fine with me.

Thanks,
Taylor

Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 18 2022, Taylor Blau wrote:

> On Sat, Feb 19, 2022 at 03:48:42AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> > +}
>> >> > +
>> >> > +# If [AL]SAN is in effect we want to abort so that we notice problems.
>> >> > +prepend_var XSAN_OPTIONS : abort_on_error=1
>> >>
>> >> XSAN_OPTIONS stands for "options that are common to all ?SAN", I
>> >> guess.
>> >
>> > I was also unclear on this. Looking around in the google/sanitizers
>> > repository, I don't see something called "XSAN_OPTIONS" mentioned
>> > anywhere (neither in documentation nor in the actual source code).
>> >
>> > Is this a convenience variable that we use to store options that are
>> > common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the
>> > extra confusion is worth it, since it only contains abort_on_error=1.
>> >
>> > I guess it makes it (along with the prepend_var function introduced by
>> > this patch) possible for a caller to write XSAN_OPTIONS=... into their
>> > environment and then run a test script and have their settings feed into
>> > ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find
>> > this super useful (or, at least not dramatically more convenient than
>> > setting both variables).
>> >
>> > I could be wrong, and I'm obviously biased towards my own usage of the
>> > ASAN/LSAN builds, but to me this patch might be clearer without the
>> > extra variable.
>>
>> Sorry, yes it's just a git.git invention to refer to "common LSAN and
>> ASAN' things. Perhaps GIT_ASAN_AND_ASAN_COMMON or something would be
>> much less confusing & better? Bikeshedding most welcome :)
>
> Yeah, I would be fine with something like GIT_ASAN_AND_LSAN_COMMON if it
> makes things easier. I think it definitely is clearer, and does make it
> easier to add new options to both.
>
> I probably wouldn't ever tweak its value myself, so I don't think I have
> a strong opinion here. If it were me, I'd probably just as soon
> duplicate setting `abort_on_error=1` in the ASan- and LSan-specific
> variables. But I don't have strong feelings here, so whatever makes the
> most sense to you is fine with me.

Once you get to tweaking some more advanced options it makes sense to
share a lot for both. One change I have locally on top of this is e.g.:

    prepend_var XSAN_OPTIONS : dedup_token_length=9999
    prepend_var XSAN_OPTIONS : log_exe_name=1
    prepend_var XSAN_OPTIONS : log_path=\"$TEST_RESULTS_XSAN_FILE\"

I.e. for a feature to log machine-readable stacktraces when you run the
test suite with LSAN or ASAN.

    prepend_var XSAN_OPTIONS : dedup_token_length=9999
    prepend_var XSAN_OPTIONS : log_exe_name=1
    prepend_var XSAN_OPTIONS : log_path=\"$TEST_RESULTS_XSAN_FILE\"

Which is how I ended up with XSAN_OPTIONS I think, i.e. I'd test things
with LSAN, switch to ASAN, would have to rename the variable
etc. Changing a single letter was easier ... :)

Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Taylor Blau]

On Sat, Feb 19, 2022 at 04:02:23AM +0100, Ævar Arnfjörð Bjarmason wrote:
> Once you get to tweaking some more advanced options it makes sense to
> share a lot for both. One change I have locally on top of this is e.g.:

Makes sense. I run the tests with sanitization quite often, but rarely
use any fanciful options like you.

Thanks,
Taylor

[PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces

[Ævar Arnfjörð Bjarmason]

A UX improvement for LSAN stack traces. See
https://lore.kernel.org/git/cover-0.4-00000000000-20220218T205753Z-avarab@gmail.com/
for the v1.

I think this v2 should address all the comments on the v1, thanks
Taylor & Junio!

Changes:
 * Renamed XSAN_OPTIONS to GIT_XSAN_OPTIONS
 * The "prepend_var" now handles an empty $3, as suggested by Junio.
 * I added a new 2/4 updating the $TEST_DIRECTORY commentary to note
   that we depend on it pointing to *the* "t" directory.

Ævar Arnfjörð Bjarmason (4):
  test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
  test-lib: correct commentary on TEST_DIRECTORY overriding
  test-lib: make $GIT_BUILD_DIR an absolute path
  test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

 t/test-lib.sh | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

Range-diff against v1:
1:  75c8f7a719c ! 1:  01e63a72231 test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
    +    test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
     
         Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
         variables, rather than punting out entirely if we already have them in
    @@ t/test-lib.sh: then
      GIT_BUILD_DIR="$TEST_DIRECTORY"/..
      
     +# Prepend a string to a VAR using an arbitrary ":" delimiter, not
    -+# adding the delimiter if VAR is empty. I.e. a generalized:
    ++# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
     +#
    -+#	VAR=$1${VAR:+$VAR}
    ++#	VAR=$1${VAR:+${1:+$2}$VAR}
     +#
    -+# Usage (using ":" as a delimiter):
    ++# Usage (using ":" as the $2 delimiter):
     +#
    -+#	prepend_var VAR : $1
    ++#	prepend_var VAR : VALUE
     +prepend_var () {
    -+	eval "$1=$3\${$1:+$2\$$1}"
    ++	eval "$1=$3\${$1:+${3:+$2}\$$1}"
     +}
     +
    -+# If [AL]SAN is in effect we want to abort so that we notice problems.
    -+prepend_var XSAN_OPTIONS : abort_on_error=1
    ++# If [AL]SAN is in effect we want to abort so that we notice
    ++# problems. The GIT_XSAN_OPTIONS variable can be used to set common
    ++# defaults shared between [AL]SAN_OPTIONS.
    ++prepend_var GIT_XSAN_OPTIONS : abort_on_error=1
     +
      # If we were built with ASAN, it may complain about leaks
      # of program-lifetime variables. Disable it by default to lower
    @@ t/test-lib.sh: then
      # before we even do our "did we build git yet" check (since we don't
      # want that one to complain to stderr).
     -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
    -+prepend_var ASAN_OPTIONS : $XSAN_OPTIONS
    ++prepend_var ASAN_OPTIONS : $GIT_XSAN_OPTIONS
     +prepend_var ASAN_OPTIONS : detect_leaks=0
      export ASAN_OPTIONS
      
     -# If LSAN is in effect we _do_ want leak checking, but we still
     -# want to abort so that we notice the problems.
     -: ${LSAN_OPTIONS=abort_on_error=1}
    -+prepend_var LSAN_OPTIONS : $XSAN_OPTIONS
    ++prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS
      export LSAN_OPTIONS
      
      if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-:  ----------- > 2:  0c2867e30dc test-lib: correct commentary on TEST_DIRECTORY overriding
2:  4c53c6157ac ! 3:  229654027b8 test-lib: make $GIT_BUILD_DIR an absolute path
    @@ Commit message
         "/path/to/build". The "TEST_DIRECTORY" here is already made an
         absolute path a few lines above this.
     
    -    This will be helpful to LSAN_OPTIONS which will want to strip the
    -    build directory path from filenames, which we couldn't do if we had a
    -    "/.." in there.
    +    We could simply do $(cd "$TEST_DIRECTORY"/.." && pwd) here, but as
    +    noted in the preceding commit the "$TEST_DIRECTORY" can't be anything
    +    except the path containing this test-lib.sh file at this point, so we
    +    can more cheaply and equally strip the "/t" off the end.
    +
    +    This change will be helpful to LSAN_OPTIONS which will want to strip
    +    the build directory path from filenames, which we couldn't do if we
    +    had a "/.." in there.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/test-lib.sh: then
     +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
      
      # Prepend a string to a VAR using an arbitrary ":" delimiter, not
    - # adding the delimiter if VAR is empty. I.e. a generalized:
    + # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
    +@@ t/test-lib.sh: prepend_var () {
    + # problems. The GIT_XSAN_OPTIONS variable can be used to set common
    + # defaults shared between [AL]SAN_OPTIONS.
    + prepend_var GIT_XSAN_OPTIONS : abort_on_error=1
    ++prepend_var GIT_XSAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"
    + 
    + # If we were built with ASAN, it may complain about leaks
    + # of program-lifetime variables. Disable it by default to lower
3:  ec852e6d691 < -:  ----------- test-lib: add "strip_path_prefix" to XSAN_OPTIONS
4:  2becf76a14a ! 4:  e6a48b6e4ce test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
    @@ t/test-lib.sh
     @@ t/test-lib.sh: prepend_var ASAN_OPTIONS : detect_leaks=0
      export ASAN_OPTIONS
      
    - prepend_var LSAN_OPTIONS : $XSAN_OPTIONS
    + prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS
     +prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
      export LSAN_OPTIONS
      
-- 
2.35.1.1130.g7c6dd716f26

[PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
variables, rather than punting out entirely if we already have them in
the environment.

We do want to take any user-provided settings over our own, but we can
do do that by prepending our defaults to the variable. The
libsanitizer options parsing has "last option wins" semantics.

It's now possible to do e.g.:

    LSAN_OPTIONS=report_objects=1 ./t0006-date.sh

And not have the "report_objects=1" setting overwrite our sensible
default of "abort_on_error=1", but by prepending to the list we ensure
that:

    LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh

Will take the desired "abort_on_error=0" over our default.

See b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)
for the original pattern being altered here, and
85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,
2017-09-05) for when LSAN_OPTIONS was added in addition to the
then-existing ASAN_OPTIONS.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index e4716b0b867..7e6978d1817 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -36,17 +36,33 @@ then
 fi
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 
+# Prepend a string to a VAR using an arbitrary ":" delimiter, not
+# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
+#
+#	VAR=$1${VAR:+${1:+$2}$VAR}
+#
+# Usage (using ":" as the $2 delimiter):
+#
+#	prepend_var VAR : VALUE
+prepend_var () {
+	eval "$1=$3\${$1:+${3:+$2}\$$1}"
+}
+
+# If [AL]SAN is in effect we want to abort so that we notice
+# problems. The GIT_XSAN_OPTIONS variable can be used to set common
+# defaults shared between [AL]SAN_OPTIONS.
+prepend_var GIT_XSAN_OPTIONS : abort_on_error=1
+
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
 # the noise level. This needs to happen at the start of the script,
 # before we even do our "did we build git yet" check (since we don't
 # want that one to complain to stderr).
-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
+prepend_var ASAN_OPTIONS : $GIT_XSAN_OPTIONS
+prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
-# If LSAN is in effect we _do_ want leak checking, but we still
-# want to abort so that we notice the problems.
-: ${LSAN_OPTIONS=abort_on_error=1}
+prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-- 
2.35.1.1130.g7c6dd716f26

[PATCH v2 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding

[Ævar Arnfjörð Bjarmason]

Correct a misleading comment added by me in 62f539043c7 (test-lib:
Allow overriding of TEST_DIRECTORY, 2010-08-19).

Between that comment and the later addition of
85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,
2013-11-17) the comments were on the wrong arms of the "if". I.e. the
"allow tests to override this" was on the "test -z" arm.

But more importantly this could be read allowing the "$TEST_DIRECTORY"
to be some path outside of t/. As explained in the updated comment
that's impossible, rather it was meant for *tests* that ran outside of
t/, i.e. the "t0000-basic.sh" tests that use "lib-subtest.sh".

Those tests have a different working directory, but they set the
"TEST_DIRECTORY" to the same path for bootstrapping. The comments now
reflect that, and further comment on why we have a hard dependency on
this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7e6978d1817..8fa7379e128 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -19,13 +19,20 @@
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 if test -z "$TEST_DIRECTORY"
 then
-	# We allow tests to override this, in case they want to run tests
-	# outside of t/, e.g. for running tests on the test library
-	# itself.
-	TEST_DIRECTORY=$(pwd)
-else
 	# ensure that TEST_DIRECTORY is an absolute path so that it
 	# is valid even if the current working directory is changed
+	TEST_DIRECTORY=$(pwd)
+else
+	# The TEST_DIRECTORY will always be the path to the "t"
+	# directory in the git.git checkout. This is overridden by
+	# e.g. t/lib-subtest.sh, but only because its $(pwd) is
+	# different. Those tests still set "$TEST_DIRECTORY" to the
+	# same path.
+	#
+	# See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for
+	# hard assumptions about "$GIT_BUILD_DIR/t" existing and being
+	# the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper"
+	# needing to exist.
 	TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
 fi
 if test -z "$TEST_OUTPUT_DIRECTORY"
-- 
2.35.1.1130.g7c6dd716f26

[PATCH v2 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to
"/path/to/build". The "TEST_DIRECTORY" here is already made an
absolute path a few lines above this.

We could simply do $(cd "$TEST_DIRECTORY"/.." && pwd) here, but as
noted in the preceding commit the "$TEST_DIRECTORY" can't be anything
except the path containing this test-lib.sh file at this point, so we
can more cheaply and equally strip the "/t" off the end.

This change will be helpful to LSAN_OPTIONS which will want to strip
the build directory path from filenames, which we couldn't do if we
had a "/.." in there.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8fa7379e128..80944035f2c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -41,7 +41,7 @@ then
 	# elsewhere
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
-GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
 
 # Prepend a string to a VAR using an arbitrary ":" delimiter, not
 # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
@@ -59,6 +59,7 @@ prepend_var () {
 # problems. The GIT_XSAN_OPTIONS variable can be used to set common
 # defaults shared between [AL]SAN_OPTIONS.
 prepend_var GIT_XSAN_OPTIONS : abort_on_error=1
+prepend_var GIT_XSAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"
 
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
-- 
2.35.1.1130.g7c6dd716f26

[PATCH v2 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":

    $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
        #1 0x9444a4 in xstrdup wrapper.c:29:14
        #2 0x5995fa in parse_date_format date.c:991:24
        #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
        #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
        #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
        #6 0x4cd1e3 in main common-main.c:52:11
        #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #8 0x422b09 in _start (t/helper/test-tool+0x422b09)

    SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Whereas LSAN would emit this instead:

    $ ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f2be1d614aa in strdup string/strdup.c:42:15

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:

    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
        #2 0x5cb164 in xstrdup wrapper.c:29:14
        #3 0x495ee9 in parse_date_format date.c:991:24
        #4 0x453aac in show_dates t/helper/test-date.c:39:2
        #5 0x453782 in cmd__date t/helper/test-date.c:116:3
        #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
        #7 0x451f1e in main common-main.c:52:11
        #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:

    $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
    Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
      Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
      Range (min … max):    2.122 s …  2.152 s    3 runs

    Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
      Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
      Range (min … max):    1.941 s …  2.044 s    3 runs

    Summary
      'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
        1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'

I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 80944035f2c..129668c685f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -71,6 +71,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
 prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS
+prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-- 
2.35.1.1130.g7c6dd716f26

Re: [PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
> variables, rather than punting out entirely if we already have them in
> the environment.
>
> We do want to take any user-provided settings over our own, but we can
> do do that by prepending our defaults to the variable. The

do do do?

> libsanitizer options parsing has "last option wins" semantics.
> ...
> +# If [AL]SAN is in effect we want to abort so that we notice
> +# problems. The GIT_XSAN_OPTIONS variable can be used to set common

As we say GIT_ prefix, we can just drop X.  "Git's sanitizer options".

Re: [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces

[Taylor Blau]

On Sat, Feb 19, 2022 at 12:29:39PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I think this v2 should address all the comments on the v1, thanks
> Taylor & Junio!

Thanks; this version looks good to me, though I agree with Junio's
feedback on the first patch. TBH, I do not feel strongly at all about
"GIT_XSAN_OPTIONS" versus "GIT_SAN_OPTIONS", either seems fine to (with
a slight bias towards the first, since it makes it clear that it targets
both ASan and LSan).

But either way, this looks like it's almost there.

Thanks,
Taylor

[PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces

[Ævar Arnfjörð Bjarmason]

A UX improvement for [AL]SAN stack traces. See
https://lore.kernel.org/git/cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com/
for the v2.

Changes:
 * Renamed GIT_XSAN_OPTIONS to GIT_SAN_OPTIONS per Junio's suggestion
 * Fixed grammar ("do do do...")" in commit message.

Ævar Arnfjörð Bjarmason (4):
  test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS
  test-lib: correct commentary on TEST_DIRECTORY overriding
  test-lib: make $GIT_BUILD_DIR an absolute path
  test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

 t/test-lib.sh | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

Range-diff against v2:
1:  01e63a72231 ! 1:  bf31efca464 test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
    +    test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS
     
         Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
         variables, rather than punting out entirely if we already have them in
         the environment.
     
    -    We do want to take any user-provided settings over our own, but we can
    -    do do that by prepending our defaults to the variable. The
    -    libsanitizer options parsing has "last option wins" semantics.
    +    We want to take any user-provided settings over our own, but we can do
    +    that by prepending our defaults to the variable. The libsanitizer
    +    options parsing has "last option wins" semantics.
     
         It's now possible to do e.g.:
     
    @@ t/test-lib.sh: then
     +}
     +
     +# If [AL]SAN is in effect we want to abort so that we notice
    -+# problems. The GIT_XSAN_OPTIONS variable can be used to set common
    ++# problems. The GIT_SAN_OPTIONS variable can be used to set common
     +# defaults shared between [AL]SAN_OPTIONS.
    -+prepend_var GIT_XSAN_OPTIONS : abort_on_error=1
    ++prepend_var GIT_SAN_OPTIONS : abort_on_error=1
     +
      # If we were built with ASAN, it may complain about leaks
      # of program-lifetime variables. Disable it by default to lower
    @@ t/test-lib.sh: then
      # before we even do our "did we build git yet" check (since we don't
      # want that one to complain to stderr).
     -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
    -+prepend_var ASAN_OPTIONS : $GIT_XSAN_OPTIONS
    ++prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS
     +prepend_var ASAN_OPTIONS : detect_leaks=0
      export ASAN_OPTIONS
      
     -# If LSAN is in effect we _do_ want leak checking, but we still
     -# want to abort so that we notice the problems.
     -: ${LSAN_OPTIONS=abort_on_error=1}
    -+prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS
    ++prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
      export LSAN_OPTIONS
      
      if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
2:  0c2867e30dc = 2:  33a628e9c3a test-lib: correct commentary on TEST_DIRECTORY overriding
3:  229654027b8 ! 3:  b03ae29fc92 test-lib: make $GIT_BUILD_DIR an absolute path
    @@ t/test-lib.sh: then
      # Prepend a string to a VAR using an arbitrary ":" delimiter, not
      # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
     @@ t/test-lib.sh: prepend_var () {
    - # problems. The GIT_XSAN_OPTIONS variable can be used to set common
    + # problems. The GIT_SAN_OPTIONS variable can be used to set common
      # defaults shared between [AL]SAN_OPTIONS.
    - prepend_var GIT_XSAN_OPTIONS : abort_on_error=1
    -+prepend_var GIT_XSAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"
    + prepend_var GIT_SAN_OPTIONS : abort_on_error=1
    ++prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"
      
      # If we were built with ASAN, it may complain about leaks
      # of program-lifetime variables. Disable it by default to lower
4:  e6a48b6e4ce ! 4:  d212009e517 test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
    @@ t/test-lib.sh
     @@ t/test-lib.sh: prepend_var ASAN_OPTIONS : detect_leaks=0
      export ASAN_OPTIONS
      
    - prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS
    + prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
     +prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
      export LSAN_OPTIONS
      
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v3 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
variables, rather than punting out entirely if we already have them in
the environment.

We want to take any user-provided settings over our own, but we can do
that by prepending our defaults to the variable. The libsanitizer
options parsing has "last option wins" semantics.

It's now possible to do e.g.:

    LSAN_OPTIONS=report_objects=1 ./t0006-date.sh

And not have the "report_objects=1" setting overwrite our sensible
default of "abort_on_error=1", but by prepending to the list we ensure
that:

    LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh

Will take the desired "abort_on_error=0" over our default.

See b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)
for the original pattern being altered here, and
85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,
2017-09-05) for when LSAN_OPTIONS was added in addition to the
then-existing ASAN_OPTIONS.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index e4716b0b867..55f263a02d3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -36,17 +36,33 @@ then
 fi
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 
+# Prepend a string to a VAR using an arbitrary ":" delimiter, not
+# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
+#
+#	VAR=$1${VAR:+${1:+$2}$VAR}
+#
+# Usage (using ":" as the $2 delimiter):
+#
+#	prepend_var VAR : VALUE
+prepend_var () {
+	eval "$1=$3\${$1:+${3:+$2}\$$1}"
+}
+
+# If [AL]SAN is in effect we want to abort so that we notice
+# problems. The GIT_SAN_OPTIONS variable can be used to set common
+# defaults shared between [AL]SAN_OPTIONS.
+prepend_var GIT_SAN_OPTIONS : abort_on_error=1
+
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
 # the noise level. This needs to happen at the start of the script,
 # before we even do our "did we build git yet" check (since we don't
 # want that one to complain to stderr).
-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
+prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS
+prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
-# If LSAN is in effect we _do_ want leak checking, but we still
-# want to abort so that we notice the problems.
-: ${LSAN_OPTIONS=abort_on_error=1}
+prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v3 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding

[Ævar Arnfjörð Bjarmason]

Correct a misleading comment added by me in 62f539043c7 (test-lib:
Allow overriding of TEST_DIRECTORY, 2010-08-19).

Between that comment and the later addition of
85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,
2013-11-17) the comments were on the wrong arms of the "if". I.e. the
"allow tests to override this" was on the "test -z" arm.

But more importantly this could be read allowing the "$TEST_DIRECTORY"
to be some path outside of t/. As explained in the updated comment
that's impossible, rather it was meant for *tests* that ran outside of
t/, i.e. the "t0000-basic.sh" tests that use "lib-subtest.sh".

Those tests have a different working directory, but they set the
"TEST_DIRECTORY" to the same path for bootstrapping. The comments now
reflect that, and further comment on why we have a hard dependency on
this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55f263a02d3..77c3fabd041 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -19,13 +19,20 @@
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 if test -z "$TEST_DIRECTORY"
 then
-	# We allow tests to override this, in case they want to run tests
-	# outside of t/, e.g. for running tests on the test library
-	# itself.
-	TEST_DIRECTORY=$(pwd)
-else
 	# ensure that TEST_DIRECTORY is an absolute path so that it
 	# is valid even if the current working directory is changed
+	TEST_DIRECTORY=$(pwd)
+else
+	# The TEST_DIRECTORY will always be the path to the "t"
+	# directory in the git.git checkout. This is overridden by
+	# e.g. t/lib-subtest.sh, but only because its $(pwd) is
+	# different. Those tests still set "$TEST_DIRECTORY" to the
+	# same path.
+	#
+	# See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for
+	# hard assumptions about "$GIT_BUILD_DIR/t" existing and being
+	# the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper"
+	# needing to exist.
 	TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
 fi
 if test -z "$TEST_OUTPUT_DIRECTORY"
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to
"/path/to/build". The "TEST_DIRECTORY" here is already made an
absolute path a few lines above this.

We could simply do $(cd "$TEST_DIRECTORY"/.." && pwd) here, but as
noted in the preceding commit the "$TEST_DIRECTORY" can't be anything
except the path containing this test-lib.sh file at this point, so we
can more cheaply and equally strip the "/t" off the end.

This change will be helpful to LSAN_OPTIONS which will want to strip
the build directory path from filenames, which we couldn't do if we
had a "/.." in there.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 77c3fabd041..ff13321bfd3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -41,7 +41,7 @@ then
 	# elsewhere
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
-GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
 
 # Prepend a string to a VAR using an arbitrary ":" delimiter, not
 # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
@@ -59,6 +59,7 @@ prepend_var () {
 # problems. The GIT_SAN_OPTIONS variable can be used to set common
 # defaults shared between [AL]SAN_OPTIONS.
 prepend_var GIT_SAN_OPTIONS : abort_on_error=1
+prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"
 
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
-- 
2.35.1.1132.ga1fe46f8690

[PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":

    $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
        #1 0x9444a4 in xstrdup wrapper.c:29:14
        #2 0x5995fa in parse_date_format date.c:991:24
        #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
        #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
        #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
        #6 0x4cd1e3 in main common-main.c:52:11
        #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #8 0x422b09 in _start (t/helper/test-tool+0x422b09)

    SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Whereas LSAN would emit this instead:

    $ ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f2be1d614aa in strdup string/strdup.c:42:15

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:

    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
        #2 0x5cb164 in xstrdup wrapper.c:29:14
        #3 0x495ee9 in parse_date_format date.c:991:24
        #4 0x453aac in show_dates t/helper/test-date.c:39:2
        #5 0x453782 in cmd__date t/helper/test-date.c:116:3
        #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
        #7 0x451f1e in main common-main.c:52:11
        #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:

    $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
    Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
      Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
      Range (min … max):    2.122 s …  2.152 s    3 runs

    Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
      Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
      Range (min … max):    1.941 s …  2.044 s    3 runs

    Summary
      'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
        1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'

I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ff13321bfd3..a96d19a332e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -71,6 +71,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
 prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
+prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-- 
2.35.1.1132.ga1fe46f8690

Re: [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> A UX improvement for [AL]SAN stack traces. See
> https://lore.kernel.org/git/cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com/
> for the v2.
>
> Changes:
>  * Renamed GIT_XSAN_OPTIONS to GIT_SAN_OPTIONS per Junio's suggestion

Heh, I wasn't suggesting anything.  I am OK with or without X, or
with X but X replaced with anything else ;-)

Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Taylor Blau]

On Mon, Feb 21, 2022 at 04:58:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 77c3fabd041..ff13321bfd3 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -41,7 +41,7 @@ then
>  	# elsewhere
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
> +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"

Sorry to notice this so late, but this hunk caught my eye. What happens
if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")?

Before this change, we would have set GIT_BUILD_DIR to the parent of
whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still
do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if
it doesn't, then we'll set GIT_BUILD_DIR to be the same as
TEST_DIRECTORY, which is a behavior change.

If you just want GIT_BUILD_DIR to be absolute in order to for LSan to
understand how to correctly strip it out of filenames, could we replace
this with:

    GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)"

or (an admittedly less clear)

    GIT_BUILD_DIR="$(dirname "$TEST_DIRECTORY")"

Thanks,
Taylor

Re: [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

[Taylor Blau]

On Mon, Feb 21, 2022 at 04:58:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
> stack traces from LSAN. This isn't required under ASAN which will emit
> traces such as this one for a leak in "t/t0006-date.sh":
>
>     $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
>     [...]
>     Direct leak of 3 byte(s) in 1 object(s) allocated from:
>         #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
>         #1 0x9444a4 in xstrdup wrapper.c:29:14
>         #2 0x5995fa in parse_date_format date.c:991:24
>         #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
>         #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
>         #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
>         #6 0x4cd1e3 in main common-main.c:52:11
>         #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
>         #8 0x422b09 in _start (t/helper/test-tool+0x422b09)
>
>     SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
>     Aborted
>
> Whereas LSAN would emit this instead:
>
>     $ ./t0006-date.sh -vixd
>     [...]
>     Direct leak of 3 byte(s) in 1 object(s) allocated from:
>         #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
>         #1 0x7f2be1d614aa in strdup string/strdup.c:42:15
>
>     SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
>     Aborted
>
> Now we'll instead git this sensible stack trace under
> LSAN. I.e. almost the same one (but starting with "malloc", as is
> usual for LSAN) as under ASAN:
>
>     Direct leak of 3 byte(s) in 1 object(s) allocated from:
>         #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
>         #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
>         #2 0x5cb164 in xstrdup wrapper.c:29:14
>         #3 0x495ee9 in parse_date_format date.c:991:24
>         #4 0x453aac in show_dates t/helper/test-date.c:39:2
>         #5 0x453782 in cmd__date t/helper/test-date.c:116:3
>         #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
>         #7 0x451f1e in main common-main.c:52:11
>         #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
>         #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)
>
>     SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
>     Aborted

This is great, by the way. I have often hit that bug in LSan and been
incredibly frustrated by it. I'm happy to see it getting fixed here,
thank you.

> As the option name suggests this does make things slower, e.g. for
> t0001-init.sh we're around 10% slower:
>
>     $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
>     Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
>       Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
>       Range (min … max):    2.122 s …  2.152 s    3 runs
>
>     Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
>       Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
>       Range (min … max):    1.941 s …  2.044 s    3 runs
>
>     Summary
>       'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
>         1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'
>
> I think that's more than worth it to get the more meaningful stack
> traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
> one-off "fast" runs.

I completely agree. I am almost always run ASan / LSan tests a single
script at a time (often focusing on just one script that I know
demonstrates some bug).

At GitHub, we use both a sanitized and un-sanitized build when running
CI. So we'll probably feel the effects a little more during the "run
make test under a sanitized build" CI job, but we could easily set
fast_unwind_on_malloc=0 if it becomes too big of a problem for us
(though I suspect it won't matter in practice).

Thanks,
Taylor

Re: [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 21 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> A UX improvement for [AL]SAN stack traces. See
>> https://lore.kernel.org/git/cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com/
>> for the v2.
>>
>> Changes:
>>  * Renamed GIT_XSAN_OPTIONS to GIT_SAN_OPTIONS per Junio's suggestion
>
> Heh, I wasn't suggesting anything.  I am OK with or without X, or
> with X but X replaced with anything else ;-)

Ah, anyway I'm happy with this version, so unless there's another reason
for a re-roll...

Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 21 2022, Taylor Blau wrote:

> On Mon, Feb 21, 2022 at 04:58:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 77c3fabd041..ff13321bfd3 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -41,7 +41,7 @@ then
>>  	# elsewhere
>>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>>  fi
>> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>> +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>
> Sorry to notice this so late, but this hunk caught my eye. What happens
> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")?

I think that the preceding 2/4 should cover your concern here, i.e. I
think that's not possible.

> Before this change, we would have set GIT_BUILD_DIR to the parent of
> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still
> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if
> it doesn't, then we'll set GIT_BUILD_DIR to be the same as
> TEST_DIRECTORY, which is a behavior change.

Indeed, but I believe (again see 2/4) that can't happen.

> If you just want GIT_BUILD_DIR to be absolute in order to for LSan to
> understand how to correctly strip it out of filenames, could we replace
> this with:
>
>     GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)"
>
> or (an admittedly less clear)
>
>     GIT_BUILD_DIR="$(dirname "$TEST_DIRECTORY")"

Yes. I almost changed it to the former in this re-roll, but as noted in
<220219.86y227fvz1.gmgdl@evledraar.gmail.com> thought it was better to
have it clear that this really wasn't a generic mechanism, we really do
need/want the actual "t" directory here.

Re: [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 21 2022, Taylor Blau wrote:

> On Mon, Feb 21, 2022 at 04:58:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
>> stack traces from LSAN. This isn't required under ASAN which will emit
>> traces such as this one for a leak in "t/t0006-date.sh":
>>
>>     $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
>>     [...]
>>     Direct leak of 3 byte(s) in 1 object(s) allocated from:
>>         #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
>>         #1 0x9444a4 in xstrdup wrapper.c:29:14
>>         #2 0x5995fa in parse_date_format date.c:991:24
>>         #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
>>         #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
>>         #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
>>         #6 0x4cd1e3 in main common-main.c:52:11
>>         #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
>>         #8 0x422b09 in _start (t/helper/test-tool+0x422b09)
>>
>>     SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
>>     Aborted
>>
>> Whereas LSAN would emit this instead:
>>
>>     $ ./t0006-date.sh -vixd
>>     [...]
>>     Direct leak of 3 byte(s) in 1 object(s) allocated from:
>>         #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
>>         #1 0x7f2be1d614aa in strdup string/strdup.c:42:15
>>
>>     SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
>>     Aborted
>>
>> Now we'll instead git this sensible stack trace under
>> LSAN. I.e. almost the same one (but starting with "malloc", as is
>> usual for LSAN) as under ASAN:
>>
>>     Direct leak of 3 byte(s) in 1 object(s) allocated from:
>>         #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
>>         #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
>>         #2 0x5cb164 in xstrdup wrapper.c:29:14
>>         #3 0x495ee9 in parse_date_format date.c:991:24
>>         #4 0x453aac in show_dates t/helper/test-date.c:39:2
>>         #5 0x453782 in cmd__date t/helper/test-date.c:116:3
>>         #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
>>         #7 0x451f1e in main common-main.c:52:11
>>         #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
>>         #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)
>>
>>     SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
>>     Aborted
>
> This is great, by the way. I have often hit that bug in LSan and been
> incredibly frustrated by it. I'm happy to see it getting fixed here,
> thank you.

Cheers!

>> As the option name suggests this does make things slower, e.g. for
>> t0001-init.sh we're around 10% slower:
>>
>>     $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
>>     Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
>>       Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
>>       Range (min … max):    2.122 s …  2.152 s    3 runs
>>
>>     Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
>>       Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
>>       Range (min … max):    1.941 s …  2.044 s    3 runs
>>
>>     Summary
>>       'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
>>         1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'
>>
>> I think that's more than worth it to get the more meaningful stack
>> traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
>> one-off "fast" runs.
>
> I completely agree. I am almost always run ASan / LSan tests a single
> script at a time (often focusing on just one script that I know
> demonstrates some bug).
>
> At GitHub, we use both a sanitized and un-sanitized build when running
> CI. So we'll probably feel the effects a little more during the "run
> make test under a sanitized build" CI job, but we could easily set
> fast_unwind_on_malloc=0 if it becomes too big of a problem for us
> (though I suspect it won't matter in practice).

I suspect you mean SANITIZE=address not SANITIZE=leak, i.e. ASAN not
LSAN. Note that the performance of the two is often quite different
(with ASAN starting out much slower).

I almost never use ASAN, but I've been using LSAN a lot.

I also haven't been able to have ASAN spew out these worse stack traces,
as noted in the commit message. But in some brief testing now if I were
setting fast_unwind_on_malloc=0 it would be around 15% slower:
    
    $ hyperfine -L v 0,1 'ASAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 5
    Benchmark 1: ASAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
      Time (mean ± σ):      3.264 s ±  0.014 s    [User: 2.456 s, System: 0.927 s]
      Range (min … max):    3.249 s …  3.282 s    5 runs
     
    Benchmark 2: ASAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
      Time (mean ± σ):      2.826 s ±  0.023 s    [User: 2.008 s, System: 0.936 s]
      Range (min … max):    2.814 s …  2.866 s    5 runs
     
    Summary
      'ASAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
        1.15 ± 0.01 times faster than 'ASAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'

But I don't know in the ASAN case what, if anything, you're getting for
that reduction in performance.

Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Sorry to notice this so late, but this hunk caught my eye. What happens
>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")?
>
> I think that the preceding 2/4 should cover your concern here, i.e. I
> think that's not possible.
>
>> Before this change, we would have set GIT_BUILD_DIR to the parent of
>> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still
>> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if
>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as
>> TEST_DIRECTORY, which is a behavior change.
>
> Indeed, but I believe (again see 2/4) that can't happen.

It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY
override logic did not mean to support such a use case".

I am perfectly fine if we declared that we do not to support the use
of that override mechanism by anybody but the "subtest" thing we do
ourselves.  If we can catch a workflow that misuses the mechansim
cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set
and it does not end in "/t"), we should do so, I would think, instead
of doing the "go up and do pwd", which will make things worse.

Thanks.

Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 21 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Sorry to notice this so late, but this hunk caught my eye. What happens
>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")?
>>
>> I think that the preceding 2/4 should cover your concern here, i.e. I
>> think that's not possible.
>>
>>> Before this change, we would have set GIT_BUILD_DIR to the parent of
>>> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still
>>> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if
>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as
>>> TEST_DIRECTORY, which is a behavior change.
>>
>> Indeed, but I believe (again see 2/4) that can't happen.
>
> It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY
> override logic did not mean to support such a use case".

To clarify with "can't happen" I mean (and should have said) that "can't
work", i.e. it would error out anyway.

E.g. try in a git.git checkout:
    
    (
        mv t t2 &&
        cd t &&
        ./t0001-init.sh
    )

It will die with:
    
    You need to build test-tool:
    Run "make t/helper/test-tool" in the source (toplevel) directory
    FATAL: Unexpected exit with code 1

And if you were to manually patch test-lib.sh to get past that error it
would start erroring on e.g.:

    sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory

And if you "fix" that it'll error out on something else.

I.e. we'll have discovered that $(pwd)/.. must be our build directory,
and we then construct paths by adding the string "/t/[...]" to that.

> I am perfectly fine if we declared that we do not to support the use
> of that override mechanism by anybody but the "subtest" thing we do
> ourselves.  If we can catch a workflow that misuses the mechansim
> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set
> and it does not end in "/t"), we should do so, I would think, instead
> of doing the "go up and do pwd", which will make things worse.

What I was going for in 2/4 in
http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com
is that we've already declared that. I.e. test-lib.sh has various
assumptions about appending "/t/..." to the build directory being a
valid way to get paths to various test-lib.sh-adjacent code.

So trimming off "/t" here with a string operation v.s. $(cd .. && pwd)
is being consistent with that code.

It would be odd to make the bit at the top very generic, only to have
the reader keep reading and wonder how that generic mechanism and the
subsequent hardcoding of "/t/[...]" are supposed to work together.

Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Feb 21 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>> Sorry to notice this so late, but this hunk caught my eye. What happens
>>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")?
>>>
>>> I think that the preceding 2/4 should cover your concern here, i.e. I
>>> think that's not possible.
>>>
>>>> Before this change, we would have set GIT_BUILD_DIR to the parent of
>>>> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still
>>>> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if
>>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as
>>>> TEST_DIRECTORY, which is a behavior change.
>>>
>>> Indeed, but I believe (again see 2/4) that can't happen.
>>
>> It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY
>> override logic did not mean to support such a use case".
>
> To clarify with "can't happen" I mean (and should have said) that "can't
> work", i.e. it would error out anyway.
>
> E.g. try in a git.git checkout:
>     
>     (
>         mv t t2 &&
>         cd t &&
>         ./t0001-init.sh
>     )
>
> It will die with:
>     
>     You need to build test-tool:
>     Run "make t/helper/test-tool" in the source (toplevel) directory
>     FATAL: Unexpected exit with code 1
>
> And if you were to manually patch test-lib.sh to get past that error it
> would start erroring on e.g.:
>
>     sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory
>
> And if you "fix" that it'll error out on something else.
>
> I.e. we'll have discovered that $(pwd)/.. must be our build directory,
> and we then construct paths by adding the string "/t/[...]" to that.
>
>> I am perfectly fine if we declared that we do not to support the use
>> of that override mechanism by anybody but the "subtest" thing we do
>> ourselves.  If we can catch a workflow that misuses the mechansim
>> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set
>> and it does not end in "/t"), we should do so, I would think, instead
>> of doing the "go up and do pwd", which will make things worse.
>
> What I was going for in 2/4 in
> http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com
> is that we've already declared that. I.e. test-lib.sh has various
> assumptions about appending "/t/..." to the build directory being a
> valid way to get paths to various test-lib.sh-adjacent code.
>
> So trimming off "/t" here with a string operation v.s. $(cd .. && pwd)
> is being consistent with that code.
>
> It would be odd to make the bit at the top very generic, only to have
> the reader keep reading and wonder how that generic mechanism and the
> subsequent hardcoding of "/t/[...]" are supposed to work together.

Correct.  That is why I said $(... pwd) to pretend that we can take
anything would make it worse in a separate message.

If we have to strip off /t anyway, piggy-backing on that part to
detect and abort when somebody misused the mechanism would be a good
idea---which is what I said in the message you are responding to and
not responding to.

Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 23 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Mon, Feb 21 2022, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>>> Sorry to notice this so late, but this hunk caught my eye. What happens
>>>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")?
>>>>
>>>> I think that the preceding 2/4 should cover your concern here, i.e. I
>>>> think that's not possible.
>>>>
>>>>> Before this change, we would have set GIT_BUILD_DIR to the parent of
>>>>> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still
>>>>> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if
>>>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as
>>>>> TEST_DIRECTORY, which is a behavior change.
>>>>
>>>> Indeed, but I believe (again see 2/4) that can't happen.
>>>
>>> It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY
>>> override logic did not mean to support such a use case".
>>
>> To clarify with "can't happen" I mean (and should have said) that "can't
>> work", i.e. it would error out anyway.
>>
>> E.g. try in a git.git checkout:
>>     
>>     (
>>         mv t t2 &&
>>         cd t &&
>>         ./t0001-init.sh
>>     )
>>
>> It will die with:
>>     
>>     You need to build test-tool:
>>     Run "make t/helper/test-tool" in the source (toplevel) directory
>>     FATAL: Unexpected exit with code 1
>>
>> And if you were to manually patch test-lib.sh to get past that error it
>> would start erroring on e.g.:
>>
>>     sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory
>>
>> And if you "fix" that it'll error out on something else.
>>
>> I.e. we'll have discovered that $(pwd)/.. must be our build directory,
>> and we then construct paths by adding the string "/t/[...]" to that.
>>
>>> I am perfectly fine if we declared that we do not to support the use
>>> of that override mechanism by anybody but the "subtest" thing we do
>>> ourselves.  If we can catch a workflow that misuses the mechansim
>>> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set
>>> and it does not end in "/t"), we should do so, I would think, instead
>>> of doing the "go up and do pwd", which will make things worse.
>>
>> What I was going for in 2/4 in
>> http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com
>> is that we've already declared that. I.e. test-lib.sh has various
>> assumptions about appending "/t/..." to the build directory being a
>> valid way to get paths to various test-lib.sh-adjacent code.
>>
>> So trimming off "/t" here with a string operation v.s. $(cd .. && pwd)
>> is being consistent with that code.
>>
>> It would be odd to make the bit at the top very generic, only to have
>> the reader keep reading and wonder how that generic mechanism and the
>> subsequent hardcoding of "/t/[...]" are supposed to work together.
>
> Correct.  That is why I said $(... pwd) to pretend that we can take
> anything would make it worse in a separate message.
>
> If we have to strip off /t anyway, piggy-backing on that part to
> detect and abort when somebody misused the mechanism would be a good
> idea---which is what I said in the message you are responding to and
> not responding to.

So you're OK with the assumption/method being used here, but would
prefer if we also added an explicit check/"exit 1"? E.g.:
	
	if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}"
	then
	        echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
	        exit 1
	fi

?

Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> So you're OK with the assumption/method being used here, but would
> prefer if we also added an explicit check/"exit 1"? E.g.:
> 	
> 	if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}"
> 	then
> 	        echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> 	        exit 1
> 	fi

Exactly.

[PATCH v4 0/4] test-lib: improve LSAN + ASAN stack traces

[Ævar Arnfjörð Bjarmason]

A UX improvement for [AL]SAN stack traces. See
https://lore.kernel.org/git/cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com/
for the v3.

Changes since v3:

 * Add a comment/assertion in 2/4 about what "t" directory we
   expect/must have.

 * The 3/4 "change" is only for rebasing on the new 2/4.

Ævar Arnfjörð Bjarmason (4):
  test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS
  test-lib: correct and assert TEST_DIRECTORY overriding
  test-lib: make $GIT_BUILD_DIR an absolute path
  test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

 t/test-lib.sh | 50 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 10 deletions(-)

Range-diff against v3:
1:  bf31efca464 = 1:  d1967ab34a5 test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS
2:  33a628e9c3a ! 2:  97586ad4541 test-lib: correct commentary on TEST_DIRECTORY overriding
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-lib: correct commentary on TEST_DIRECTORY overriding
    +    test-lib: correct and assert TEST_DIRECTORY overriding
     
         Correct a misleading comment added by me in 62f539043c7 (test-lib:
    -    Allow overriding of TEST_DIRECTORY, 2010-08-19).
    +    Allow overriding of TEST_DIRECTORY, 2010-08-19), and add an assertion
    +    that TEST_DIRECTORY cannot point to any directory except the "t"
    +    directory in the top-level of git.git.
     
    -    Between that comment and the later addition of
    +    This assertion is in effect not new, since we'd already die if that
    +    wasn't the case[1], but it and the updated commentary help to make
    +    that clearer.
    +
    +    The existing comments were also on the wrong arms of the
    +    "if". I.e. the "allow tests to override this" was on the "test -z"
    +    arm. That came about due to a combination of 62f539043c7 and
         85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,
    -    2013-11-17) the comments were on the wrong arms of the "if". I.e. the
    -    "allow tests to override this" was on the "test -z" arm.
    +    2013-11-17).
     
    -    But more importantly this could be read allowing the "$TEST_DIRECTORY"
    +    Those earlier comments could be read as allowing the "$TEST_DIRECTORY"
         to be some path outside of t/. As explained in the updated comment
         that's impossible, rather it was meant for *tests* that ran outside of
         t/, i.e. the "t0000-basic.sh" tests that use "lib-subtest.sh".
    @@ Commit message
         reflect that, and further comment on why we have a hard dependency on
         this.
     
    +    1. https://lore.kernel.org/git/220222.86o82z8als.gmgdl@evledraar.gmail.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/test-lib.sh ##
    @@ t/test-lib.sh
      	TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
      fi
      if test -z "$TEST_OUTPUT_DIRECTORY"
    +@@ t/test-lib.sh: then
    + 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
    + fi
    + GIT_BUILD_DIR="$TEST_DIRECTORY"/..
    ++if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}"
    ++then
    ++	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
    ++	exit 1
    ++fi
    + 
    + # Prepend a string to a VAR using an arbitrary ":" delimiter, not
    + # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
3:  b03ae29fc92 ! 3:  c25c4532c72 test-lib: make $GIT_BUILD_DIR an absolute path
    @@ t/test-lib.sh: then
      	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
      fi
     -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
    +-if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}"
     +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
    - 
    - # Prepend a string to a VAR using an arbitrary ":" delimiter, not
    - # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
    ++if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
    + then
    + 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
    + 	exit 1
     @@ t/test-lib.sh: prepend_var () {
      # problems. The GIT_SAN_OPTIONS variable can be used to set common
      # defaults shared between [AL]SAN_OPTIONS.
4:  d212009e517 = 4:  fa4946ce7ef test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
-- 
2.35.1.1188.g137d9ee5e75

[PATCH v4 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
variables, rather than punting out entirely if we already have them in
the environment.

We want to take any user-provided settings over our own, but we can do
that by prepending our defaults to the variable. The libsanitizer
options parsing has "last option wins" semantics.

It's now possible to do e.g.:

    LSAN_OPTIONS=report_objects=1 ./t0006-date.sh

And not have the "report_objects=1" setting overwrite our sensible
default of "abort_on_error=1", but by prepending to the list we ensure
that:

    LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh

Will take the desired "abort_on_error=0" over our default.

See b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)
for the original pattern being altered here, and
85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,
2017-09-05) for when LSAN_OPTIONS was added in addition to the
then-existing ASAN_OPTIONS.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index e4716b0b867..55f263a02d3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -36,17 +36,33 @@ then
 fi
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 
+# Prepend a string to a VAR using an arbitrary ":" delimiter, not
+# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
+#
+#	VAR=$1${VAR:+${1:+$2}$VAR}
+#
+# Usage (using ":" as the $2 delimiter):
+#
+#	prepend_var VAR : VALUE
+prepend_var () {
+	eval "$1=$3\${$1:+${3:+$2}\$$1}"
+}
+
+# If [AL]SAN is in effect we want to abort so that we notice
+# problems. The GIT_SAN_OPTIONS variable can be used to set common
+# defaults shared between [AL]SAN_OPTIONS.
+prepend_var GIT_SAN_OPTIONS : abort_on_error=1
+
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
 # the noise level. This needs to happen at the start of the script,
 # before we even do our "did we build git yet" check (since we don't
 # want that one to complain to stderr).
-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
+prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS
+prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
-# If LSAN is in effect we _do_ want leak checking, but we still
-# want to abort so that we notice the problems.
-: ${LSAN_OPTIONS=abort_on_error=1}
+prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-- 
2.35.1.1188.g137d9ee5e75

[PATCH v4 2/4] test-lib: correct and assert TEST_DIRECTORY overriding

[Ævar Arnfjörð Bjarmason]

Correct a misleading comment added by me in 62f539043c7 (test-lib:
Allow overriding of TEST_DIRECTORY, 2010-08-19), and add an assertion
that TEST_DIRECTORY cannot point to any directory except the "t"
directory in the top-level of git.git.

This assertion is in effect not new, since we'd already die if that
wasn't the case[1], but it and the updated commentary help to make
that clearer.

The existing comments were also on the wrong arms of the
"if". I.e. the "allow tests to override this" was on the "test -z"
arm. That came about due to a combination of 62f539043c7 and
85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,
2013-11-17).

Those earlier comments could be read as allowing the "$TEST_DIRECTORY"
to be some path outside of t/. As explained in the updated comment
that's impossible, rather it was meant for *tests* that ran outside of
t/, i.e. the "t0000-basic.sh" tests that use "lib-subtest.sh".

Those tests have a different working directory, but they set the
"TEST_DIRECTORY" to the same path for bootstrapping. The comments now
reflect that, and further comment on why we have a hard dependency on
this.

1. https://lore.kernel.org/git/220222.86o82z8als.gmgdl@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55f263a02d3..48ee3b16ecd 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -19,13 +19,20 @@
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 if test -z "$TEST_DIRECTORY"
 then
-	# We allow tests to override this, in case they want to run tests
-	# outside of t/, e.g. for running tests on the test library
-	# itself.
-	TEST_DIRECTORY=$(pwd)
-else
 	# ensure that TEST_DIRECTORY is an absolute path so that it
 	# is valid even if the current working directory is changed
+	TEST_DIRECTORY=$(pwd)
+else
+	# The TEST_DIRECTORY will always be the path to the "t"
+	# directory in the git.git checkout. This is overridden by
+	# e.g. t/lib-subtest.sh, but only because its $(pwd) is
+	# different. Those tests still set "$TEST_DIRECTORY" to the
+	# same path.
+	#
+	# See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for
+	# hard assumptions about "$GIT_BUILD_DIR/t" existing and being
+	# the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper"
+	# needing to exist.
 	TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
 fi
 if test -z "$TEST_OUTPUT_DIRECTORY"
@@ -35,6 +42,11 @@ then
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}"
+then
+	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
+	exit 1
+fi
 
 # Prepend a string to a VAR using an arbitrary ":" delimiter, not
 # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
-- 
2.35.1.1188.g137d9ee5e75

[PATCH v4 3/4] test-lib: make $GIT_BUILD_DIR an absolute path

[Ævar Arnfjörð Bjarmason]

Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to
"/path/to/build". The "TEST_DIRECTORY" here is already made an
absolute path a few lines above this.

We could simply do $(cd "$TEST_DIRECTORY"/.." && pwd) here, but as
noted in the preceding commit the "$TEST_DIRECTORY" can't be anything
except the path containing this test-lib.sh file at this point, so we
can more cheaply and equally strip the "/t" off the end.

This change will be helpful to LSAN_OPTIONS which will want to strip
the build directory path from filenames, which we couldn't do if we
had a "/.." in there.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 48ee3b16ecd..ba5186c859b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -41,8 +41,8 @@ then
 	# elsewhere
 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
 fi
-GIT_BUILD_DIR="$TEST_DIRECTORY"/..
-if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}"
+GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
+if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
 then
 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
 	exit 1
@@ -64,6 +64,7 @@ prepend_var () {
 # problems. The GIT_SAN_OPTIONS variable can be used to set common
 # defaults shared between [AL]SAN_OPTIONS.
 prepend_var GIT_SAN_OPTIONS : abort_on_error=1
+prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"
 
 # If we were built with ASAN, it may complain about leaks
 # of program-lifetime variables. Disable it by default to lower
-- 
2.35.1.1188.g137d9ee5e75

[PATCH v4 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS

[Ævar Arnfjörð Bjarmason]

Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":

    $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
        #1 0x9444a4 in xstrdup wrapper.c:29:14
        #2 0x5995fa in parse_date_format date.c:991:24
        #3 0x4d2056 in show_dates t/helper/test-date.c:39:2
        #4 0x4d174a in cmd__date t/helper/test-date.c:116:3
        #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
        #6 0x4cd1e3 in main common-main.c:52:11
        #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #8 0x422b09 in _start (t/helper/test-tool+0x422b09)

    SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Whereas LSAN would emit this instead:

    $ ./t0006-date.sh -vixd
    [...]
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f2be1d614aa in strdup string/strdup.c:42:15

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:

    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
        #1 0x7f012af5c4aa in strdup string/strdup.c:42:15
        #2 0x5cb164 in xstrdup wrapper.c:29:14
        #3 0x495ee9 in parse_date_format date.c:991:24
        #4 0x453aac in show_dates t/helper/test-date.c:39:2
        #5 0x453782 in cmd__date t/helper/test-date.c:116:3
        #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
        #7 0x451f1e in main common-main.c:52:11
        #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
        #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)

    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
    Aborted

As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:

    $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
    Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
      Time (mean ± σ):      2.135 s ±  0.015 s    [User: 1.951 s, System: 0.554 s]
      Range (min … max):    2.122 s …  2.152 s    3 runs

    Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
      Time (mean ± σ):      1.981 s ±  0.055 s    [User: 1.769 s, System: 0.488 s]
      Range (min … max):    1.941 s …  2.044 s    3 runs

    Summary
      'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
        1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'

I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ba5186c859b..9af5fb7674d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -76,6 +76,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0
 export ASAN_OPTIONS
 
 prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
+prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-- 
2.35.1.1188.g137d9ee5e75