* [PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
2022-02-19 11:29 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Ævar Arnfjörð Bjarmason
@ 2022-02-19 11:29 ` Ævar Arnfjörð Bjarmason
2022-02-20 7:52 ` Junio C Hamano
2022-02-19 11:29 ` [PATCH v2 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-19 11:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS
2022-02-19 11:29 ` [PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
@ 2022-02-20 7:52 ` Junio C Hamano
0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2022-02-20 7:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Taylor Blau
Æ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".
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding
2022-02-19 11:29 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Ævar Arnfjörð Bjarmason
2022-02-19 11:29 ` [PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
@ 2022-02-19 11:29 ` Ævar Arnfjörð Bjarmason
2022-02-19 11:29 ` [PATCH v2 3/4] test-lib: make $GIT_BUILD_DIR an absolute path Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-19 11:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v2 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-19 11:29 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Ævar Arnfjörð Bjarmason
2022-02-19 11:29 ` [PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
2022-02-19 11:29 ` [PATCH v2 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding Ævar Arnfjörð Bjarmason
@ 2022-02-19 11:29 ` Ævar Arnfjörð Bjarmason
2022-02-19 11:29 ` [PATCH v2 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-19 11:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v2 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
2022-02-19 11:29 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-02-19 11:29 ` [PATCH v2 3/4] test-lib: make $GIT_BUILD_DIR an absolute path Ævar Arnfjörð Bjarmason
@ 2022-02-19 11:29 ` Ævar Arnfjörð Bjarmason
2022-02-21 2:30 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Taylor Blau
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-19 11:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces
2022-02-19 11:29 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-02-19 11:29 ` [PATCH v2 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS Ævar Arnfjörð Bjarmason
@ 2022-02-21 2:30 ` Taylor Blau
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
5 siblings, 0 replies; 39+ messages in thread
From: Taylor Blau @ 2022-02-21 2:30 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces
2022-02-19 11:29 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-02-21 2:30 ` [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces Taylor Blau
@ 2022-02-21 15:58 ` Ævar Arnfjörð Bjarmason
2022-02-21 15:58 ` [PATCH v3 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
` (5 more replies)
5 siblings, 6 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-21 15:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH v3 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
@ 2022-02-21 15:58 ` Ævar Arnfjörð Bjarmason
2022-02-21 15:58 ` [PATCH v3 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-21 15:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v3 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2022-02-21 15:58 ` [PATCH v3 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
@ 2022-02-21 15:58 ` Ævar Arnfjörð Bjarmason
2022-02-21 15:58 ` [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-21 15:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2022-02-21 15:58 ` [PATCH v3 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
2022-02-21 15:58 ` [PATCH v3 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding Ævar Arnfjörð Bjarmason
@ 2022-02-21 15:58 ` Ævar Arnfjörð Bjarmason
2022-02-21 17:29 ` Taylor Blau
2022-02-21 15:58 ` [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-21 15:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-21 15:58 ` [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path Ævar Arnfjörð Bjarmason
@ 2022-02-21 17:29 ` Taylor Blau
2022-02-21 18:55 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 39+ messages in thread
From: Taylor Blau @ 2022-02-21 17:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-21 17:29 ` Taylor Blau
@ 2022-02-21 18:55 ` Ævar Arnfjörð Bjarmason
2022-02-22 7:19 ` Junio C Hamano
0 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-21 18:55 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
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.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-21 18:55 ` Ævar Arnfjörð Bjarmason
@ 2022-02-22 7:19 ` Junio C Hamano
2022-02-22 10:14 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2022-02-22 7:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Taylor Blau, git
Æ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.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-22 7:19 ` Junio C Hamano
@ 2022-02-22 10:14 ` Ævar Arnfjörð Bjarmason
2022-02-23 20:16 ` Junio C Hamano
0 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-22 10:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
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.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-22 10:14 ` Ævar Arnfjörð Bjarmason
@ 2022-02-23 20:16 ` Junio C Hamano
2022-02-24 9:14 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2022-02-23 20:16 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Taylor Blau, git
Æ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.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-23 20:16 ` Junio C Hamano
@ 2022-02-24 9:14 ` Ævar Arnfjörð Bjarmason
2022-02-24 20:05 ` Junio C Hamano
0 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-24 9:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
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
?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-24 9:14 ` Ævar Arnfjörð Bjarmason
@ 2022-02-24 20:05 ` Junio C Hamano
0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2022-02-24 20:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Taylor Blau, git
Æ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.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-02-21 15:58 ` [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path Ævar Arnfjörð Bjarmason
@ 2022-02-21 15:58 ` Ævar Arnfjörð Bjarmason
2022-02-21 17:32 ` Taylor Blau
2022-02-21 17:16 ` [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces Junio C Hamano
2022-02-27 10:25 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-21 15:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
2022-02-21 15:58 ` [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS Ævar Arnfjörð Bjarmason
@ 2022-02-21 17:32 ` Taylor Blau
2022-02-21 18:59 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 39+ messages in thread
From: Taylor Blau @ 2022-02-21 17:32 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
2022-02-21 17:32 ` Taylor Blau
@ 2022-02-21 18:59 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-21 18:59 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
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.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-02-21 15:58 ` [PATCH v3 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS Ævar Arnfjörð Bjarmason
@ 2022-02-21 17:16 ` Junio C Hamano
2022-02-21 18:55 ` Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2022-02-21 17:16 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Taylor Blau
Æ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 ;-)
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH v4 0/4] test-lib: improve LSAN + ASAN stack traces
2022-02-21 15:58 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-02-21 17:16 ` [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces Junio C Hamano
@ 2022-02-27 10:25 ` Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
` (3 more replies)
5 siblings, 4 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-27 10:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH v4 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS
2022-02-27 10:25 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
@ 2022-02-27 10:25 ` Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 2/4] test-lib: correct and assert TEST_DIRECTORY overriding Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
3 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-27 10:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v4 2/4] test-lib: correct and assert TEST_DIRECTORY overriding
2022-02-27 10:25 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
@ 2022-02-27 10:25 ` Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 3/4] test-lib: make $GIT_BUILD_DIR an absolute path Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS Ævar Arnfjörð Bjarmason
3 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-27 10:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v4 3/4] test-lib: make $GIT_BUILD_DIR an absolute path
2022-02-27 10:25 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 2/4] test-lib: correct and assert TEST_DIRECTORY overriding Ævar Arnfjörð Bjarmason
@ 2022-02-27 10:25 ` Ævar Arnfjörð Bjarmason
2022-02-27 10:25 ` [PATCH v4 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS Ævar Arnfjörð Bjarmason
3 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-27 10:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v4 4/4] test-lib: add "fast_unwind_on_malloc=0" to LSAN_OPTIONS
2022-02-27 10:25 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-02-27 10:25 ` [PATCH v4 3/4] test-lib: make $GIT_BUILD_DIR an absolute path Ævar Arnfjörð Bjarmason
@ 2022-02-27 10:25 ` Ævar Arnfjörð Bjarmason
3 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-27 10:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau,
Æ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
^ permalink raw reply related [flat|nested] 39+ messages in thread