| Message ID | pull.1210.git.1649507317350.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > As the address sanitizer checks for a superset of the issues detected > by setting MALLOC_CHECK_ (which tries to detect things like double > frees and off-by-one errors) there is no need to set the latter when > compiling with -fsanitize=address. Very good idea. > I'm submitting this now as it fixes a regression introduced in the > current cycle. Having said that there is an easy workaround (once one > has discovered GIT_TEST_NO_MALLOC_CHECK) so I'd be happy to wait until > the start of the next cycle given I've just missed -rc1. Yeah, if this patch were broken, we'd be in worse place than we currently are, so I'd rather not fast-track it. I will queue it in 'seen' and possibly merge to 'next' as it is a good idea to avoid using both at the same time, though. Thanks.
On Sat, Apr 09 2022, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > As the address sanitizer checks for a superset of the issues detected > by setting MALLOC_CHECK_ (which tries to detect things like double > frees and off-by-one errors) there is no need to set the latter when > compiling with -fsanitize=address. > > This fixes a regression introduced by 131b94a10a ("test-lib.sh: Use > GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34", 2022-03-04) > which causes all the tests to fail with the message > > ASan runtime does not come first in initial library list; > you should either link runtime to your application or > manually preload it with LD_PRELOAD. > > when git is compiled with SANITIZE=address on systems with glibc >= > 2.34. I have tested SANITIZE=leak and SANITIZE=undefined and they do > not suffer from this regression so the fix in this patch should be > sufficient. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK > > I'm submitting this now as it fixes a regression introduced in the > current cycle. Having said that there is an easy workaround (once one > has discovered GIT_TEST_NO_MALLOC_CHECK) so I'd be happy to wait until > the start of the next cycle given I've just missed -rc1. I wonder why we have to justify that we'll only turn on TEST_NO_MALLOC_CHECK if it's SANITIZE=address. I.e. we also have SANITIZE=undefined, wouldn't it be more future-proof to just say that these analysis options are mutually exclusive by default? That would have the bonus of e.g. making SANITIZE=leak faster, it's already slow enough without the extra help of glibc's instrumentation. > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1210%2Fphillipwood%2Fwip%2Ftest-malloc-asan-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1210/phillipwood/wip/test-malloc-asan-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1210 > > Makefile | 5 ++++- > t/test-lib.sh | 5 +++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Makefile b/Makefile > index 91738485626..76d187991d2 100644 > --- a/Makefile > +++ b/Makefile > @@ -1267,8 +1267,9 @@ PTHREAD_CFLAGS = > SPARSE_FLAGS ?= -std=gnu99 > SP_EXTRA_FLAGS = -Wno-universal-initializer > > -# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target > +# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak,address targets > SANITIZE_LEAK = > +SANITIZE_ADDRESS = > > # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will > # usually result in less CPU usage at the cost of higher peak memory. > @@ -1314,6 +1315,7 @@ SANITIZE_LEAK = YesCompiledWithIt > endif > ifneq ($(filter address,$(SANITIZERS)),) > NO_REGEX = NeededForASAN > +SANITIZE_ADDRESS = YesCompiledWithIt > endif > endif > > @@ -2861,6 +2863,7 @@ GIT-BUILD-OPTIONS: FORCE > @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+ > @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+ > @echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+ > + @echo SANITIZE_ADDRESS=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_ADDRESS)))'\' >>$@+ Then this could just add SANITIZERS=$(SANITIZERS), we still need SANITIZE_LEAK as we care about that specifically, but This mostly sounds sensible, but for this: > -# Add libc MALLOC and MALLOC_PERTURB test > -# only if we are not executing the test with valgrind > +# Add libc MALLOC and MALLOC_PERTURB test only if we are not executing > +# the test with valgrind and have not compiled with SANITIZE=address. > if test -n "$valgrind" || > + test -n "$SANITIZE_ADDRESS" || > test -n "$TEST_NO_MALLOC_CHECK" > then > setup_malloc_check () { We could check $SANITIZERS here instead.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I wonder why we have to justify that we'll only turn on > TEST_NO_MALLOC_CHECK if it's SANITIZE=address. > > I.e. we also have SANITIZE=undefined, wouldn't it be more future-proof > to just say that these analysis options are mutually exclusive by > default? Given that the SANITIZE mechanism itself allows more than one to be requested at the same time, it is unclear to me why other checks like undefined needs to exclude checks done by other mechanisms like MALLOC_CHECK_ by default. If I correctly read under-the-three-dash commentary Phillip wrote, it's not like that use of MALLOC_CHECK_ inherently interferes with the way SANITIZE=undefined wants to work, no?
On Mon, Apr 11 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I wonder why we have to justify that we'll only turn on >> TEST_NO_MALLOC_CHECK if it's SANITIZE=address. >> >> I.e. we also have SANITIZE=undefined, wouldn't it be more future-proof >> to just say that these analysis options are mutually exclusive by >> default? > > Given that the SANITIZE mechanism itself allows more than one to be > requested at the same time, it is unclear to me why other checks > like undefined needs to exclude checks done by other mechanisms like > MALLOC_CHECK_ by default. If I correctly read under-the-three-dash > commentary Phillip wrote, it's not like that use of MALLOC_CHECK_ > inherently interferes with the way SANITIZE=undefined wants to work, > no? Because: * It makes it slower, and part of the utility of these checks is that they run in a timely fashion. * We add these glibc checks because we'd like to catch malloc()/free() issues, and run the test suite with them by default. Someone using the SANITIZE=* feature is almost certain to be also doing a "normal" test run, so I don't think we're getting anything extra by combining the two, except needlessly slowing it down. * Even though SANITIZE=leak,address & valgrind are strictly speaking incompatible with the glibc check, having inject itself into other sanitize modes is surely going to make debugging harder until you discover that we're also injecting the custom malloc.
diff --git a/Makefile b/Makefile index 91738485626..76d187991d2 100644 --- a/Makefile +++ b/Makefile @@ -1267,8 +1267,9 @@ PTHREAD_CFLAGS = SPARSE_FLAGS ?= -std=gnu99 SP_EXTRA_FLAGS = -Wno-universal-initializer -# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target +# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak,address targets SANITIZE_LEAK = +SANITIZE_ADDRESS = # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will # usually result in less CPU usage at the cost of higher peak memory. @@ -1314,6 +1315,7 @@ SANITIZE_LEAK = YesCompiledWithIt endif ifneq ($(filter address,$(SANITIZERS)),) NO_REGEX = NeededForASAN +SANITIZE_ADDRESS = YesCompiledWithIt endif endif @@ -2861,6 +2863,7 @@ GIT-BUILD-OPTIONS: FORCE @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+ @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+ @echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+ + @echo SANITIZE_ADDRESS=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_ADDRESS)))'\' >>$@+ @echo X=\'$(X)\' >>$@+ ifdef FSMONITOR_DAEMON_BACKEND @echo FSMONITOR_DAEMON_BACKEND=\''$(subst ','\'',$(subst ','\'',$(FSMONITOR_DAEMON_BACKEND)))'\' >>$@+ diff --git a/t/test-lib.sh b/t/test-lib.sh index 531cef097db..f09e8f3efce 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -535,9 +535,10 @@ case $GIT_TEST_FSYNC in ;; esac -# Add libc MALLOC and MALLOC_PERTURB test -# only if we are not executing the test with valgrind +# Add libc MALLOC and MALLOC_PERTURB test only if we are not executing +# the test with valgrind and have not compiled with SANITIZE=address. if test -n "$valgrind" || + test -n "$SANITIZE_ADDRESS" || test -n "$TEST_NO_MALLOC_CHECK" then setup_malloc_check () {