| Message ID | patch-v3-3.4-b03ae29fc92-20220221T155656Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib: improve LSAN + ASAN stack traces | expand |
On Mon, Feb 21, 2022 at 04:58:34PM +0100, Ævar Arnfjörð Bjarmason wrote: > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 77c3fabd041..ff13321bfd3 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -41,7 +41,7 @@ then > # elsewhere > TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY > fi > -GIT_BUILD_DIR="$TEST_DIRECTORY"/.. > +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}" Sorry to notice this so late, but this hunk caught my eye. What happens if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")? Before this change, we would have set GIT_BUILD_DIR to the parent of whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if it doesn't, then we'll set GIT_BUILD_DIR to be the same as TEST_DIRECTORY, which is a behavior change. If you just want GIT_BUILD_DIR to be absolute in order to for LSan to understand how to correctly strip it out of filenames, could we replace this with: GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)" or (an admittedly less clear) GIT_BUILD_DIR="$(dirname "$TEST_DIRECTORY")" Thanks, Taylor
On Mon, Feb 21 2022, Taylor Blau wrote: > On Mon, Feb 21, 2022 at 04:58:34PM +0100, Ævar Arnfjörð Bjarmason wrote: >> diff --git a/t/test-lib.sh b/t/test-lib.sh >> index 77c3fabd041..ff13321bfd3 100644 >> --- a/t/test-lib.sh >> +++ b/t/test-lib.sh >> @@ -41,7 +41,7 @@ then >> # elsewhere >> TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY >> fi >> -GIT_BUILD_DIR="$TEST_DIRECTORY"/.. >> +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}" > > Sorry to notice this so late, but this hunk caught my eye. What happens > if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")? I think that the preceding 2/4 should cover your concern here, i.e. I think that's not possible. > Before this change, we would have set GIT_BUILD_DIR to the parent of > whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still > do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if > it doesn't, then we'll set GIT_BUILD_DIR to be the same as > TEST_DIRECTORY, which is a behavior change. Indeed, but I believe (again see 2/4) that can't happen. > If you just want GIT_BUILD_DIR to be absolute in order to for LSan to > understand how to correctly strip it out of filenames, could we replace > this with: > > GIT_BUILD_DIR="$(cd "$TEST_DIRECTORY/.." && pwd)" > > or (an admittedly less clear) > > GIT_BUILD_DIR="$(dirname "$TEST_DIRECTORY")" Yes. I almost changed it to the former in this re-roll, but as noted in <220219.86y227fvz1.gmgdl@evledraar.gmail.com> thought it was better to have it clear that this really wasn't a generic mechanism, we really do need/want the actual "t" directory here.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Sorry to notice this so late, but this hunk caught my eye. What happens >> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")? > > I think that the preceding 2/4 should cover your concern here, i.e. I > think that's not possible. > >> Before this change, we would have set GIT_BUILD_DIR to the parent of >> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still >> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if >> it doesn't, then we'll set GIT_BUILD_DIR to be the same as >> TEST_DIRECTORY, which is a behavior change. > > Indeed, but I believe (again see 2/4) that can't happen. It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY override logic did not mean to support such a use case". I am perfectly fine if we declared that we do not to support the use of that override mechanism by anybody but the "subtest" thing we do ourselves. If we can catch a workflow that misuses the mechansim cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set and it does not end in "/t"), we should do so, I would think, instead of doing the "go up and do pwd", which will make things worse. Thanks.
On Mon, Feb 21 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> Sorry to notice this so late, but this hunk caught my eye. What happens >>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")? >> >> I think that the preceding 2/4 should cover your concern here, i.e. I >> think that's not possible. >> >>> Before this change, we would have set GIT_BUILD_DIR to the parent of >>> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still >>> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if >>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as >>> TEST_DIRECTORY, which is a behavior change. >> >> Indeed, but I believe (again see 2/4) that can't happen. > > It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY > override logic did not mean to support such a use case". To clarify with "can't happen" I mean (and should have said) that "can't work", i.e. it would error out anyway. E.g. try in a git.git checkout: ( mv t t2 && cd t && ./t0001-init.sh ) It will die with: You need to build test-tool: Run "make t/helper/test-tool" in the source (toplevel) directory FATAL: Unexpected exit with code 1 And if you were to manually patch test-lib.sh to get past that error it would start erroring on e.g.: sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory And if you "fix" that it'll error out on something else. I.e. we'll have discovered that $(pwd)/.. must be our build directory, and we then construct paths by adding the string "/t/[...]" to that. > I am perfectly fine if we declared that we do not to support the use > of that override mechanism by anybody but the "subtest" thing we do > ourselves. If we can catch a workflow that misuses the mechansim > cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set > and it does not end in "/t"), we should do so, I would think, instead > of doing the "go up and do pwd", which will make things worse. What I was going for in 2/4 in http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com is that we've already declared that. I.e. test-lib.sh has various assumptions about appending "/t/..." to the build directory being a valid way to get paths to various test-lib.sh-adjacent code. So trimming off "/t" here with a string operation v.s. $(cd .. && pwd) is being consistent with that code. It would be odd to make the bit at the top very generic, only to have the reader keep reading and wonder how that generic mechanism and the subsequent hardcoding of "/t/[...]" are supposed to work together.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Mon, Feb 21 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>>> Sorry to notice this so late, but this hunk caught my eye. What happens >>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")? >>> >>> I think that the preceding 2/4 should cover your concern here, i.e. I >>> think that's not possible. >>> >>>> Before this change, we would have set GIT_BUILD_DIR to the parent of >>>> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still >>>> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if >>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as >>>> TEST_DIRECTORY, which is a behavior change. >>> >>> Indeed, but I believe (again see 2/4) that can't happen. >> >> It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY >> override logic did not mean to support such a use case". > > To clarify with "can't happen" I mean (and should have said) that "can't > work", i.e. it would error out anyway. > > E.g. try in a git.git checkout: > > ( > mv t t2 && > cd t && > ./t0001-init.sh > ) > > It will die with: > > You need to build test-tool: > Run "make t/helper/test-tool" in the source (toplevel) directory > FATAL: Unexpected exit with code 1 > > And if you were to manually patch test-lib.sh to get past that error it > would start erroring on e.g.: > > sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory > > And if you "fix" that it'll error out on something else. > > I.e. we'll have discovered that $(pwd)/.. must be our build directory, > and we then construct paths by adding the string "/t/[...]" to that. > >> I am perfectly fine if we declared that we do not to support the use >> of that override mechanism by anybody but the "subtest" thing we do >> ourselves. If we can catch a workflow that misuses the mechansim >> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set >> and it does not end in "/t"), we should do so, I would think, instead >> of doing the "go up and do pwd", which will make things worse. > > What I was going for in 2/4 in > http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com > is that we've already declared that. I.e. test-lib.sh has various > assumptions about appending "/t/..." to the build directory being a > valid way to get paths to various test-lib.sh-adjacent code. > > So trimming off "/t" here with a string operation v.s. $(cd .. && pwd) > is being consistent with that code. > > It would be odd to make the bit at the top very generic, only to have > the reader keep reading and wonder how that generic mechanism and the > subsequent hardcoding of "/t/[...]" are supposed to work together. Correct. That is why I said $(... pwd) to pretend that we can take anything would make it worse in a separate message. If we have to strip off /t anyway, piggy-backing on that part to detect and abort when somebody misused the mechanism would be a good idea---which is what I said in the message you are responding to and not responding to.
On Wed, Feb 23 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Mon, Feb 21 2022, Junio C Hamano wrote: >> >>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> >>>>> Sorry to notice this so late, but this hunk caught my eye. What happens >>>>> if `TEST_DIRECTORY` is provided by the user (and doesn't end in "/t")? >>>> >>>> I think that the preceding 2/4 should cover your concern here, i.e. I >>>> think that's not possible. >>>> >>>>> Before this change, we would have set GIT_BUILD_DIR to the parent of >>>>> whatever TEST_DIRECTORY is, whether or not it ended in "/t". We'll still >>>>> do the same thing with this patch if TEST_DIRECTORY ends in "/t". But if >>>>> it doesn't, then we'll set GIT_BUILD_DIR to be the same as >>>>> TEST_DIRECTORY, which is a behavior change. >>>> >>>> Indeed, but I believe (again see 2/4) that can't happen. >>> >>> It is not like "can't happen", but "whoever wrote the TEST_DIRECTORY >>> override logic did not mean to support such a use case". >> >> To clarify with "can't happen" I mean (and should have said) that "can't >> work", i.e. it would error out anyway. >> >> E.g. try in a git.git checkout: >> >> ( >> mv t t2 && >> cd t && >> ./t0001-init.sh >> ) >> >> It will die with: >> >> You need to build test-tool: >> Run "make t/helper/test-tool" in the source (toplevel) directory >> FATAL: Unexpected exit with code 1 >> >> And if you were to manually patch test-lib.sh to get past that error it >> would start erroring on e.g.: >> >> sed: couldn't open file /home/avar/g/git/t2/../t/chainlint.sed: No such file or directory >> >> And if you "fix" that it'll error out on something else. >> >> I.e. we'll have discovered that $(pwd)/.. must be our build directory, >> and we then construct paths by adding the string "/t/[...]" to that. >> >>> I am perfectly fine if we declared that we do not to support the use >>> of that override mechanism by anybody but the "subtest" thing we do >>> ourselves. If we can catch a workflow that misuses the mechansim >>> cheaply enough (e.g. perhaps erroring out if TEST_DIRECTORY is set >>> and it does not end in "/t"), we should do so, I would think, instead >>> of doing the "go up and do pwd", which will make things worse. >> >> What I was going for in 2/4 in >> http://lore.kernel.org/git/patch-v3-2.4-33a628e9c3a-20220221T155656Z-avarab@gmail.com >> is that we've already declared that. I.e. test-lib.sh has various >> assumptions about appending "/t/..." to the build directory being a >> valid way to get paths to various test-lib.sh-adjacent code. >> >> So trimming off "/t" here with a string operation v.s. $(cd .. && pwd) >> is being consistent with that code. >> >> It would be odd to make the bit at the top very generic, only to have >> the reader keep reading and wonder how that generic mechanism and the >> subsequent hardcoding of "/t/[...]" are supposed to work together. > > Correct. That is why I said $(... pwd) to pretend that we can take > anything would make it worse in a separate message. > > If we have to strip off /t anyway, piggy-backing on that part to > detect and abort when somebody misused the mechanism would be a good > idea---which is what I said in the message you are responding to and > not responding to. So you're OK with the assumption/method being used here, but would prefer if we also added an explicit check/"exit 1"? E.g.: if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}" then echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2 exit 1 fi ?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > So you're OK with the assumption/method being used here, but would > prefer if we also added an explicit check/"exit 1"? E.g.: > > if test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/t}" > then > echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2 > exit 1 > fi Exactly.
diff --git a/t/test-lib.sh b/t/test-lib.sh index 77c3fabd041..ff13321bfd3 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -41,7 +41,7 @@ then # elsewhere TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY fi -GIT_BUILD_DIR="$TEST_DIRECTORY"/.. +GIT_BUILD_DIR="${TEST_DIRECTORY%/t}" # Prepend a string to a VAR using an arbitrary ":" delimiter, not # adding the delimiter if VAR or VALUE is empty. I.e. a generalized: @@ -59,6 +59,7 @@ prepend_var () { # problems. The GIT_SAN_OPTIONS variable can be used to set common # defaults shared between [AL]SAN_OPTIONS. prepend_var GIT_SAN_OPTIONS : abort_on_error=1 +prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\" # If we were built with ASAN, it may complain about leaks # of program-lifetime variables. Disable it by default to lower
Change the GIT_BUILD_DIR from a path like "/path/to/build/t/.." to "/path/to/build". The "TEST_DIRECTORY" here is already made an absolute path a few lines above this. We could simply do $(cd "$TEST_DIRECTORY"/.." && pwd) here, but as noted in the preceding commit the "$TEST_DIRECTORY" can't be anything except the path containing this test-lib.sh file at this point, so we can more cheaply and equally strip the "/t" off the end. This change will be helpful to LSAN_OPTIONS which will want to strip the build directory path from filenames, which we couldn't do if we had a "/.." in there. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/test-lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)