{"thread":{"id":"57441","subject":"[PATCH 0/4] test-lib: improve LSAN + ASAN stack traces","startedAt":"2022-02-18T21:01:55Z","lastAt":"2022-02-27T10:26:00Z","messageCount":39,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"448810","messageId":"cover-0.4-00000000000-20220218T205753Z-avarab@gmail.com","threadId":"57441","inReplyTo":null,"subject":"[PATCH 0/4] test-lib: improve LSAN + ASAN stack traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T21:01:41Z","receivedAt":"2022-02-18T21:01:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This small series provides a much needed UX improvement for\nSANITIZE=leak stack traces. As noted in 4/4 this makes LSAN around 10%\nslower, but in some cases the stack traces we show now are useless, so\nI think it's worth it.\n\nThis also changes the stack traces to strip the absolute path to the\nbuild directory from them. See 3/4 for how much better that looks.\n\nThis series is a result of a suggestion by Jeff King in [1], when some\nof these bad stack traces (which for anyone re-reading that, I had the\nwrong idea about, we just needed fast_unwind_on_malloc=0) were\ndiscussed.\n\n1. https://lore.kernel.org/git/YXxh%2FGMuy+sBViVY@coredump.intra.peff.net/\n\nÆvar Arnfjörð Bjarmason (4):\n test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS\n test-lib: make $GIT_BUILD_DIR an absolute path\n test-lib: add \"strip_path_prefix\" to XSAN_OPTIONS\n test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS\n\n t/test-lib.sh | 26 +++++++++++++++++++++-----\n 1 file changed, 21 insertions(+), 5 deletions(-)\n\n-- \n2.35.1.1031.g277d4562d2e\n\n"},{"id":"448811","messageId":"patch-1.4-75c8f7a719c-20220218T205753Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-0.4-00000000000-20220218T205753Z-avarab@gmail.com","subject":"[PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T21:01:42Z","receivedAt":"2022-02-18T21:02:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\nvariables, rather than punting out entirely if we already have them in\nthe environment.\n\nWe do want to take any user-provided settings over our own, but we can\ndo do that by prepending our defaults to the variable. The\nlibsanitizer options parsing has \"last option wins\" semantics.\n\nIt's now possible to do e.g.:\n\n LSAN_OPTIONS=report_objects=1 ./t0006-date.sh\n\nAnd not have the \"report_objects=1\" setting overwrite our sensible\ndefault of \"abort_on_error=1\", but by prepending to the list we ensure\nthat:\n\n LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh\n\nWill take the desired \"abort_on_error=0\" over our default.\n\nSee b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)\nfor the original pattern being altered here, and\n85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,\n2017-09-05) for when LSAN_OPTIONS was added in addition to the\nthen-existing ASAN_OPTIONS.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e4716b0b867..3212966a82f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -36,17 +36,31 @@ then\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n+# Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n+# adding the delimiter if VAR is empty. I.e. a generalized:\n+#\n+#\tVAR=$1${VAR:+$VAR}\n+#\n+# Usage (using \":\" as a delimiter):\n+#\n+#\tprepend_var VAR : $1\n+prepend_var () {\n+\teval \"$1=$3\\${$1:+$2\\$$1}\"\n+}\n+\n+# If [AL]SAN is in effect we want to abort so that we notice problems.\n+prepend_var XSAN_OPTIONS : abort_on_error=1\n+\n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n # the noise level. This needs to happen at the start of the script,\n # before we even do our \"did we build git yet\" check (since we don't\n # want that one to complain to stderr).\n-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n+prepend_var ASAN_OPTIONS : $XSAN_OPTIONS\n+prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n-# If LSAN is in effect we _do_ want leak checking, but we still\n-# want to abort so that we notice the problems.\n-: ${LSAN_OPTIONS=abort_on_error=1}\n+prepend_var LSAN_OPTIONS : $XSAN_OPTIONS\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-- \n2.35.1.1031.g277d4562d2e\n\n"},{"id":"448812","messageId":"patch-2.4-4c53c6157ac-20220218T205753Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-0.4-00000000000-20220218T205753Z-avarab@gmail.com","subject":"[PATCH 2/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T21:01:43Z","receivedAt":"2022-02-18T21:02:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GIT_BUILD_DIR from a path like \"/path/to/build/t/..\" to\n\"/path/to/build\". The \"TEST_DIRECTORY\" here is already made an\nabsolute path a few lines above this.\n\nThis will be helpful to LSAN_OPTIONS which will want to strip the\nbuild directory path from filenames, which we couldn't do if we had a\n\"/..\" in there.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 3212966a82f..4f523b82ce5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -34,7 +34,7 @@ then\n \t# elsewhere\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n-GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n+GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n \n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n # adding the delimiter if VAR is empty. I.e. a generalized:\n-- \n2.35.1.1031.g277d4562d2e\n\n"},{"id":"448814","messageId":"xmqqley7yd6e.fsf@gitster.g","threadId":"57441","inReplyTo":"patch-1.4-75c8f7a719c-20220218T205753Z-avarab@gmail.com","subject":"Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-18T23:20:41Z","receivedAt":"2022-02-18T23:20:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\n> variables, rather than punting out entirely if we already have them in\n> the environment.\n>\n> We do want to take any user-provided settings over our own, but we can\n> do do that by prepending our defaults to the variable. The\n> libsanitizer options parsing has \"last option wins\" semantics.\n>\n> It's now possible to do e.g.:\n>\n> LSAN_OPTIONS=report_objects=1 ./t0006-date.sh\n>\n> And not have the \"report_objects=1\" setting overwrite our sensible\n> default of \"abort_on_error=1\", but by prepending to the list we ensure\n> that:\n>\n> LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh\n>\n> Will take the desired \"abort_on_error=0\" over our default.\n\nMakes sense.\n\n> +# Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n> +# adding the delimiter if VAR is empty. I.e. a generalized:\n> +#\n> +#\tVAR=$1${VAR:+$VAR}\n\nThis reads: \"Begin with the first parameter, and if VAR is set to a\nnon-empty string, append $VAR immediately after it without any\ndelimiter\". I would understand if it were\n\n VAR=$1${VAR:+\":$VAR\"}\n\n(or without the first colon, allowing an empty string as a valid\nmember of colon-separated list).\n\n> +# Usage (using \":\" as a delimiter):\n> +#\n> +#\tprepend_var VAR : $1\n> +prepend_var () {\n> +\teval \"$1=$3\\${$1:+$2\\$$1}\"\n\nThis one is correct; the above sample, when passed \":\" and \"VAR\" to\n$1 and $2, will specialize into the above example.\n\n> +}\n> +\n> +# If [AL]SAN is in effect we want to abort so that we notice problems.\n> +prepend_var XSAN_OPTIONS : abort_on_error=1\n\nXSAN_OPTIONS stands for \"options that are common to all ?SAN\", I\nguess.\n\n> # If we were built with ASAN, it may complain about leaks\n> # of program-lifetime variables. Disable it by default to lower\n> # the noise level. This needs to happen at the start of the script,\n> # before we even do our \"did we build git yet\" check (since we don't\n> # want that one to complain to stderr).\n> -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n> +prepend_var ASAN_OPTIONS : $XSAN_OPTIONS\n> +prepend_var ASAN_OPTIONS : detect_leaks=0\n\nThis makes me wonder if you want to generalize prepend_var even\nfurther to notice when \"$3\" is an empty string. It already avoids\nspending an extra colon (and introducing an empty element in the\ncolon delimited list) by paying attention to the current value of\n$1, so it would make sense to do the same for the incoming value.\n\nIOW, the current prepend_var implementation relies on $XSAN_OPTIONS\nand detect_leaks=0 both being non-empty string.\n\n> export ASAN_OPTIONS\n> \n> -# If LSAN is in effect we _do_ want leak checking, but we still\n> -# want to abort so that we notice the problems.\n> -: ${LSAN_OPTIONS=abort_on_error=1}\n> +prepend_var LSAN_OPTIONS : $XSAN_OPTIONS\n\nBut other than that, I think this step makes quite a lot of sense.\n\n> export LSAN_OPTIONS\n> \n> if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n"},{"id":"448815","messageId":"xmqqee3zycpg.fsf@gitster.g","threadId":"57441","inReplyTo":"patch-2.4-4c53c6157ac-20220218T205753Z-avarab@gmail.com","subject":"Re: [PATCH 2/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-18T23:30:51Z","receivedAt":"2022-02-18T23:30:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the GIT_BUILD_DIR from a path like \"/path/to/build/t/..\" to\n> \"/path/to/build\". The \"TEST_DIRECTORY\" here is already made an\n> absolute path a few lines above this.\n>\n> This will be helpful to LSAN_OPTIONS which will want to strip the\n> build directory path from filenames, which we couldn't do if we had a\n> \"/..\" in there.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/test-lib.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 3212966a82f..4f523b82ce5 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -34,7 +34,7 @@ then\n> \t# elsewhere\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> -GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n> +GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n\nThis makes perfect sense in the normal case, but the provision the\ncode that precedes this part has, i.e.\n\n if test -z \"$TEST_DIRECTORY\"\n then\n # We allow tests to override this, in case they want to run tests\n # outside of t/, e.g. for running tests on the test library\n # itself.\n TEST_DIRECTORY=$(pwd)\n else\n # ensure that TEST_DIRECTORY is an absolute path so that it\n # is valid even if the current working directory is changed\n TEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) || exit 1\n fi\n\nto allow TEST_DIRECTORY to be set externally robs the guarantee that\nyou can sensibly strip \"/t\" from its tail and expect everything to\nwork correctly. The only thing the original requires on such an\nexternally given TEST_DIRECTORY is that one level above it is usable\nas GIT_BUILD_DIR.\n\nIOW,\n\n\tGIT_BUILD_DIR=\"$(cd \"$TEST_DIRECTORY/..\" && pwd)\"\n\nwould give you what you want to achieve in either code path, as long\nas the original was working correctly for whatever value that is\ngiven to TEST_DIRECTORY externally.\n\nSo, perhaps\n\n\tif test -z \"$TEST_DIRECTORY\"\n\tthen\n\t\tTEST_DIRECTORY=$(pwd)\n\t\tGIT_BUILD_DIR=${TEST_DIRECTORY%/t}\n\telse\n\t\tTEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) &&\n\t\tGIT_BUILD_DIR=$(cd \"$TEST_DIRECTORY/..\" && pwd)\n\tfi\n\nor something like that? I dunno.\n\n"},{"id":"448823","messageId":"220219.86y227fvz1.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"xmqqee3zycpg.fsf@gitster.g","subject":"Re: [PATCH 2/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T01:58:48Z","receivedAt":"2022-02-19T02:09:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 18 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change the GIT_BUILD_DIR from a path like \"/path/to/build/t/..\" to\n>> \"/path/to/build\". The \"TEST_DIRECTORY\" here is already made an\n>> absolute path a few lines above this.\n>>\n>> This will be helpful to LSAN_OPTIONS which will want to strip the\n>> build directory path from filenames, which we couldn't do if we had a\n>> \"/..\" in there.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> t/test-lib.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 3212966a82f..4f523b82ce5 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -34,7 +34,7 @@ then\n>> \t# elsewhere\n>> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n>> fi\n>> -GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n>> +GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n>\n> This makes perfect sense in the normal case, but the provision the\n> code that precedes this part has, i.e.\n>\n> if test -z \"$TEST_DIRECTORY\"\n> then\n> # We allow tests to override this, in case they want to run tests\n> # outside of t/, e.g. for running tests on the test library\n> # itself.\n> TEST_DIRECTORY=$(pwd)\n> else\n> # ensure that TEST_DIRECTORY is an absolute path so that it\n> # is valid even if the current working directory is changed\n> TEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) || exit 1\n> fi\n>\n> to allow TEST_DIRECTORY to be set externally robs the guarantee that\n> you can sensibly strip \"/t\" from its tail and expect everything to\n> work correctly. The only thing the original requires on such an\n> externally given TEST_DIRECTORY is that one level above it is usable\n> as GIT_BUILD_DIR.\n>\n> IOW,\n>\n> \tGIT_BUILD_DIR=\"$(cd \"$TEST_DIRECTORY/..\" && pwd)\"\n>\n> would give you what you want to achieve in either code path, as long\n> as the original was working correctly for whatever value that is\n> given to TEST_DIRECTORY externally.\n>\n> So, perhaps\n>\n> \tif test -z \"$TEST_DIRECTORY\"\n> \tthen\n> \t\tTEST_DIRECTORY=$(pwd)\n> \t\tGIT_BUILD_DIR=${TEST_DIRECTORY%/t}\n> \telse\n> \t\tTEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) &&\n> \t\tGIT_BUILD_DIR=$(cd \"$TEST_DIRECTORY/..\" && pwd)\n> \tfi\n>\n> or something like that? I dunno.\n\nI think you're being led astray by the \"we allow tests to override this\"\ncomment, which is something I added in 62f539043c7 (test-lib: Allow\noverriding of TEST_DIRECTORY, 2010-08-19). I'm having some trouble\nunderstanding what I meant at the time.\n\nBut it's not the case that we support a TEST_DIRECTORY pointing to\nsomething that isn't inside the \"t/\" directory in our tree, as looking\nat uses of the two shows:\n \n $ git grep -E '\\$(GIT_BUILD_DIR|TEST_DIRECTORY)' -- t/test-lib.sh\n t/test-lib.sh:if test -z \"$TEST_DIRECTORY\"\n t/test-lib.sh: TEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) || exit 1\n t/test-lib.sh: TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n t/test-lib.sh:GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n t/test-lib.sh:if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n t/test-lib.sh:. \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n t/test-lib.sh:\"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X\" >/dev/null\n t/test-lib.sh:. \"$TEST_DIRECTORY/test-lib-functions.sh\"\n t/test-lib.sh: $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!')\n t/test-lib.sh: symlink_target=\"$GIT_BUILD_DIR/t/helper/$base\"\n t/test-lib.sh: symlink_target=\"$GIT_BUILD_DIR/$base\"\n t/test-lib.sh: GIT_VALGRIND=$TEST_DIRECTORY/valgrind\n t/test-lib.sh: for file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/t/helper/test-*\n t/test-lib.sh: make_symlink \"$GIT_BUILD_DIR\"/mergetools \"$GIT_VALGRIND/bin/mergetools\"\n t/test-lib.sh: PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH\n t/test-lib.sh: git_bin_dir=\"$GIT_BUILD_DIR/bin-wrappers\"\n t/test-lib.sh: GIT_EXEC_PATH=$GIT_BUILD_DIR\n t/test-lib.sh: PATH=\"$GIT_BUILD_DIR:$GIT_BUILD_DIR/t/helper:$PATH\"\n t/test-lib.sh:GIT_TEMPLATE_DIR=\"$GIT_BUILD_DIR\"/templates/blt\n t/test-lib.sh:GITPERLLIB=\"$GIT_BUILD_DIR\"/perl/build/lib\n t/test-lib.sh:test -d \"$GIT_BUILD_DIR\"/templates/blt || {\n t/test-lib.sh:if ! test -x \"$GIT_BUILD_DIR\"/t/helper/test-tool$X\n\nI.e. we already depend on the build dir being one-above the\nTEST_DIRECTORY, and per test-lib.sh we load test-lib-functions.sh from\n$TEST_DIRECTORY, and refer to $GIT_BUILD_DIR/t/... for various things\n(e.g. t/helper).\n\nThat being said it was a bit of a micro-optimization of mine to avoid a\n\"&& pwd\" there, and perhaps it was too clever.\n\nBut I do think we can 100% rely on just stripping the \"/t\" from the end\nof the string.\n\nRe-reading it again, what that comment really meant to say is that we\nallow overriding TEST_DIRECTORY from $(pwd) because when we run that\ntest-lib.sh we may be in another directory.\n\nBut that overriding code still sets us to the same directory, which ends\nin a \"/t\". See my subsequent 7b905119703 (t/t0000-basic.sh: Run the\npassing TODO test inside its own test-lib, 2010-08-19) for the first use\nof it.\n\nI.e. it's for the t0000-basic.sh testing where we run sub-tests, which\nnow live in t/lib-subtest.sh.\n\nSo I could keep that code as-is for a re-roll, but adjust the misleading\ncomment while I'm at it, how does that sound?\n"},{"id":"448824","messageId":"YhBY8oD/xJZUQsj9@nand.local","threadId":"57441","inReplyTo":"xmqqley7yd6e.fsf@gitster.g","subject":"Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-19T02:41:54Z","receivedAt":"2022-02-19T02:42:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Feb 18, 2022 at 03:20:41PM -0800, Junio C Hamano wrote:\n> > +# Usage (using \":\" as a delimiter):\n> > +#\n> > +#\tprepend_var VAR : $1\n> > +prepend_var () {\n> > +\teval \"$1=$3\\${$1:+$2\\$$1}\"\n>\n> This one is correct; the above sample, when passed \":\" and \"VAR\" to\n> $1 and $2, will specialize into the above example.\n\n> > +}\n> > +\n> > +# If [AL]SAN is in effect we want to abort so that we notice problems.\n> > +prepend_var XSAN_OPTIONS : abort_on_error=1\n>\n> XSAN_OPTIONS stands for \"options that are common to all ?SAN\", I\n> guess.\n\nI was also unclear on this. Looking around in the google/sanitizers\nrepository, I don't see something called \"XSAN_OPTIONS\" mentioned\nanywhere (neither in documentation nor in the actual source code).\n\nIs this a convenience variable that we use to store options that are\ncommon to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the\nextra confusion is worth it, since it only contains abort_on_error=1.\n\nI guess it makes it (along with the prepend_var function introduced by\nthis patch) possible for a caller to write XSAN_OPTIONS=... into their\nenvironment and then run a test script and have their settings feed into\nASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find\nthis super useful (or, at least not dramatically more convenient than\nsetting both variables).\n\nI could be wrong, and I'm obviously biased towards my own usage of the\nASAN/LSAN builds, but to me this patch might be clearer without the\nextra variable.\n\n> > # If we were built with ASAN, it may complain about leaks\n> > # of program-lifetime variables. Disable it by default to lower\n> > # the noise level. This needs to happen at the start of the script,\n> > # before we even do our \"did we build git yet\" check (since we don't\n> > # want that one to complain to stderr).\n> > -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n> > +prepend_var ASAN_OPTIONS : $XSAN_OPTIONS\n> > +prepend_var ASAN_OPTIONS : detect_leaks=0\n>\n> This makes me wonder if you want to generalize prepend_var even\n> further to notice when \"$3\" is an empty string. It already avoids\n> spending an extra colon (and introducing an empty element in the\n> colon delimited list) by paying attention to the current value of\n> $1, so it would make sense to do the same for the incoming value.\n>\n> IOW, the current prepend_var implementation relies on $XSAN_OPTIONS\n> and detect_leaks=0 both being non-empty string.\n\nYes, agreed.\n\nThanks,\nTaylor\n"},{"id":"448825","messageId":"220219.86ley7fu3k.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"YhBY8oD/xJZUQsj9@nand.local","subject":"Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T02:48:42Z","receivedAt":"2022-02-19T02:50:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 18 2022, Taylor Blau wrote:\n\n> On Fri, Feb 18, 2022 at 03:20:41PM -0800, Junio C Hamano wrote:\n>> > +# Usage (using \":\" as a delimiter):\n>> > +#\n>> > +#\tprepend_var VAR : $1\n>> > +prepend_var () {\n>> > +\teval \"$1=$3\\${$1:+$2\\$$1}\"\n>>\n>> This one is correct; the above sample, when passed \":\" and \"VAR\" to\n>> $1 and $2, will specialize into the above example.\n>\n>> > +}\n>> > +\n>> > +# If [AL]SAN is in effect we want to abort so that we notice problems.\n>> > +prepend_var XSAN_OPTIONS : abort_on_error=1\n>>\n>> XSAN_OPTIONS stands for \"options that are common to all ?SAN\", I\n>> guess.\n>\n> I was also unclear on this. Looking around in the google/sanitizers\n> repository, I don't see something called \"XSAN_OPTIONS\" mentioned\n> anywhere (neither in documentation nor in the actual source code).\n>\n> Is this a convenience variable that we use to store options that are\n> common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the\n> extra confusion is worth it, since it only contains abort_on_error=1.\n>\n> I guess it makes it (along with the prepend_var function introduced by\n> this patch) possible for a caller to write XSAN_OPTIONS=... into their\n> environment and then run a test script and have their settings feed into\n> ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find\n> this super useful (or, at least not dramatically more convenient than\n> setting both variables).\n>\n> I could be wrong, and I'm obviously biased towards my own usage of the\n> ASAN/LSAN builds, but to me this patch might be clearer without the\n> extra variable.\n\nSorry, yes it's just a git.git invention to refer to \"common LSAN and\nASAN' things. Perhaps GIT_ASAN_AND_ASAN_COMMON or something would be\nmuch less confusing & better? Bikeshedding most welcome :)\n"},{"id":"448827","messageId":"YhBclvZUYcsTabye@nand.local","threadId":"57441","inReplyTo":"220219.86ley7fu3k.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-19T02:57:26Z","receivedAt":"2022-02-19T02:57:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 19, 2022 at 03:48:42AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> > +}\n> >> > +\n> >> > +# If [AL]SAN is in effect we want to abort so that we notice problems.\n> >> > +prepend_var XSAN_OPTIONS : abort_on_error=1\n> >>\n> >> XSAN_OPTIONS stands for \"options that are common to all ?SAN\", I\n> >> guess.\n> >\n> > I was also unclear on this. Looking around in the google/sanitizers\n> > repository, I don't see something called \"XSAN_OPTIONS\" mentioned\n> > anywhere (neither in documentation nor in the actual source code).\n> >\n> > Is this a convenience variable that we use to store options that are\n> > common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the\n> > extra confusion is worth it, since it only contains abort_on_error=1.\n> >\n> > I guess it makes it (along with the prepend_var function introduced by\n> > this patch) possible for a caller to write XSAN_OPTIONS=... into their\n> > environment and then run a test script and have their settings feed into\n> > ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find\n> > this super useful (or, at least not dramatically more convenient than\n> > setting both variables).\n> >\n> > I could be wrong, and I'm obviously biased towards my own usage of the\n> > ASAN/LSAN builds, but to me this patch might be clearer without the\n> > extra variable.\n>\n> Sorry, yes it's just a git.git invention to refer to \"common LSAN and\n> ASAN' things. Perhaps GIT_ASAN_AND_ASAN_COMMON or something would be\n> much less confusing & better? Bikeshedding most welcome :)\n\nYeah, I would be fine with something like GIT_ASAN_AND_LSAN_COMMON if it\nmakes things easier. I think it definitely is clearer, and does make it\neasier to add new options to both.\n\nI probably wouldn't ever tweak its value myself, so I don't think I have\na strong opinion here. If it were me, I'd probably just as soon\nduplicate setting `abort_on_error=1` in the ASan- and LSan-specific\nvariables. But I don't have strong feelings here, so whatever makes the\nmost sense to you is fine with me.\n\nThanks,\nTaylor\n"},{"id":"448829","messageId":"220219.86czjjftde.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"YhBclvZUYcsTabye@nand.local","subject":"Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T03:02:23Z","receivedAt":"2022-02-19T03:05:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 18 2022, Taylor Blau wrote:\n\n> On Sat, Feb 19, 2022 at 03:48:42AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> >> > +}\n>> >> > +\n>> >> > +# If [AL]SAN is in effect we want to abort so that we notice problems.\n>> >> > +prepend_var XSAN_OPTIONS : abort_on_error=1\n>> >>\n>> >> XSAN_OPTIONS stands for \"options that are common to all ?SAN\", I\n>> >> guess.\n>> >\n>> > I was also unclear on this. Looking around in the google/sanitizers\n>> > repository, I don't see something called \"XSAN_OPTIONS\" mentioned\n>> > anywhere (neither in documentation nor in the actual source code).\n>> >\n>> > Is this a convenience variable that we use to store options that are\n>> > common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the\n>> > extra confusion is worth it, since it only contains abort_on_error=1.\n>> >\n>> > I guess it makes it (along with the prepend_var function introduced by\n>> > this patch) possible for a caller to write XSAN_OPTIONS=... into their\n>> > environment and then run a test script and have their settings feed into\n>> > ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find\n>> > this super useful (or, at least not dramatically more convenient than\n>> > setting both variables).\n>> >\n>> > I could be wrong, and I'm obviously biased towards my own usage of the\n>> > ASAN/LSAN builds, but to me this patch might be clearer without the\n>> > extra variable.\n>>\n>> Sorry, yes it's just a git.git invention to refer to \"common LSAN and\n>> ASAN' things. Perhaps GIT_ASAN_AND_ASAN_COMMON or something would be\n>> much less confusing & better? Bikeshedding most welcome :)\n>\n> Yeah, I would be fine with something like GIT_ASAN_AND_LSAN_COMMON if it\n> makes things easier. I think it definitely is clearer, and does make it\n> easier to add new options to both.\n>\n> I probably wouldn't ever tweak its value myself, so I don't think I have\n> a strong opinion here. If it were me, I'd probably just as soon\n> duplicate setting `abort_on_error=1` in the ASan- and LSan-specific\n> variables. But I don't have strong feelings here, so whatever makes the\n> most sense to you is fine with me.\n\nOnce you get to tweaking some more advanced options it makes sense to\nshare a lot for both. One change I have locally on top of this is e.g.:\n\n prepend_var XSAN_OPTIONS : dedup_token_length=9999\n prepend_var XSAN_OPTIONS : log_exe_name=1\n prepend_var XSAN_OPTIONS : log_path=\\\"$TEST_RESULTS_XSAN_FILE\\\"\n\nI.e. for a feature to log machine-readable stacktraces when you run the\ntest suite with LSAN or ASAN.\n\n prepend_var XSAN_OPTIONS : dedup_token_length=9999\n prepend_var XSAN_OPTIONS : log_exe_name=1\n prepend_var XSAN_OPTIONS : log_path=\\\"$TEST_RESULTS_XSAN_FILE\\\"\n\nWhich is how I ended up with XSAN_OPTIONS I think, i.e. I'd test things\nwith LSAN, switch to ASAN, would have to rename the variable\netc. Changing a single letter was easier ... :)\n"},{"id":"448832","messageId":"YhBpP0idXDB2a0Sy@nand.local","threadId":"57441","inReplyTo":"220219.86czjjftde.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-19T03:51:27Z","receivedAt":"2022-02-19T03:51:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 19, 2022 at 04:02:23AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Once you get to tweaking some more advanced options it makes sense to\n> share a lot for both. One change I have locally on top of this is e.g.:\n\nMakes sense. I run the tests with sanitization quite often, but rarely\nuse any fanciful options like you.\n\nThanks,\nTaylor\n"},{"id":"448850","messageId":"patch-v2-1.4-01e63a72231-20220219T112653Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com","subject":"[PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T11:29:40Z","receivedAt":"2022-02-19T11:29:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\nvariables, rather than punting out entirely if we already have them in\nthe environment.\n\nWe do want to take any user-provided settings over our own, but we can\ndo do that by prepending our defaults to the variable. The\nlibsanitizer options parsing has \"last option wins\" semantics.\n\nIt's now possible to do e.g.:\n\n LSAN_OPTIONS=report_objects=1 ./t0006-date.sh\n\nAnd not have the \"report_objects=1\" setting overwrite our sensible\ndefault of \"abort_on_error=1\", but by prepending to the list we ensure\nthat:\n\n LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh\n\nWill take the desired \"abort_on_error=0\" over our default.\n\nSee b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)\nfor the original pattern being altered here, and\n85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,\n2017-09-05) for when LSAN_OPTIONS was added in addition to the\nthen-existing ASAN_OPTIONS.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 24 ++++++++++++++++++++----\n 1 file changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e4716b0b867..7e6978d1817 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -36,17 +36,33 @@ then\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n+# Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n+# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n+#\n+#\tVAR=$1${VAR:+${1:+$2}$VAR}\n+#\n+# Usage (using \":\" as the $2 delimiter):\n+#\n+#\tprepend_var VAR : VALUE\n+prepend_var () {\n+\teval \"$1=$3\\${$1:+${3:+$2}\\$$1}\"\n+}\n+\n+# If [AL]SAN is in effect we want to abort so that we notice\n+# problems. The GIT_XSAN_OPTIONS variable can be used to set common\n+# defaults shared between [AL]SAN_OPTIONS.\n+prepend_var GIT_XSAN_OPTIONS : abort_on_error=1\n+\n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n # the noise level. This needs to happen at the start of the script,\n # before we even do our \"did we build git yet\" check (since we don't\n # want that one to complain to stderr).\n-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n+prepend_var ASAN_OPTIONS : $GIT_XSAN_OPTIONS\n+prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n-# If LSAN is in effect we _do_ want leak checking, but we still\n-# want to abort so that we notice the problems.\n-: ${LSAN_OPTIONS=abort_on_error=1}\n+prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-- \n2.35.1.1130.g7c6dd716f26\n\n"},{"id":"448851","messageId":"cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-0.4-00000000000-20220218T205753Z-avarab@gmail.com","subject":"[PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T11:29:39Z","receivedAt":"2022-02-19T11:29:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A UX improvement for LSAN stack traces. See\nhttps://lore.kernel.org/git/cover-0.4-00000000000-20220218T205753Z-avarab@gmail.com/\nfor the v1.\n\nI think this v2 should address all the comments on the v1, thanks\nTaylor & Junio!\n\nChanges:\n * Renamed XSAN_OPTIONS to GIT_XSAN_OPTIONS\n * The \"prepend_var\" now handles an empty $3, as suggested by Junio.\n * I added a new 2/4 updating the $TEST_DIRECTORY commentary to note\n that we depend on it pointing to *the* \"t\" directory.\n\nÆvar Arnfjörð Bjarmason (4):\n test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS\n test-lib: correct commentary on TEST_DIRECTORY overriding\n test-lib: make $GIT_BUILD_DIR an absolute path\n test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS\n\n t/test-lib.sh | 45 +++++++++++++++++++++++++++++++++++----------\n 1 file changed, 35 insertions(+), 10 deletions(-)\n\nRange-diff against v1:\n1: 75c8f7a719c ! 1: 01e63a72231 test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS\n + test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS\n \n Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\n variables, rather than punting out entirely if we already have them in\n @@ t/test-lib.sh: then\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n +# Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n -+# adding the delimiter if VAR is empty. I.e. a generalized:\n ++# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n +#\n -+#\tVAR=$1${VAR:+$VAR}\n ++#\tVAR=$1${VAR:+${1:+$2}$VAR}\n +#\n -+# Usage (using \":\" as a delimiter):\n ++# Usage (using \":\" as the $2 delimiter):\n +#\n -+#\tprepend_var VAR : $1\n ++#\tprepend_var VAR : VALUE\n +prepend_var () {\n -+\teval \"$1=$3\\${$1:+$2\\$$1}\"\n ++\teval \"$1=$3\\${$1:+${3:+$2}\\$$1}\"\n +}\n +\n -+# If [AL]SAN is in effect we want to abort so that we notice problems.\n -+prepend_var XSAN_OPTIONS : abort_on_error=1\n ++# If [AL]SAN is in effect we want to abort so that we notice\n ++# problems. The GIT_XSAN_OPTIONS variable can be used to set common\n ++# defaults shared between [AL]SAN_OPTIONS.\n ++prepend_var GIT_XSAN_OPTIONS : abort_on_error=1\n +\n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n @@ t/test-lib.sh: then\n # before we even do our \"did we build git yet\" check (since we don't\n # want that one to complain to stderr).\n -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n -+prepend_var ASAN_OPTIONS : $XSAN_OPTIONS\n ++prepend_var ASAN_OPTIONS : $GIT_XSAN_OPTIONS\n +prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n -# If LSAN is in effect we _do_ want leak checking, but we still\n -# want to abort so that we notice the problems.\n -: ${LSAN_OPTIONS=abort_on_error=1}\n -+prepend_var LSAN_OPTIONS : $XSAN_OPTIONS\n ++prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-: ----------- > 2: 0c2867e30dc test-lib: correct commentary on TEST_DIRECTORY overriding\n2: 4c53c6157ac ! 3: 229654027b8 test-lib: make $GIT_BUILD_DIR an absolute path\n @@ Commit message\n \"/path/to/build\". The \"TEST_DIRECTORY\" here is already made an\n absolute path a few lines above this.\n \n - This will be helpful to LSAN_OPTIONS which will want to strip the\n - build directory path from filenames, which we couldn't do if we had a\n - \"/..\" in there.\n + We could simply do $(cd \"$TEST_DIRECTORY\"/..\" && pwd) here, but as\n + noted in the preceding commit the \"$TEST_DIRECTORY\" can't be anything\n + except the path containing this test-lib.sh file at this point, so we\n + can more cheaply and equally strip the \"/t\" off the end.\n +\n + This change will be helpful to LSAN_OPTIONS which will want to strip\n + the build directory path from filenames, which we couldn't do if we\n + had a \"/..\" in there.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ t/test-lib.sh: then\n +GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n \n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n - # adding the delimiter if VAR is empty. I.e. a generalized:\n + # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n +@@ t/test-lib.sh: prepend_var () {\n + # problems. The GIT_XSAN_OPTIONS variable can be used to set common\n + # defaults shared between [AL]SAN_OPTIONS.\n + prepend_var GIT_XSAN_OPTIONS : abort_on_error=1\n ++prepend_var GIT_XSAN_OPTIONS : strip_path_prefix=\\\"$GIT_BUILD_DIR/\\\"\n + \n + # If we were built with ASAN, it may complain about leaks\n + # of program-lifetime variables. Disable it by default to lower\n3: ec852e6d691 < -: ----------- test-lib: add \"strip_path_prefix\" to XSAN_OPTIONS\n4: 2becf76a14a ! 4: e6a48b6e4ce test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS\n @@ t/test-lib.sh\n @@ t/test-lib.sh: prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n - prepend_var LSAN_OPTIONS : $XSAN_OPTIONS\n + prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS\n +prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n-- \n2.35.1.1130.g7c6dd716f26\n\n"},{"id":"448852","messageId":"patch-v2-2.4-0c2867e30dc-20220219T112653Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com","subject":"[PATCH v2 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T11:29:41Z","receivedAt":"2022-02-19T11:30:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Correct a misleading comment added by me in 62f539043c7 (test-lib:\nAllow overriding of TEST_DIRECTORY, 2010-08-19).\n\nBetween that comment and the later addition of\n85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,\n2013-11-17) the comments were on the wrong arms of the \"if\". I.e. the\n\"allow tests to override this\" was on the \"test -z\" arm.\n\nBut more importantly this could be read allowing the \"$TEST_DIRECTORY\"\nto be some path outside of t/. As explained in the updated comment\nthat's impossible, rather it was meant for *tests* that ran outside of\nt/, i.e. the \"t0000-basic.sh\" tests that use \"lib-subtest.sh\".\n\nThose tests have a different working directory, but they set the\n\"TEST_DIRECTORY\" to the same path for bootstrapping. The comments now\nreflect that, and further comment on why we have a hard dependency on\nthis.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7e6978d1817..8fa7379e128 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -19,13 +19,20 @@\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n if test -z \"$TEST_DIRECTORY\"\n then\n-\t# We allow tests to override this, in case they want to run tests\n-\t# outside of t/, e.g. for running tests on the test library\n-\t# itself.\n-\tTEST_DIRECTORY=$(pwd)\n-else\n \t# ensure that TEST_DIRECTORY is an absolute path so that it\n \t# is valid even if the current working directory is changed\n+\tTEST_DIRECTORY=$(pwd)\n+else\n+\t# The TEST_DIRECTORY will always be the path to the \"t\"\n+\t# directory in the git.git checkout. This is overridden by\n+\t# e.g. t/lib-subtest.sh, but only because its $(pwd) is\n+\t# different. Those tests still set \"$TEST_DIRECTORY\" to the\n+\t# same path.\n+\t#\n+\t# See use of \"$GIT_BUILD_DIR\" and \"$TEST_DIRECTORY\" below for\n+\t# hard assumptions about \"$GIT_BUILD_DIR/t\" existing and being\n+\t# the \"$TEST_DIRECTORY\", and e.g. \"$TEST_DIRECTORY/helper\"\n+\t# needing to exist.\n \tTEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) || exit 1\n fi\n if test -z \"$TEST_OUTPUT_DIRECTORY\"\n-- \n2.35.1.1130.g7c6dd716f26\n\n"},{"id":"448853","messageId":"patch-v2-3.4-229654027b8-20220219T112653Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com","subject":"[PATCH v2 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T11:29:42Z","receivedAt":"2022-02-19T11:30:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GIT_BUILD_DIR from a path like \"/path/to/build/t/..\" to\n\"/path/to/build\". The \"TEST_DIRECTORY\" here is already made an\nabsolute path a few lines above this.\n\nWe could simply do $(cd \"$TEST_DIRECTORY\"/..\" && pwd) here, but as\nnoted in the preceding commit the \"$TEST_DIRECTORY\" can't be anything\nexcept the path containing this test-lib.sh file at this point, so we\ncan more cheaply and equally strip the \"/t\" off the end.\n\nThis change will be helpful to LSAN_OPTIONS which will want to strip\nthe build directory path from filenames, which we couldn't do if we\nhad a \"/..\" in there.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8fa7379e128..80944035f2c 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -41,7 +41,7 @@ then\n \t# elsewhere\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n-GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n+GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n \n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n@@ -59,6 +59,7 @@ prepend_var () {\n # problems. The GIT_XSAN_OPTIONS variable can be used to set common\n # defaults shared between [AL]SAN_OPTIONS.\n prepend_var GIT_XSAN_OPTIONS : abort_on_error=1\n+prepend_var GIT_XSAN_OPTIONS : strip_path_prefix=\\\"$GIT_BUILD_DIR/\\\"\n \n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n-- \n2.35.1.1130.g7c6dd716f26\n\n"},{"id":"448854","messageId":"patch-v2-4.4-e6a48b6e4ce-20220219T112653Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com","subject":"[PATCH v2 4/4] test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T11:29:43Z","receivedAt":"2022-02-19T11:30:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS to get more meaningful\nstack traces from LSAN. This isn't required under ASAN which will emit\ntraces such as this one for a leak in \"t/t0006-date.sh\":\n\n $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd\n [...]\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)\n #1 0x9444a4 in xstrdup wrapper.c:29:14\n #2 0x5995fa in parse_date_format date.c:991:24\n #3 0x4d2056 in show_dates t/helper/test-date.c:39:2\n #4 0x4d174a in cmd__date t/helper/test-date.c:116:3\n #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11\n #6 0x4cd1e3 in main common-main.c:52:11\n #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n #8 0x422b09 in _start (t/helper/test-tool+0x422b09)\n\n SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nWhereas LSAN would emit this instead:\n\n $ ./t0006-date.sh -vixd\n [...]\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n #1 0x7f2be1d614aa in strdup string/strdup.c:42:15\n\n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nNow we'll instead git this sensible stack trace under\nLSAN. I.e. almost the same one (but starting with \"malloc\", as is\nusual for LSAN) as under ASAN:\n\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n #1 0x7f012af5c4aa in strdup string/strdup.c:42:15\n #2 0x5cb164 in xstrdup wrapper.c:29:14\n #3 0x495ee9 in parse_date_format date.c:991:24\n #4 0x453aac in show_dates t/helper/test-date.c:39:2\n #5 0x453782 in cmd__date t/helper/test-date.c:116:3\n #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11\n #7 0x451f1e in main common-main.c:52:11\n #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)\n\n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nAs the option name suggests this does make things slower, e.g. for\nt0001-init.sh we're around 10% slower:\n\n $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3\n Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh\n Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]\n Range (min … max): 2.122 s … 2.152 s 3 runs\n\n Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh\n Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]\n Range (min … max): 1.941 s … 2.044 s 3 runs\n\n Summary\n 'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran\n 1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'\n\nI think that's more than worth it to get the more meaningful stack\ntraces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for\none-off \"fast\" runs.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 80944035f2c..129668c685f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -71,6 +71,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS\n+prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-- \n2.35.1.1130.g7c6dd716f26\n\n"},{"id":"448894","messageId":"xmqqh78uvut1.fsf@gitster.g","threadId":"57441","inReplyTo":"patch-v2-1.4-01e63a72231-20220219T112653Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/4] test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-20T07:52:42Z","receivedAt":"2022-02-20T07:52:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\n> variables, rather than punting out entirely if we already have them in\n> the environment.\n>\n> We do want to take any user-provided settings over our own, but we can\n> do do that by prepending our defaults to the variable. The\n\ndo do do?\n\n> libsanitizer options parsing has \"last option wins\" semantics.\n> ...\n> +# If [AL]SAN is in effect we want to abort so that we notice\n> +# problems. The GIT_XSAN_OPTIONS variable can be used to set common\n\nAs we say GIT_ prefix, we can just drop X. \"Git's sanitizer options\".\n\n"},{"id":"448942","messageId":"YhL5YTrbcWXv2PBo@nand.local","threadId":"57441","inReplyTo":"cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/4] test-lib: improve LSAN + ASAN stack traces","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T02:30:57Z","receivedAt":"2022-02-21T02:31:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 19, 2022 at 12:29:39PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> I think this v2 should address all the comments on the v1, thanks\n> Taylor & Junio!\n\nThanks; this version looks good to me, though I agree with Junio's\nfeedback on the first patch. TBH, I do not feel strongly at all about\n\"GIT_XSAN_OPTIONS\" versus \"GIT_SAN_OPTIONS\", either seems fine to (with\na slight bias towards the first, since it makes it clear that it targets\nboth ASan and LSan).\n\nBut either way, this looks like it's almost there.\n\nThanks,\nTaylor\n"},{"id":"449009","messageId":"patch-v3-1.4-bf31efca464-20220221T155656Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com","subject":"[PATCH v3 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T15:58:32Z","receivedAt":"2022-02-21T15:59:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\nvariables, rather than punting out entirely if we already have them in\nthe environment.\n\nWe want to take any user-provided settings over our own, but we can do\nthat by prepending our defaults to the variable. The libsanitizer\noptions parsing has \"last option wins\" semantics.\n\nIt's now possible to do e.g.:\n\n LSAN_OPTIONS=report_objects=1 ./t0006-date.sh\n\nAnd not have the \"report_objects=1\" setting overwrite our sensible\ndefault of \"abort_on_error=1\", but by prepending to the list we ensure\nthat:\n\n LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh\n\nWill take the desired \"abort_on_error=0\" over our default.\n\nSee b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)\nfor the original pattern being altered here, and\n85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,\n2017-09-05) for when LSAN_OPTIONS was added in addition to the\nthen-existing ASAN_OPTIONS.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 24 ++++++++++++++++++++----\n 1 file changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e4716b0b867..55f263a02d3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -36,17 +36,33 @@ then\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n+# Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n+# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n+#\n+#\tVAR=$1${VAR:+${1:+$2}$VAR}\n+#\n+# Usage (using \":\" as the $2 delimiter):\n+#\n+#\tprepend_var VAR : VALUE\n+prepend_var () {\n+\teval \"$1=$3\\${$1:+${3:+$2}\\$$1}\"\n+}\n+\n+# If [AL]SAN is in effect we want to abort so that we notice\n+# problems. The GIT_SAN_OPTIONS variable can be used to set common\n+# defaults shared between [AL]SAN_OPTIONS.\n+prepend_var GIT_SAN_OPTIONS : abort_on_error=1\n+\n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n # the noise level. This needs to happen at the start of the script,\n # before we even do our \"did we build git yet\" check (since we don't\n # want that one to complain to stderr).\n-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n+prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS\n+prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n-# If LSAN is in effect we _do_ want leak checking, but we still\n-# want to abort so that we notice the problems.\n-: ${LSAN_OPTIONS=abort_on_error=1}\n+prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-- \n2.35.1.1132.ga1fe46f8690\n\n"},{"id":"449010","messageId":"cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com","subject":"[PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T15:58:31Z","receivedAt":"2022-02-21T15:59:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A UX improvement for [AL]SAN stack traces. See\nhttps://lore.kernel.org/git/cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com/\nfor the v2.\n\nChanges:\n * Renamed GIT_XSAN_OPTIONS to GIT_SAN_OPTIONS per Junio's suggestion\n * Fixed grammar (\"do do do...\")\" in commit message.\n\nÆvar Arnfjörð Bjarmason (4):\n test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS\n test-lib: correct commentary on TEST_DIRECTORY overriding\n test-lib: make $GIT_BUILD_DIR an absolute path\n test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS\n\n t/test-lib.sh | 45 +++++++++++++++++++++++++++++++++++----------\n 1 file changed, 35 insertions(+), 10 deletions(-)\n\nRange-diff against v2:\n1: 01e63a72231 ! 1: bf31efca464 test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - test-lib: add GIT_XSAN_OPTIONS, inherit [AL]SAN_OPTIONS\n + test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS\n \n Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\n variables, rather than punting out entirely if we already have them in\n the environment.\n \n - We do want to take any user-provided settings over our own, but we can\n - do do that by prepending our defaults to the variable. The\n - libsanitizer options parsing has \"last option wins\" semantics.\n + We want to take any user-provided settings over our own, but we can do\n + that by prepending our defaults to the variable. The libsanitizer\n + options parsing has \"last option wins\" semantics.\n \n It's now possible to do e.g.:\n \n @@ t/test-lib.sh: then\n +}\n +\n +# If [AL]SAN is in effect we want to abort so that we notice\n -+# problems. The GIT_XSAN_OPTIONS variable can be used to set common\n ++# problems. The GIT_SAN_OPTIONS variable can be used to set common\n +# defaults shared between [AL]SAN_OPTIONS.\n -+prepend_var GIT_XSAN_OPTIONS : abort_on_error=1\n ++prepend_var GIT_SAN_OPTIONS : abort_on_error=1\n +\n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n @@ t/test-lib.sh: then\n # before we even do our \"did we build git yet\" check (since we don't\n # want that one to complain to stderr).\n -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n -+prepend_var ASAN_OPTIONS : $GIT_XSAN_OPTIONS\n ++prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS\n +prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n -# If LSAN is in effect we _do_ want leak checking, but we still\n -# want to abort so that we notice the problems.\n -: ${LSAN_OPTIONS=abort_on_error=1}\n -+prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS\n ++prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n2: 0c2867e30dc = 2: 33a628e9c3a test-lib: correct commentary on TEST_DIRECTORY overriding\n3: 229654027b8 ! 3: b03ae29fc92 test-lib: make $GIT_BUILD_DIR an absolute path\n @@ t/test-lib.sh: then\n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n @@ t/test-lib.sh: prepend_var () {\n - # problems. The GIT_XSAN_OPTIONS variable can be used to set common\n + # problems. The GIT_SAN_OPTIONS variable can be used to set common\n # defaults shared between [AL]SAN_OPTIONS.\n - prepend_var GIT_XSAN_OPTIONS : abort_on_error=1\n -+prepend_var GIT_XSAN_OPTIONS : strip_path_prefix=\\\"$GIT_BUILD_DIR/\\\"\n + prepend_var GIT_SAN_OPTIONS : abort_on_error=1\n ++prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\\\"$GIT_BUILD_DIR/\\\"\n \n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n4: e6a48b6e4ce ! 4: d212009e517 test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS\n @@ t/test-lib.sh\n @@ t/test-lib.sh: prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n - prepend_var LSAN_OPTIONS : $GIT_XSAN_OPTIONS\n + prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n +prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n-- \n2.35.1.1132.ga1fe46f8690\n\n"},{"id":"449011","messageId":"patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com","subject":"[PATCH v3 2/4] test-lib: correct commentary on TEST_DIRECTORY overriding","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T15:58:33Z","receivedAt":"2022-02-21T15:59:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Correct a misleading comment added by me in 62f539043c7 (test-lib:\nAllow overriding of TEST_DIRECTORY, 2010-08-19).\n\nBetween that comment and the later addition of\n85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,\n2013-11-17) the comments were on the wrong arms of the \"if\". I.e. the\n\"allow tests to override this\" was on the \"test -z\" arm.\n\nBut more importantly this could be read allowing the \"$TEST_DIRECTORY\"\nto be some path outside of t/. As explained in the updated comment\nthat's impossible, rather it was meant for *tests* that ran outside of\nt/, i.e. the \"t0000-basic.sh\" tests that use \"lib-subtest.sh\".\n\nThose tests have a different working directory, but they set the\n\"TEST_DIRECTORY\" to the same path for bootstrapping. The comments now\nreflect that, and further comment on why we have a hard dependency on\nthis.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 55f263a02d3..77c3fabd041 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -19,13 +19,20 @@\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n if test -z \"$TEST_DIRECTORY\"\n then\n-\t# We allow tests to override this, in case they want to run tests\n-\t# outside of t/, e.g. for running tests on the test library\n-\t# itself.\n-\tTEST_DIRECTORY=$(pwd)\n-else\n \t# ensure that TEST_DIRECTORY is an absolute path so that it\n \t# is valid even if the current working directory is changed\n+\tTEST_DIRECTORY=$(pwd)\n+else\n+\t# The TEST_DIRECTORY will always be the path to the \"t\"\n+\t# directory in the git.git checkout. This is overridden by\n+\t# e.g. t/lib-subtest.sh, but only because its $(pwd) is\n+\t# different. Those tests still set \"$TEST_DIRECTORY\" to the\n+\t# same path.\n+\t#\n+\t# See use of \"$GIT_BUILD_DIR\" and \"$TEST_DIRECTORY\" below for\n+\t# hard assumptions about \"$GIT_BUILD_DIR/t\" existing and being\n+\t# the \"$TEST_DIRECTORY\", and e.g. \"$TEST_DIRECTORY/helper\"\n+\t# needing to exist.\n \tTEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) || exit 1\n fi\n if test -z \"$TEST_OUTPUT_DIRECTORY\"\n-- \n2.35.1.1132.ga1fe46f8690\n\n"},{"id":"449012","messageId":"patch-v3-3.4-b03ae29fc92-20220221T155656Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com","subject":"[PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T15:58:34Z","receivedAt":"2022-02-21T15:59:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GIT_BUILD_DIR from a path like \"/path/to/build/t/..\" to\n\"/path/to/build\". The \"TEST_DIRECTORY\" here is already made an\nabsolute path a few lines above this.\n\nWe could simply do $(cd \"$TEST_DIRECTORY\"/..\" && pwd) here, but as\nnoted in the preceding commit the \"$TEST_DIRECTORY\" can't be anything\nexcept the path containing this test-lib.sh file at this point, so we\ncan more cheaply and equally strip the \"/t\" off the end.\n\nThis change will be helpful to LSAN_OPTIONS which will want to strip\nthe build directory path from filenames, which we couldn't do if we\nhad a \"/..\" in there.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 77c3fabd041..ff13321bfd3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -41,7 +41,7 @@ then\n \t# elsewhere\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n-GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n+GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n \n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n@@ -59,6 +59,7 @@ prepend_var () {\n # problems. The GIT_SAN_OPTIONS variable can be used to set common\n # defaults shared between [AL]SAN_OPTIONS.\n prepend_var GIT_SAN_OPTIONS : abort_on_error=1\n+prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\\\"$GIT_BUILD_DIR/\\\"\n \n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n-- \n2.35.1.1132.ga1fe46f8690\n\n"},{"id":"449013","messageId":"patch-v3-4.4-d212009e517-20220221T155656Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com","subject":"[PATCH v3 4/4] test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T15:58:35Z","receivedAt":"2022-02-21T15:59:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS to get more meaningful\nstack traces from LSAN. This isn't required under ASAN which will emit\ntraces such as this one for a leak in \"t/t0006-date.sh\":\n\n $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd\n [...]\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)\n #1 0x9444a4 in xstrdup wrapper.c:29:14\n #2 0x5995fa in parse_date_format date.c:991:24\n #3 0x4d2056 in show_dates t/helper/test-date.c:39:2\n #4 0x4d174a in cmd__date t/helper/test-date.c:116:3\n #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11\n #6 0x4cd1e3 in main common-main.c:52:11\n #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n #8 0x422b09 in _start (t/helper/test-tool+0x422b09)\n\n SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nWhereas LSAN would emit this instead:\n\n $ ./t0006-date.sh -vixd\n [...]\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n #1 0x7f2be1d614aa in strdup string/strdup.c:42:15\n\n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nNow we'll instead git this sensible stack trace under\nLSAN. I.e. almost the same one (but starting with \"malloc\", as is\nusual for LSAN) as under ASAN:\n\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n #1 0x7f012af5c4aa in strdup string/strdup.c:42:15\n #2 0x5cb164 in xstrdup wrapper.c:29:14\n #3 0x495ee9 in parse_date_format date.c:991:24\n #4 0x453aac in show_dates t/helper/test-date.c:39:2\n #5 0x453782 in cmd__date t/helper/test-date.c:116:3\n #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11\n #7 0x451f1e in main common-main.c:52:11\n #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)\n\n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nAs the option name suggests this does make things slower, e.g. for\nt0001-init.sh we're around 10% slower:\n\n $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3\n Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh\n Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]\n Range (min … max): 2.122 s … 2.152 s 3 runs\n\n Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh\n Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]\n Range (min … max): 1.941 s … 2.044 s 3 runs\n\n Summary\n 'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran\n 1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'\n\nI think that's more than worth it to get the more meaningful stack\ntraces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for\none-off \"fast\" runs.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ff13321bfd3..a96d19a332e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -71,6 +71,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n+prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-- \n2.35.1.1132.ga1fe46f8690\n\n"},{"id":"449021","messageId":"xmqqo830ta0w.fsf@gitster.g","threadId":"57441","inReplyTo":"cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com","subject":"Re: [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-21T17:16:47Z","receivedAt":"2022-02-21T17:18:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> A UX improvement for [AL]SAN stack traces. See\n> https://lore.kernel.org/git/cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com/\n> for the v2.\n>\n> Changes:\n> * Renamed GIT_XSAN_OPTIONS to GIT_SAN_OPTIONS per Junio's suggestion\n\nHeh, I wasn't suggesting anything. I am OK with or without X, or\nwith X but X replaced with anything else ;-)\n"},{"id":"449022","messageId":"YhPL+wSxtI0KIz07@nand.local","threadId":"57441","inReplyTo":"patch-v3-3.4-b03ae29fc92-20220221T155656Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T17:29:31Z","receivedAt":"2022-02-21T17:30:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 21, 2022 at 04:58:34PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 77c3fabd041..ff13321bfd3 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -41,7 +41,7 @@ then\n> \t# elsewhere\n> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n> fi\n> -GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n> +GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n\nSorry to notice this so late, but this hunk caught my eye. What happens\nif `TEST_DIRECTORY` is provided by the user (and doesn't end in \"/t\")?\n\nBefore this change, we would have set GIT_BUILD_DIR to the parent of\nwhatever TEST_DIRECTORY is, whether or not it ended in \"/t\". We'll still\ndo the same thing with this patch if TEST_DIRECTORY ends in \"/t\". But if\nit doesn't, then we'll set GIT_BUILD_DIR to be the same as\nTEST_DIRECTORY, which is a behavior change.\n\nIf you just want GIT_BUILD_DIR to be absolute in order to for LSan to\nunderstand how to correctly strip it out of filenames, could we replace\nthis with:\n\n GIT_BUILD_DIR=\"$(cd \"$TEST_DIRECTORY/..\" && pwd)\"\n\nor (an admittedly less clear)\n\n GIT_BUILD_DIR=\"$(dirname \"$TEST_DIRECTORY\")\"\n\nThanks,\nTaylor\n"},{"id":"449023","messageId":"YhPMt3HwSsErvM0l@nand.local","threadId":"57441","inReplyTo":"patch-v3-4.4-d212009e517-20220221T155656Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/4] test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T17:32:39Z","receivedAt":"2022-02-21T17:32:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 21, 2022 at 04:58:35PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS to get more meaningful\n> stack traces from LSAN. This isn't required under ASAN which will emit\n> traces such as this one for a leak in \"t/t0006-date.sh\":\n>\n> $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd\n> [...]\n> Direct leak of 3 byte(s) in 1 object(s) allocated from:\n> #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)\n> #1 0x9444a4 in xstrdup wrapper.c:29:14\n> #2 0x5995fa in parse_date_format date.c:991:24\n> #3 0x4d2056 in show_dates t/helper/test-date.c:39:2\n> #4 0x4d174a in cmd__date t/helper/test-date.c:116:3\n> #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11\n> #6 0x4cd1e3 in main common-main.c:52:11\n> #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n> #8 0x422b09 in _start (t/helper/test-tool+0x422b09)\n>\n> SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).\n> Aborted\n>\n> Whereas LSAN would emit this instead:\n>\n> $ ./t0006-date.sh -vixd\n> [...]\n> Direct leak of 3 byte(s) in 1 object(s) allocated from:\n> #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n> #1 0x7f2be1d614aa in strdup string/strdup.c:42:15\n>\n> SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n> Aborted\n>\n> Now we'll instead git this sensible stack trace under\n> LSAN. I.e. almost the same one (but starting with \"malloc\", as is\n> usual for LSAN) as under ASAN:\n>\n> Direct leak of 3 byte(s) in 1 object(s) allocated from:\n> #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n> #1 0x7f012af5c4aa in strdup string/strdup.c:42:15\n> #2 0x5cb164 in xstrdup wrapper.c:29:14\n> #3 0x495ee9 in parse_date_format date.c:991:24\n> #4 0x453aac in show_dates t/helper/test-date.c:39:2\n> #5 0x453782 in cmd__date t/helper/test-date.c:116:3\n> #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11\n> #7 0x451f1e in main common-main.c:52:11\n> #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n> #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)\n>\n> SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n> Aborted\n\nThis is great, by the way. I have often hit that bug in LSan and been\nincredibly frustrated by it. I'm happy to see it getting fixed here,\nthank you.\n\n> As the option name suggests this does make things slower, e.g. for\n> t0001-init.sh we're around 10% slower:\n>\n> $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3\n> Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh\n> Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]\n> Range (min … max): 2.122 s … 2.152 s 3 runs\n>\n> Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh\n> Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]\n> Range (min … max): 1.941 s … 2.044 s 3 runs\n>\n> Summary\n> 'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran\n> 1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'\n>\n> I think that's more than worth it to get the more meaningful stack\n> traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for\n> one-off \"fast\" runs.\n\nI completely agree. I am almost always run ASan / LSan tests a single\nscript at a time (often focusing on just one script that I know\ndemonstrates some bug).\n\nAt GitHub, we use both a sanitized and un-sanitized build when running\nCI. So we'll probably feel the effects a little more during the \"run\nmake test under a sanitized build\" CI job, but we could easily set\nfast_unwind_on_malloc=0 if it becomes too big of a problem for us\n(though I suspect it won't matter in practice).\n\nThanks,\nTaylor\n"},{"id":"449038","messageId":"220221.86czjgaw2e.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"xmqqo830ta0w.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] test-lib: improve LSAN + ASAN stack traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T18:55:24Z","receivedAt":"2022-02-21T18:55:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 21 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> A UX improvement for [AL]SAN stack traces. See\n>> https://lore.kernel.org/git/cover-v2-0.4-00000000000-20220219T112653Z-avarab@gmail.com/\n>> for the v2.\n>>\n>> Changes:\n>> * Renamed GIT_XSAN_OPTIONS to GIT_SAN_OPTIONS per Junio's suggestion\n>\n> Heh, I wasn't suggesting anything. I am OK with or without X, or\n> with X but X replaced with anything else ;-)\n\nAh, anyway I'm happy with this version, so unless there's another reason\nfor a re-roll...\n"},{"id":"449040","messageId":"220221.868ru4avw6.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"YhPL+wSxtI0KIz07@nand.local","subject":"Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T18:55:50Z","receivedAt":"2022-02-21T19:00:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 21 2022, Taylor Blau wrote:\n\n> On Mon, Feb 21, 2022 at 04:58:34PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 77c3fabd041..ff13321bfd3 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -41,7 +41,7 @@ then\n>> \t# elsewhere\n>> \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n>> fi\n>> -GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n>> +GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n>\n> Sorry to notice this so late, but this hunk caught my eye. What happens\n> if `TEST_DIRECTORY` is provided by the user (and doesn't end in \"/t\")?\n\nI think that the preceding 2/4 should cover your concern here, i.e. I\nthink that's not possible.\n\n> Before this change, we would have set GIT_BUILD_DIR to the parent of\n> whatever TEST_DIRECTORY is, whether or not it ended in \"/t\". We'll still\n> do the same thing with this patch if TEST_DIRECTORY ends in \"/t\". But if\n> it doesn't, then we'll set GIT_BUILD_DIR to be the same as\n> TEST_DIRECTORY, which is a behavior change.\n\nIndeed, but I believe (again see 2/4) that can't happen.\n\n> If you just want GIT_BUILD_DIR to be absolute in order to for LSan to\n> understand how to correctly strip it out of filenames, could we replace\n> this with:\n>\n> GIT_BUILD_DIR=\"$(cd \"$TEST_DIRECTORY/..\" && pwd)\"\n>\n> or (an admittedly less clear)\n>\n> GIT_BUILD_DIR=\"$(dirname \"$TEST_DIRECTORY\")\"\n\nYes. I almost changed it to the former in this re-roll, but as noted in\n<220219.86y227fvz1.gmgdl@evledraar.gmail.com> thought it was better to\nhave it clear that this really wasn't a generic mechanism, we really do\nneed/want the actual \"t\" directory here.\n\n"},{"id":"449042","messageId":"220221.864k4sav0t.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"YhPMt3HwSsErvM0l@nand.local","subject":"Re: [PATCH v3 4/4] test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T18:59:45Z","receivedAt":"2022-02-21T19:18:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 21 2022, Taylor Blau wrote:\n\n> On Mon, Feb 21, 2022 at 04:58:35PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> Add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS to get more meaningful\n>> stack traces from LSAN. This isn't required under ASAN which will emit\n>> traces such as this one for a leak in \"t/t0006-date.sh\":\n>>\n>> $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd\n>> [...]\n>> Direct leak of 3 byte(s) in 1 object(s) allocated from:\n>> #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)\n>> #1 0x9444a4 in xstrdup wrapper.c:29:14\n>> #2 0x5995fa in parse_date_format date.c:991:24\n>> #3 0x4d2056 in show_dates t/helper/test-date.c:39:2\n>> #4 0x4d174a in cmd__date t/helper/test-date.c:116:3\n>> #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11\n>> #6 0x4cd1e3 in main common-main.c:52:11\n>> #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n>> #8 0x422b09 in _start (t/helper/test-tool+0x422b09)\n>>\n>> SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).\n>> Aborted\n>>\n>> Whereas LSAN would emit this instead:\n>>\n>> $ ./t0006-date.sh -vixd\n>> [...]\n>> Direct leak of 3 byte(s) in 1 object(s) allocated from:\n>> #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n>> #1 0x7f2be1d614aa in strdup string/strdup.c:42:15\n>>\n>> SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n>> Aborted\n>>\n>> Now we'll instead git this sensible stack trace under\n>> LSAN. I.e. almost the same one (but starting with \"malloc\", as is\n>> usual for LSAN) as under ASAN:\n>>\n>> Direct leak of 3 byte(s) in 1 object(s) allocated from:\n>> #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n>> #1 0x7f012af5c4aa in strdup string/strdup.c:42:15\n>> #2 0x5cb164 in xstrdup wrapper.c:29:14\n>> #3 0x495ee9 in parse_date_format date.c:991:24\n>> #4 0x453aac in show_dates t/helper/test-date.c:39:2\n>> #5 0x453782 in cmd__date t/helper/test-date.c:116:3\n>> #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11\n>> #7 0x451f1e in main common-main.c:52:11\n>> #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n>> #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)\n>>\n>> SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n>> Aborted\n>\n> This is great, by the way. I have often hit that bug in LSan and been\n> incredibly frustrated by it. I'm happy to see it getting fixed here,\n> thank you.\n\nCheers!\n\n>> As the option name suggests this does make things slower, e.g. for\n>> t0001-init.sh we're around 10% slower:\n>>\n>> $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3\n>> Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh\n>> Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]\n>> Range (min … max): 2.122 s … 2.152 s 3 runs\n>>\n>> Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh\n>> Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]\n>> Range (min … max): 1.941 s … 2.044 s 3 runs\n>>\n>> Summary\n>> 'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran\n>> 1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'\n>>\n>> I think that's more than worth it to get the more meaningful stack\n>> traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for\n>> one-off \"fast\" runs.\n>\n> I completely agree. I am almost always run ASan / LSan tests a single\n> script at a time (often focusing on just one script that I know\n> demonstrates some bug).\n>\n> At GitHub, we use both a sanitized and un-sanitized build when running\n> CI. So we'll probably feel the effects a little more during the \"run\n> make test under a sanitized build\" CI job, but we could easily set\n> fast_unwind_on_malloc=0 if it becomes too big of a problem for us\n> (though I suspect it won't matter in practice).\n\nI suspect you mean SANITIZE=address not SANITIZE=leak, i.e. ASAN not\nLSAN. Note that the performance of the two is often quite different\n(with ASAN starting out much slower).\n\nI almost never use ASAN, but I've been using LSAN a lot.\n\nI also haven't been able to have ASAN spew out these worse stack traces,\nas noted in the commit message. But in some brief testing now if I were\nsetting fast_unwind_on_malloc=0 it would be around 15% slower:\n \n $ hyperfine -L v 0,1 'ASAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 5\n Benchmark 1: ASAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh\n Time (mean ± σ): 3.264 s ± 0.014 s [User: 2.456 s, System: 0.927 s]\n Range (min … max): 3.249 s … 3.282 s 5 runs\n \n Benchmark 2: ASAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh\n Time (mean ± σ): 2.826 s ± 0.023 s [User: 2.008 s, System: 0.936 s]\n Range (min … max): 2.814 s … 2.866 s 5 runs\n \n Summary\n 'ASAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran\n 1.15 ± 0.01 times faster than 'ASAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'\n\nBut I don't know in the ASAN case what, if anything, you're getting for\nthat reduction in performance.\n"},{"id":"449090","messageId":"xmqqee3vwepd.fsf@gitster.g","threadId":"57441","inReplyTo":"220221.868ru4avw6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-22T07:19:42Z","receivedAt":"2022-02-22T07:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Sorry to notice this so late, but this hunk caught my eye. What happens\n>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in \"/t\")?\n>\n> I think that the preceding 2/4 should cover your concern here, i.e. I\n> think that's not possible.\n>\n>> Before this change, we would have set GIT_BUILD_DIR to the parent of\n>> whatever TEST_DIRECTORY is, whether or not it ended in \"/t\". We'll still\n>> do the same thing with this patch if TEST_DIRECTORY ends in \"/t\". But if\n>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as\n>> TEST_DIRECTORY, which is a behavior change.\n>\n> Indeed, but I believe (again see 2/4) that can't happen.\n\nIt is not like \"can't happen\", but \"whoever wrote the TEST_DIRECTORY\noverride logic did not mean to support such a use case\".\n\nI am perfectly fine if we declared that we do not to support the use\nof that override mechanism by anybody but the \"subtest\" thing we do\nourselves. If we can catch a workflow that misuses the mechansim\ncheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set\nand it does not end in \"/t\"), we should do so, I would think, instead\nof doing the \"go up and do pwd\", which will make things worse.\n\nThanks.\n"},{"id":"449098","messageId":"220222.86o82z8als.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"xmqqee3vwepd.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-22T10:14:17Z","receivedAt":"2022-02-22T10:22:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 21 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> Sorry to notice this so late, but this hunk caught my eye. What happens\n>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in \"/t\")?\n>>\n>> I think that the preceding 2/4 should cover your concern here, i.e. I\n>> think that's not possible.\n>>\n>>> Before this change, we would have set GIT_BUILD_DIR to the parent of\n>>> whatever TEST_DIRECTORY is, whether or not it ended in \"/t\". We'll still\n>>> do the same thing with this patch if TEST_DIRECTORY ends in \"/t\". But if\n>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as\n>>> TEST_DIRECTORY, which is a behavior change.\n>>\n>> Indeed, but I believe (again see 2/4) that can't happen.\n>\n> It is not like \"can't happen\", but \"whoever wrote the TEST_DIRECTORY\n> override logic did not mean to support such a use case\".\n\nTo clarify with \"can't happen\" I mean (and should have said) that \"can't\nwork\", i.e. it would error out anyway.\n\nE.g. try in a git.git checkout:\n \n (\n mv t t2 &&\n cd t &&\n ./t0001-init.sh\n )\n\nIt will die with:\n \n You need to build test-tool:\n Run \"make t/helper/test-tool\" in the source (toplevel) directory\n FATAL: Unexpected exit with code 1\n\nAnd if you were to manually patch test-lib.sh to get past that error it\nwould start erroring on e.g.:\n\n sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory\n\nAnd if you \"fix\" that it'll error out on something else.\n\nI.e. we'll have discovered that $(pwd)/.. must be our build directory,\nand we then construct paths by adding the string \"/t/[...]\" to that.\n\n> I am perfectly fine if we declared that we do not to support the use\n> of that override mechanism by anybody but the \"subtest\" thing we do\n> ourselves. If we can catch a workflow that misuses the mechansim\n> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set\n> and it does not end in \"/t\"), we should do so, I would think, instead\n> of doing the \"go up and do pwd\", which will make things worse.\n\nWhat I was going for in 2/4 in\nhttp://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com\nis that we've already declared that. I.e. test-lib.sh has various\nassumptions about appending \"/t/...\" to the build directory being a\nvalid way to get paths to various test-lib.sh-adjacent code.\n\nSo trimming off \"/t\" here with a string operation v.s. $(cd .. && pwd)\nis being consistent with that code.\n\nIt would be odd to make the bit at the top very generic, only to have\nthe reader keep reading and wonder how that generic mechanism and the\nsubsequent hardcoding of \"/t/[...]\" are supposed to work together.\n"},{"id":"449337","messageId":"xmqq4k4puyn9.fsf@gitster.g","threadId":"57441","inReplyTo":"220222.86o82z8als.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-23T20:16:26Z","receivedAt":"2022-02-23T20:16:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Mon, Feb 21 2022, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>>> Sorry to notice this so late, but this hunk caught my eye. What happens\n>>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in \"/t\")?\n>>>\n>>> I think that the preceding 2/4 should cover your concern here, i.e. I\n>>> think that's not possible.\n>>>\n>>>> Before this change, we would have set GIT_BUILD_DIR to the parent of\n>>>> whatever TEST_DIRECTORY is, whether or not it ended in \"/t\". We'll still\n>>>> do the same thing with this patch if TEST_DIRECTORY ends in \"/t\". But if\n>>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as\n>>>> TEST_DIRECTORY, which is a behavior change.\n>>>\n>>> Indeed, but I believe (again see 2/4) that can't happen.\n>>\n>> It is not like \"can't happen\", but \"whoever wrote the TEST_DIRECTORY\n>> override logic did not mean to support such a use case\".\n>\n> To clarify with \"can't happen\" I mean (and should have said) that \"can't\n> work\", i.e. it would error out anyway.\n>\n> E.g. try in a git.git checkout:\n> \n> (\n> mv t t2 &&\n> cd t &&\n> ./t0001-init.sh\n> )\n>\n> It will die with:\n> \n> You need to build test-tool:\n> Run \"make t/helper/test-tool\" in the source (toplevel) directory\n> FATAL: Unexpected exit with code 1\n>\n> And if you were to manually patch test-lib.sh to get past that error it\n> would start erroring on e.g.:\n>\n> sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory\n>\n> And if you \"fix\" that it'll error out on something else.\n>\n> I.e. we'll have discovered that $(pwd)/.. must be our build directory,\n> and we then construct paths by adding the string \"/t/[...]\" to that.\n>\n>> I am perfectly fine if we declared that we do not to support the use\n>> of that override mechanism by anybody but the \"subtest\" thing we do\n>> ourselves. If we can catch a workflow that misuses the mechansim\n>> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set\n>> and it does not end in \"/t\"), we should do so, I would think, instead\n>> of doing the \"go up and do pwd\", which will make things worse.\n>\n> What I was going for in 2/4 in\n> http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com\n> is that we've already declared that. I.e. test-lib.sh has various\n> assumptions about appending \"/t/...\" to the build directory being a\n> valid way to get paths to various test-lib.sh-adjacent code.\n>\n> So trimming off \"/t\" here with a string operation v.s. $(cd .. && pwd)\n> is being consistent with that code.\n>\n> It would be odd to make the bit at the top very generic, only to have\n> the reader keep reading and wonder how that generic mechanism and the\n> subsequent hardcoding of \"/t/[...]\" are supposed to work together.\n\nCorrect. That is why I said $(... pwd) to pretend that we can take\nanything would make it worse in a separate message.\n\nIf we have to strip off /t anyway, piggy-backing on that part to\ndetect and abort when somebody misused the mechanism would be a good\nidea---which is what I said in the message you are responding to and\nnot responding to.\n\n"},{"id":"449401","messageId":"220224.86bkywaakn.gmgdl@evledraar.gmail.com","threadId":"57441","inReplyTo":"xmqq4k4puyn9.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-24T09:14:51Z","receivedAt":"2022-02-24T09:16:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 23 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Mon, Feb 21 2022, Junio C Hamano wrote:\n>>\n>>> Ævar Arnfjörð Bjarmason writes:\n>>>\n>>>>> Sorry to notice this so late, but this hunk caught my eye. What happens\n>>>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in \"/t\")?\n>>>>\n>>>> I think that the preceding 2/4 should cover your concern here, i.e. I\n>>>> think that's not possible.\n>>>>\n>>>>> Before this change, we would have set GIT_BUILD_DIR to the parent of\n>>>>> whatever TEST_DIRECTORY is, whether or not it ended in \"/t\". We'll still\n>>>>> do the same thing with this patch if TEST_DIRECTORY ends in \"/t\". But if\n>>>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as\n>>>>> TEST_DIRECTORY, which is a behavior change.\n>>>>\n>>>> Indeed, but I believe (again see 2/4) that can't happen.\n>>>\n>>> It is not like \"can't happen\", but \"whoever wrote the TEST_DIRECTORY\n>>> override logic did not mean to support such a use case\".\n>>\n>> To clarify with \"can't happen\" I mean (and should have said) that \"can't\n>> work\", i.e. it would error out anyway.\n>>\n>> E.g. try in a git.git checkout:\n>> \n>> (\n>> mv t t2 &&\n>> cd t &&\n>> ./t0001-init.sh\n>> )\n>>\n>> It will die with:\n>> \n>> You need to build test-tool:\n>> Run \"make t/helper/test-tool\" in the source (toplevel) directory\n>> FATAL: Unexpected exit with code 1\n>>\n>> And if you were to manually patch test-lib.sh to get past that error it\n>> would start erroring on e.g.:\n>>\n>> sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory\n>>\n>> And if you \"fix\" that it'll error out on something else.\n>>\n>> I.e. we'll have discovered that $(pwd)/.. must be our build directory,\n>> and we then construct paths by adding the string \"/t/[...]\" to that.\n>>\n>>> I am perfectly fine if we declared that we do not to support the use\n>>> of that override mechanism by anybody but the \"subtest\" thing we do\n>>> ourselves. If we can catch a workflow that misuses the mechansim\n>>> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set\n>>> and it does not end in \"/t\"), we should do so, I would think, instead\n>>> of doing the \"go up and do pwd\", which will make things worse.\n>>\n>> What I was going for in 2/4 in\n>> http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com\n>> is that we've already declared that. I.e. test-lib.sh has various\n>> assumptions about appending \"/t/...\" to the build directory being a\n>> valid way to get paths to various test-lib.sh-adjacent code.\n>>\n>> So trimming off \"/t\" here with a string operation v.s. $(cd .. && pwd)\n>> is being consistent with that code.\n>>\n>> It would be odd to make the bit at the top very generic, only to have\n>> the reader keep reading and wonder how that generic mechanism and the\n>> subsequent hardcoding of \"/t/[...]\" are supposed to work together.\n>\n> Correct. That is why I said $(... pwd) to pretend that we can take\n> anything would make it worse in a separate message.\n>\n> If we have to strip off /t anyway, piggy-backing on that part to\n> detect and abort when somebody misused the mechanism would be a good\n> idea---which is what I said in the message you are responding to and\n> not responding to.\n\nSo you're OK with the assumption/method being used here, but would\nprefer if we also added an explicit check/\"exit 1\"? E.g.:\n\t\n\tif test \"$TEST_DIRECTORY\" = \"${TEST_DIRECTORY%/t}\"\n\tthen\n\t echo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n\t exit 1\n\tfi\n\n?\n"},{"id":"449502","messageId":"xmqqa6egkp2y.fsf@gitster.g","threadId":"57441","inReplyTo":"220224.86bkywaakn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-24T20:05:25Z","receivedAt":"2022-02-24T20:05:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> So you're OK with the assumption/method being used here, but would\n> prefer if we also added an explicit check/\"exit 1\"? E.g.:\n> \t\n> \tif test \"$TEST_DIRECTORY\" = \"${TEST_DIRECTORY%/t}\"\n> \tthen\n> \t echo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n> \t exit 1\n> \tfi\n\nExactly.\n"},{"id":"449686","messageId":"cover-v4-0.4-00000000000-20220227T102256Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com","subject":"[PATCH v4 0/4] test-lib: improve LSAN + ASAN stack traces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-27T10:25:09Z","receivedAt":"2022-02-27T10:25:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A UX improvement for [AL]SAN stack traces. See\nhttps://lore.kernel.org/git/cover-v3-0.4-00000000000-20220221T155656Z-avarab@gmail.com/\nfor the v3.\n\nChanges since v3:\n\n * Add a comment/assertion in 2/4 about what \"t\" directory we\n expect/must have.\n\n * The 3/4 \"change\" is only for rebasing on the new 2/4.\n\nÆvar Arnfjörð Bjarmason (4):\n test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS\n test-lib: correct and assert TEST_DIRECTORY overriding\n test-lib: make $GIT_BUILD_DIR an absolute path\n test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS\n\n t/test-lib.sh | 50 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 40 insertions(+), 10 deletions(-)\n\nRange-diff against v3:\n1: bf31efca464 = 1: d1967ab34a5 test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS\n2: 33a628e9c3a ! 2: 97586ad4541 test-lib: correct commentary on TEST_DIRECTORY overriding\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - test-lib: correct commentary on TEST_DIRECTORY overriding\n + test-lib: correct and assert TEST_DIRECTORY overriding\n \n Correct a misleading comment added by me in 62f539043c7 (test-lib:\n - Allow overriding of TEST_DIRECTORY, 2010-08-19).\n + Allow overriding of TEST_DIRECTORY, 2010-08-19), and add an assertion\n + that TEST_DIRECTORY cannot point to any directory except the \"t\"\n + directory in the top-level of git.git.\n \n - Between that comment and the later addition of\n + This assertion is in effect not new, since we'd already die if that\n + wasn't the case[1], but it and the updated commentary help to make\n + that clearer.\n +\n + The existing comments were also on the wrong arms of the\n + \"if\". I.e. the \"allow tests to override this\" was on the \"test -z\"\n + arm. That came about due to a combination of 62f539043c7 and\n 85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,\n - 2013-11-17) the comments were on the wrong arms of the \"if\". I.e. the\n - \"allow tests to override this\" was on the \"test -z\" arm.\n + 2013-11-17).\n \n - But more importantly this could be read allowing the \"$TEST_DIRECTORY\"\n + Those earlier comments could be read as allowing the \"$TEST_DIRECTORY\"\n to be some path outside of t/. As explained in the updated comment\n that's impossible, rather it was meant for *tests* that ran outside of\n t/, i.e. the \"t0000-basic.sh\" tests that use \"lib-subtest.sh\".\n @@ Commit message\n reflect that, and further comment on why we have a hard dependency on\n this.\n \n + 1. https://lore.kernel.org/git/220222.86o82z8als.gmgdl@evledraar.gmail.com/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## t/test-lib.sh ##\n @@ t/test-lib.sh\n \tTEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) || exit 1\n fi\n if test -z \"$TEST_OUTPUT_DIRECTORY\"\n +@@ t/test-lib.sh: then\n + \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n + fi\n + GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n ++if test \"$TEST_DIRECTORY\" = \"${TEST_DIRECTORY%/t}\"\n ++then\n ++\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n ++\texit 1\n ++fi\n + \n + # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n + # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n3: b03ae29fc92 ! 3: c25c4532c72 test-lib: make $GIT_BUILD_DIR an absolute path\n @@ t/test-lib.sh: then\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n -GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n +-if test \"$TEST_DIRECTORY\" = \"${TEST_DIRECTORY%/t}\"\n +GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n - \n - # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n - # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n ++if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n + then\n + \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n + \texit 1\n @@ t/test-lib.sh: prepend_var () {\n # problems. The GIT_SAN_OPTIONS variable can be used to set common\n # defaults shared between [AL]SAN_OPTIONS.\n4: d212009e517 = 4: fa4946ce7ef test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS\n-- \n2.35.1.1188.g137d9ee5e75\n\n"},{"id":"449687","messageId":"patch-v4-1.4-d1967ab34a5-20220227T102256Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v4-0.4-00000000000-20220227T102256Z-avarab@gmail.com","subject":"[PATCH v4 1/4] test-lib: add GIT_SAN_OPTIONS, inherit [AL]SAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-27T10:25:10Z","receivedAt":"2022-02-27T10:25:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those\nvariables, rather than punting out entirely if we already have them in\nthe environment.\n\nWe want to take any user-provided settings over our own, but we can do\nthat by prepending our defaults to the variable. The libsanitizer\noptions parsing has \"last option wins\" semantics.\n\nIt's now possible to do e.g.:\n\n LSAN_OPTIONS=report_objects=1 ./t0006-date.sh\n\nAnd not have the \"report_objects=1\" setting overwrite our sensible\ndefault of \"abort_on_error=1\", but by prepending to the list we ensure\nthat:\n\n LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh\n\nWill take the desired \"abort_on_error=0\" over our default.\n\nSee b0f4c9087e1 (t: support clang/gcc AddressSanitizer, 2014-12-08)\nfor the original pattern being altered here, and\n85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,\n2017-09-05) for when LSAN_OPTIONS was added in addition to the\nthen-existing ASAN_OPTIONS.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 24 ++++++++++++++++++++----\n 1 file changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e4716b0b867..55f263a02d3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -36,17 +36,33 @@ then\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n+# Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n+# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n+#\n+#\tVAR=$1${VAR:+${1:+$2}$VAR}\n+#\n+# Usage (using \":\" as the $2 delimiter):\n+#\n+#\tprepend_var VAR : VALUE\n+prepend_var () {\n+\teval \"$1=$3\\${$1:+${3:+$2}\\$$1}\"\n+}\n+\n+# If [AL]SAN is in effect we want to abort so that we notice\n+# problems. The GIT_SAN_OPTIONS variable can be used to set common\n+# defaults shared between [AL]SAN_OPTIONS.\n+prepend_var GIT_SAN_OPTIONS : abort_on_error=1\n+\n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n # the noise level. This needs to happen at the start of the script,\n # before we even do our \"did we build git yet\" check (since we don't\n # want that one to complain to stderr).\n-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n+prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS\n+prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n-# If LSAN is in effect we _do_ want leak checking, but we still\n-# want to abort so that we notice the problems.\n-: ${LSAN_OPTIONS=abort_on_error=1}\n+prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-- \n2.35.1.1188.g137d9ee5e75\n\n"},{"id":"449688","messageId":"patch-v4-2.4-97586ad4541-20220227T102256Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v4-0.4-00000000000-20220227T102256Z-avarab@gmail.com","subject":"[PATCH v4 2/4] test-lib: correct and assert TEST_DIRECTORY overriding","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-27T10:25:11Z","receivedAt":"2022-02-27T10:25:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Correct a misleading comment added by me in 62f539043c7 (test-lib:\nAllow overriding of TEST_DIRECTORY, 2010-08-19), and add an assertion\nthat TEST_DIRECTORY cannot point to any directory except the \"t\"\ndirectory in the top-level of git.git.\n\nThis assertion is in effect not new, since we'd already die if that\nwasn't the case[1], but it and the updated commentary help to make\nthat clearer.\n\nThe existing comments were also on the wrong arms of the\n\"if\". I.e. the \"allow tests to override this\" was on the \"test -z\"\narm. That came about due to a combination of 62f539043c7 and\n85176d72513 (test-lib.sh: convert $TEST_DIRECTORY to an absolute path,\n2013-11-17).\n\nThose earlier comments could be read as allowing the \"$TEST_DIRECTORY\"\nto be some path outside of t/. As explained in the updated comment\nthat's impossible, rather it was meant for *tests* that ran outside of\nt/, i.e. the \"t0000-basic.sh\" tests that use \"lib-subtest.sh\".\n\nThose tests have a different working directory, but they set the\n\"TEST_DIRECTORY\" to the same path for bootstrapping. The comments now\nreflect that, and further comment on why we have a hard dependency on\nthis.\n\n1. https://lore.kernel.org/git/220222.86o82z8als.gmgdl@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 22 +++++++++++++++++-----\n 1 file changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 55f263a02d3..48ee3b16ecd 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -19,13 +19,20 @@\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n if test -z \"$TEST_DIRECTORY\"\n then\n-\t# We allow tests to override this, in case they want to run tests\n-\t# outside of t/, e.g. for running tests on the test library\n-\t# itself.\n-\tTEST_DIRECTORY=$(pwd)\n-else\n \t# ensure that TEST_DIRECTORY is an absolute path so that it\n \t# is valid even if the current working directory is changed\n+\tTEST_DIRECTORY=$(pwd)\n+else\n+\t# The TEST_DIRECTORY will always be the path to the \"t\"\n+\t# directory in the git.git checkout. This is overridden by\n+\t# e.g. t/lib-subtest.sh, but only because its $(pwd) is\n+\t# different. Those tests still set \"$TEST_DIRECTORY\" to the\n+\t# same path.\n+\t#\n+\t# See use of \"$GIT_BUILD_DIR\" and \"$TEST_DIRECTORY\" below for\n+\t# hard assumptions about \"$GIT_BUILD_DIR/t\" existing and being\n+\t# the \"$TEST_DIRECTORY\", and e.g. \"$TEST_DIRECTORY/helper\"\n+\t# needing to exist.\n \tTEST_DIRECTORY=$(cd \"$TEST_DIRECTORY\" && pwd) || exit 1\n fi\n if test -z \"$TEST_OUTPUT_DIRECTORY\"\n@@ -35,6 +42,11 @@ then\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n+if test \"$TEST_DIRECTORY\" = \"${TEST_DIRECTORY%/t}\"\n+then\n+\techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n+\texit 1\n+fi\n \n # Prepend a string to a VAR using an arbitrary \":\" delimiter, not\n # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:\n-- \n2.35.1.1188.g137d9ee5e75\n\n"},{"id":"449689","messageId":"patch-v4-3.4-c25c4532c72-20220227T102256Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v4-0.4-00000000000-20220227T102256Z-avarab@gmail.com","subject":"[PATCH v4 3/4] test-lib: make $GIT_BUILD_DIR an absolute path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-27T10:25:12Z","receivedAt":"2022-02-27T10:25:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GIT_BUILD_DIR from a path like \"/path/to/build/t/..\" to\n\"/path/to/build\". The \"TEST_DIRECTORY\" here is already made an\nabsolute path a few lines above this.\n\nWe could simply do $(cd \"$TEST_DIRECTORY\"/..\" && pwd) here, but as\nnoted in the preceding commit the \"$TEST_DIRECTORY\" can't be anything\nexcept the path containing this test-lib.sh file at this point, so we\ncan more cheaply and equally strip the \"/t\" off the end.\n\nThis change will be helpful to LSAN_OPTIONS which will want to strip\nthe build directory path from filenames, which we couldn't do if we\nhad a \"/..\" in there.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 48ee3b16ecd..ba5186c859b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -41,8 +41,8 @@ then\n \t# elsewhere\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n-GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n-if test \"$TEST_DIRECTORY\" = \"${TEST_DIRECTORY%/t}\"\n+GIT_BUILD_DIR=\"${TEST_DIRECTORY%/t}\"\n+if test \"$TEST_DIRECTORY\" = \"$GIT_BUILD_DIR\"\n then\n \techo \"PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?\" >&2\n \texit 1\n@@ -64,6 +64,7 @@ prepend_var () {\n # problems. The GIT_SAN_OPTIONS variable can be used to set common\n # defaults shared between [AL]SAN_OPTIONS.\n prepend_var GIT_SAN_OPTIONS : abort_on_error=1\n+prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\\\"$GIT_BUILD_DIR/\\\"\n \n # If we were built with ASAN, it may complain about leaks\n # of program-lifetime variables. Disable it by default to lower\n-- \n2.35.1.1188.g137d9ee5e75\n\n"},{"id":"449690","messageId":"patch-v4-4.4-fa4946ce7ef-20220227T102256Z-avarab@gmail.com","threadId":"57441","inReplyTo":"cover-v4-0.4-00000000000-20220227T102256Z-avarab@gmail.com","subject":"[PATCH v4 4/4] test-lib: add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-27T10:25:13Z","receivedAt":"2022-02-27T10:26:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add \"fast_unwind_on_malloc=0\" to LSAN_OPTIONS to get more meaningful\nstack traces from LSAN. This isn't required under ASAN which will emit\ntraces such as this one for a leak in \"t/t0006-date.sh\":\n\n $ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd\n [...]\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x488b94 in strdup (t/helper/test-tool+0x488b94)\n #1 0x9444a4 in xstrdup wrapper.c:29:14\n #2 0x5995fa in parse_date_format date.c:991:24\n #3 0x4d2056 in show_dates t/helper/test-date.c:39:2\n #4 0x4d174a in cmd__date t/helper/test-date.c:116:3\n #5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11\n #6 0x4cd1e3 in main common-main.c:52:11\n #7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n #8 0x422b09 in _start (t/helper/test-tool+0x422b09)\n\n SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nWhereas LSAN would emit this instead:\n\n $ ./t0006-date.sh -vixd\n [...]\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n #1 0x7f2be1d614aa in strdup string/strdup.c:42:15\n\n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nNow we'll instead git this sensible stack trace under\nLSAN. I.e. almost the same one (but starting with \"malloc\", as is\nusual for LSAN) as under ASAN:\n\n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)\n #1 0x7f012af5c4aa in strdup string/strdup.c:42:15\n #2 0x5cb164 in xstrdup wrapper.c:29:14\n #3 0x495ee9 in parse_date_format date.c:991:24\n #4 0x453aac in show_dates t/helper/test-date.c:39:2\n #5 0x453782 in cmd__date t/helper/test-date.c:116:3\n #6 0x451d95 in cmd_main t/helper/test-tool.c:127:11\n #7 0x451f1e in main common-main.c:52:11\n #8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16\n #9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)\n\n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n Aborted\n\nAs the option name suggests this does make things slower, e.g. for\nt0001-init.sh we're around 10% slower:\n\n $ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3\n Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh\n Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]\n Range (min … max): 2.122 s … 2.152 s 3 runs\n\n Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh\n Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]\n Range (min … max): 1.941 s … 2.044 s 3 runs\n\n Summary\n 'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran\n 1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'\n\nI think that's more than worth it to get the more meaningful stack\ntraces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for\none-off \"fast\" runs.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ba5186c859b..9af5fb7674d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -76,6 +76,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n+prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0\n export LSAN_OPTIONS\n \n if test ! -f \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n-- \n2.35.1.1188.g137d9ee5e75\n\n"}]}