git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* UNLEAK(), leak checking in the default tests etc.
@ 2021-06-09 14:38 Ævar Arnfjörð Bjarmason
  2021-06-09 17:44 ` Andrzej Hunt
  2021-06-10 19:01 ` SZEDER Gábor
  0 siblings, 2 replies; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-09 14:38 UTC (permalink / raw)
  To: Andrzej Hunt
  Cc: git, Andrzej Hunt, Jeff King, Lénaïc Huard, Derrick Stolee


[In-Reply-To
<a74bbcae7363df03bf8e93167d9274d16dc807f3.1615747662.git.gitgitgadget@gmail.com>,
but intentionally breaking threading for a new topic]

On Sun, Mar 14 2021, Andrzej Hunt via GitGitGadget wrote:

> Most of these pointers can safely be freed when cmd_clone() completes,
> therefore we make sure to free them. The one exception is that we
> have to UNLEAK(repo) because it can point either to argv[0], or a
> malloc'd string returned by absolute_pathdup().

I ran into this when manually checking with valgrind and discovered that
you need SANITIZERS for -DSUPPRESS_ANNOTATED_LEAKS to squash it.

I wonder if that shouldn't be in DEVOPTS (or even a default under
DEVELOPER=1). I.e. you don't need any other special compile flags, just
a compiled git that you then run under valgrind to spot this.

>  builtin/clone.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 51e844a2de0a..952fe3d8fc88 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  {
>  	int is_bundle = 0, is_local;
>  	const char *repo_name, *repo, *work_tree, *git_dir;
> -	char *path, *dir, *display_repo = NULL;
> +	char *path = NULL, *dir, *display_repo = NULL;
>  	int dest_exists, real_dest_exists = 0;
>  	const struct ref *refs, *remote_head;
> -	const struct ref *remote_head_points_at;
> +	struct ref *remote_head_points_at = NULL;
>  	const struct ref *our_head_points_at;
>  	struct ref *mapped_refs;
>  	const struct ref *ref;
> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	repo_name = argv[0];
>  
>  	path = get_repo_path(repo_name, &is_bundle);
> -	if (path)
> +	if (path) {
> +		FREE_AND_NULL(path);
>  		repo = absolute_pathdup(repo_name);
> -	else if (strchr(repo_name, ':')) {
> +	} else if (strchr(repo_name, ':')) {
>  		repo = repo_name;
>  		display_repo = transport_anonymize_url(repo);
>  	} else

In this case it seems better to just have a :

    int repo_heap = 0;

    Then set "repo_heap = 1" in that absolute_pathdup(repo_name) branch,
    and...

> @@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&reflog_msg);
>  	strbuf_release(&branch_top);
>  	strbuf_release(&key);
> +	free_refs(mapped_refs);
> +	free_refs(remote_head_points_at);
> +	free(dir);
> +	free(path);
> +	UNLEAK(repo);

Here do:

    if (repo_heap)
        free(repo);

But maybe there's some other out of the box way to make leak checking
Just Work without special flags in this case. I'm just noting this one
because it ended up being the only one that leaked unless I compiled
with -DSUPPRESS_ANNOTATED_LEAKS. I was fixing some leaks in the bundle
code.

Anyway, getting to the "default tests" point. I fixed a memory leak, and
wanted to it tested that the specific command doesn't leak in git's
default tests.

Do we have such a thing, if not why not?

The closest I got to getting this was:

    GIT_VALGRIND_MODE=memcheck GIT_VALGRIND_OPTIONS="--leak-check=full --errors-for-leak-kinds=definite --error-exitcode=123" <SOME TEST> --valgrind

But as t/README notes it implies --verbose so we can't currently run it
under the test harness (although I have out-of-tree patches to fix that
in general).

It seems pretty straightforward to turn that specific thing into a test
with a prereq to detect if valgrind works in that mode at all, and then
do (in some dedicated test file):

	# Exit/skip if we can't setup valgrind, then setup relevant
        # valgrind options (maybe needing to re-source test-lib.sh, ew!)
	test_expect_successs 'ls-heads should not leak' '
		git bundle ls-heads a.bdl
	'

But from what I've found so far no such thing exists, and it seems to
the extent that this is checked it's run manually as a one-off (see git
log --grep=valgrind), but we don't explicitly test for this
anywhere. Have I missed something?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-09 14:38 UNLEAK(), leak checking in the default tests etc Ævar Arnfjörð Bjarmason
@ 2021-06-09 17:44 ` Andrzej Hunt
  2021-06-09 20:36   ` Felipe Contreras
                     ` (2 more replies)
  2021-06-10 19:01 ` SZEDER Gábor
  1 sibling, 3 replies; 10+ messages in thread
From: Andrzej Hunt @ 2021-06-09 17:44 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Lénaïc Huard, Derrick Stolee



On 09/06/2021 16:38, Ævar Arnfjörð Bjarmason wrote:
> 
> [In-Reply-To
> <a74bbcae7363df03bf8e93167d9274d16dc807f3.1615747662.git.gitgitgadget@gmail.com>,
> but intentionally breaking threading for a new topic]
> 
> On Sun, Mar 14 2021, Andrzej Hunt via GitGitGadget wrote:
> 
>> Most of these pointers can safely be freed when cmd_clone() completes,
>> therefore we make sure to free them. The one exception is that we
>> have to UNLEAK(repo) because it can point either to argv[0], or a
>> malloc'd string returned by absolute_pathdup().
> 
> I ran into this when manually checking with valgrind and discovered that
> you need SANITIZERS for -DSUPPRESS_ANNOTATED_LEAKS to squash it.
> 
> I wonder if that shouldn't be in DEVOPTS (or even a default under
> DEVELOPER=1). I.e. you don't need any other special compile flags, just
> a compiled git that you then run under valgrind to spot this.

I'm not familiar with git's development conventions/philosophy, but my 
2c is that it's better not to enable it by default in order to minimise 
divergence from the code that users are running. OTOH it's not a major 
difference in behaviour so perhaps that's not a concern here.

More significantly: I get the impression it's easier to do leak checking 
using LSAN, which requires recompiling git anyway - at which point you 
get the flag for free - so how often will people actually perform leak 
checking with Valgrind in the first place?

> 
>>   builtin/clone.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 51e844a2de0a..952fe3d8fc88 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   {
>>   	int is_bundle = 0, is_local;
>>   	const char *repo_name, *repo, *work_tree, *git_dir;
>> -	char *path, *dir, *display_repo = NULL;
>> +	char *path = NULL, *dir, *display_repo = NULL;
>>   	int dest_exists, real_dest_exists = 0;
>>   	const struct ref *refs, *remote_head;
>> -	const struct ref *remote_head_points_at;
>> +	struct ref *remote_head_points_at = NULL;
>>   	const struct ref *our_head_points_at;
>>   	struct ref *mapped_refs;
>>   	const struct ref *ref;
>> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   	repo_name = argv[0];
>>   
>>   	path = get_repo_path(repo_name, &is_bundle);
>> -	if (path)
>> +	if (path) {
>> +		FREE_AND_NULL(path);
>>   		repo = absolute_pathdup(repo_name);
>> -	else if (strchr(repo_name, ':')) {
>> +	} else if (strchr(repo_name, ':')) {
>>   		repo = repo_name;
>>   		display_repo = transport_anonymize_url(repo);
>>   	} else
> 
> In this case it seems better to just have a :
> 
>      int repo_heap = 0;
> 
>      Then set "repo_heap = 1" in that absolute_pathdup(repo_name) branch,
>      and...
> 
>> @@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   	strbuf_release(&reflog_msg);
>>   	strbuf_release(&branch_top);
>>   	strbuf_release(&key);
>> +	free_refs(mapped_refs);
>> +	free_refs(remote_head_points_at);
>> +	free(dir);
>> +	free(path);
>> +	UNLEAK(repo);
> 
> Here do:
> 
>      if (repo_heap)
>          free(repo);
> 

Although this is possible, I don't think it's worth it: if UNLEAK 
already exists, we might as well use it here to make the code simpler. 
And UNLEAK is unlikely to go away anytime soon because... (continued below)

> But maybe there's some other out of the box way to make leak checking
> Just Work without special flags in this case. I'm just noting this one
> because it ended up being the only one that leaked unless I compiled
> with -DSUPPRESS_ANNOTATED_LEAKS. I was fixing some leaks in the bundle
> code.

There are trickier examples where a cmd_* function has a complex struct 
on the stack, and correctly clearing all allocated memory pointed to by 
its members (or in turn further children with potentially multiple 
levels of indirection) is a lot of work - and that work doesn't actually 
benefit the user in any way. In other words, we either need to be able 
to use UNLEAK to suppress certain classes of uninteresting memory leaks 
- which allows us to focus on the interesting/real leaks - or someone 
has to spend a lot of time doing cleanup by hand (and/or someone has to 
implement a bunch of new cleanup functions)).

In your example above, the UNLEAK can be avoided at the cost of one 
additional tracking variable - but in many other cases avoiding an 
UNLEAK is much more expensive. It's certainly valid to debate the merits 
of the UNLEAK here, but that won't remove the need for UNLEAK's 
existence in general.

(The most common example that I remember is where cmd_* has a rev_info, 
and AFAICT there's no one-liner to clean that up. Using UNLEAK is 
honestly the best approach there. I don't think I've actually submitted 
any patches doing this, but I have a few in my local backlog.)
> Anyway, getting to the "default tests" point. I fixed a memory leak, and
> wanted to it tested that the specific command doesn't leak in git's
> default tests.
> 
> Do we have such a thing, if not why not?
> 
> The closest I got to getting this was:
> 
>      GIT_VALGRIND_MODE=memcheck GIT_VALGRIND_OPTIONS="--leak-check=full --errors-for-leak-kinds=definite --error-exitcode=123" <SOME TEST> --valgrind

It's easy to perform leak-checking runs *if* you're OK recompiling with 
LSAN, instead of using valgrind. My usual recipe for running against a 
range of tests is something like:

   make SANITIZE=address,leak 
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" CFLAGS="-Og -g" 
T="\$(wildcard t00[0-9][0-9]-*.sh)" test

Additionally: I usually specify CC=clang, although gcc+LSAN has mostly 
been stable enough in my experience so you might be able to skip that.
(I've found ASAN+LSAN to be more stable than LSAN by itself, which is 
why I specify address+leak, but adding ASAN in turn requires overriding 
ASAN_OPTIONS to reenable leak checking.)

I don't know whether or not Valgrind is more/less effective at finding 
leaks, so being able to run the test suite under valgrind would be nice 
for comparison purposes though.

ATB,

   Andrzej

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-09 17:44 ` Andrzej Hunt
@ 2021-06-09 20:36   ` Felipe Contreras
  2021-06-10 10:46   ` Jeff King
  2021-06-10 10:56   ` Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 10+ messages in thread
From: Felipe Contreras @ 2021-06-09 20:36 UTC (permalink / raw)
  To: Andrzej Hunt, Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Lénaïc Huard, Derrick Stolee

Andrzej Hunt wrote:
> On 09/06/2021 16:38, Ævar Arnfjörð Bjarmason wrote:

> > I wonder if that shouldn't be in DEVOPTS (or even a default under
> > DEVELOPER=1). I.e. you don't need any other special compile flags, just
> > a compiled git that you then run under valgrind to spot this.
> 
> I'm not familiar with git's development conventions/philosophy, but my 
> 2c is that it's better not to enable it by default in order to minimise 
> divergence from the code that users are running.

It woudln't be on by default, you would need to turn it on with
`make DEVEOPER=1`.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-09 17:44 ` Andrzej Hunt
  2021-06-09 20:36   ` Felipe Contreras
@ 2021-06-10 10:46   ` Jeff King
  2021-06-10 10:56   ` Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2021-06-10 10:46 UTC (permalink / raw)
  To: Andrzej Hunt
  Cc: Ævar Arnfjörð Bjarmason, git,
	Lénaïc Huard, Derrick Stolee

On Wed, Jun 09, 2021 at 07:44:12PM +0200, Andrzej Hunt wrote:

> > I ran into this when manually checking with valgrind and discovered that
> > you need SANITIZERS for -DSUPPRESS_ANNOTATED_LEAKS to squash it.
> > 
> > I wonder if that shouldn't be in DEVOPTS (or even a default under
> > DEVELOPER=1). I.e. you don't need any other special compile flags, just
> > a compiled git that you then run under valgrind to spot this.
> 
> I'm not familiar with git's development conventions/philosophy, but my 2c is
> that it's better not to enable it by default in order to minimise divergence
> from the code that users are running. OTOH it's not a major difference in
> behaviour so perhaps that's not a concern here.

Yeah, I'd rather not enable the option during normal builds. It carries
a run-time penalty (it is actually building a pointless data structure
that _does_ effectively leak the pointers, but backed by a global so
they're "findable" by leak checkers). So it changes speed and possibly
correctness of the final binary in a way that is different from what
people would actually run in practice.

That might be worth it if there was some advantage to just turning it
on (i.e., if by running with it all the time we might detect some bug).
But by itself it does nothing useful.

If you really want to leak-check more thoroughly the normal binary, then
IMHO you'd be better off to convert UNLEAK() sites to actual free calls.

> More significantly: I get the impression it's easier to do leak checking
> using LSAN, which requires recompiling git anyway - at which point you get
> the flag for free - so how often will people actually perform leak checking
> with Valgrind in the first place?

And yeah, I'd very much agree here. It's definitely not wrong to run
with Valgrind. But it's slower and much less thorough than ASan (probably not for
leak detection, but definitely for bug-finding, since it can't look at
stack variables).

If you do use it, and want to build with -DSUPPRESS_ANNOTATED_LEAKS all
the time, that's OK, but I don't think it makes sense for it to the
default even under DEVELOPER=1. I'm not opposed to a patch to make it
easier to flip the switch, though (but I also find sticking a line in
your config.mak to be pretty easy already).

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-09 17:44 ` Andrzej Hunt
  2021-06-09 20:36   ` Felipe Contreras
  2021-06-10 10:46   ` Jeff King
@ 2021-06-10 10:56   ` Ævar Arnfjörð Bjarmason
  2021-06-10 13:38     ` Jeff King
  2 siblings, 1 reply; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-10 10:56 UTC (permalink / raw)
  To: Andrzej Hunt; +Cc: git, Jeff King, Lénaïc Huard, Derrick Stolee


On Wed, Jun 09 2021, Andrzej Hunt wrote:

> On 09/06/2021 16:38, Ævar Arnfjörð Bjarmason wrote:
>> [In-Reply-To
>> <a74bbcae7363df03bf8e93167d9274d16dc807f3.1615747662.git.gitgitgadget@gmail.com>,
>> but intentionally breaking threading for a new topic]
>> On Sun, Mar 14 2021, Andrzej Hunt via GitGitGadget wrote:
>> 
>>> Most of these pointers can safely be freed when cmd_clone() completes,
>>> therefore we make sure to free them. The one exception is that we
>>> have to UNLEAK(repo) because it can point either to argv[0], or a
>>> malloc'd string returned by absolute_pathdup().
>> I ran into this when manually checking with valgrind and discovered
>> that
>> you need SANITIZERS for -DSUPPRESS_ANNOTATED_LEAKS to squash it.
>> I wonder if that shouldn't be in DEVOPTS (or even a default under
>> DEVELOPER=1). I.e. you don't need any other special compile flags, just
>> a compiled git that you then run under valgrind to spot this.
>
> I'm not familiar with git's development conventions/philosophy, but my
> 2c is that it's better not to enable it by default in order to
> minimise divergence from the code that users are running. OTOH it's
> not a major difference in behaviour so perhaps that's not a concern
> here.
>
> More significantly: I get the impression it's easier to do leak
> checking using LSAN, which requires recompiling git anyway - at which
> point you get the flag for free - so how often will people actually
> perform leak checking with Valgrind in the first place?

*Nod*, I didn't investigate the runtime penalty you and Jeff point
out. In any case, it seems that can also be done with valgrind exclusion
rules and/or manually ignoring these cases in the test wrapper.

>> 
>>>   builtin/clone.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/builtin/clone.c b/builtin/clone.c
>>> index 51e844a2de0a..952fe3d8fc88 100644
>>> --- a/builtin/clone.c
>>> +++ b/builtin/clone.c
>>> @@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>>   {
>>>   	int is_bundle = 0, is_local;
>>>   	const char *repo_name, *repo, *work_tree, *git_dir;
>>> -	char *path, *dir, *display_repo = NULL;
>>> +	char *path = NULL, *dir, *display_repo = NULL;
>>>   	int dest_exists, real_dest_exists = 0;
>>>   	const struct ref *refs, *remote_head;
>>> -	const struct ref *remote_head_points_at;
>>> +	struct ref *remote_head_points_at = NULL;
>>>   	const struct ref *our_head_points_at;
>>>   	struct ref *mapped_refs;
>>>   	const struct ref *ref;
>>> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>>   	repo_name = argv[0];
>>>     	path = get_repo_path(repo_name, &is_bundle);
>>> -	if (path)
>>> +	if (path) {
>>> +		FREE_AND_NULL(path);
>>>   		repo = absolute_pathdup(repo_name);
>>> -	else if (strchr(repo_name, ':')) {
>>> +	} else if (strchr(repo_name, ':')) {
>>>   		repo = repo_name;
>>>   		display_repo = transport_anonymize_url(repo);
>>>   	} else
>> In this case it seems better to just have a :
>>      int repo_heap = 0;
>>      Then set "repo_heap = 1" in that absolute_pathdup(repo_name)
>> branch,
>>      and...
>> 
>>> @@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>>   	strbuf_release(&reflog_msg);
>>>   	strbuf_release(&branch_top);
>>>   	strbuf_release(&key);
>>> +	free_refs(mapped_refs);
>>> +	free_refs(remote_head_points_at);
>>> +	free(dir);
>>> +	free(path);
>>> +	UNLEAK(repo);
>> Here do:
>>      if (repo_heap)
>>          free(repo);
>> 
>
> Although this is possible, I don't think it's worth it: if UNLEAK
> already exists, we might as well use it here to make the code
> simpler. And UNLEAK is unlikely to go away anytime soon
> because... (continued below)
>
>> But maybe there's some other out of the box way to make leak checking
>> Just Work without special flags in this case. I'm just noting this one
>> because it ended up being the only one that leaked unless I compiled
>> with -DSUPPRESS_ANNOTATED_LEAKS. I was fixing some leaks in the bundle
>> code.
>
> There are trickier examples where a cmd_* function has a complex
> struct on the stack, and correctly clearing all allocated memory
> pointed to by its members (or in turn further children with
> potentially multiple levels of indirection) is a lot of work - and
> that work doesn't actually benefit the user in any way. In other
> words, we either need to be able to use UNLEAK to suppress certain
> classes of uninteresting memory leaks - which allows us to focus on
> the interesting/real leaks - or someone has to spend a lot of time
> doing cleanup by hand (and/or someone has to implement a bunch of new
> cleanup functions)).
>
> In your example above, the UNLEAK can be avoided at the cost of one
> additional tracking variable - but in many other cases avoiding an 
> UNLEAK is much more expensive. It's certainly valid to debate the
> merits of the UNLEAK here, but that won't remove the need for UNLEAK's 
> existence in general.
>
> (The most common example that I remember is where cmd_* has a
> rev_info, and AFAICT there's no one-liner to clean that up. Using
> UNLEAK is honestly the best approach there. I don't think I've
> actually submitted any patches doing this, but I have a few in my
> local backlog.)

The thing I was patching happened to be making rev_info * not leak. I
probably didn't cover some more complex cases, but some simple cases
seem relatively easy.

I.e. it just doesn't have a release() function, and at least the things
I was looking at (bundle.c code) were relatively easy cases where we
were just missing a loop to free() data from some struct.

But yes, I agree that free()-ing just before we exit() is rather useless
in itself, the reason I wanted it is because it's a useful (although not
perfect) proxy for checking if the APIs the command uses as a one-off
leak when used as libraries, where we may be processing N items, later
doing other work etc.

We should probably eventually have a s/free/end_free()/g and imitate
perl(1)'s PERL_DESTRUCT_LEVEL option. I.e. you can globally configure
perl to run in a mode that assumes a one-off command, in that case
you'll just let the OS handle the cleanup, or one where you care about
memory leaks because you're using it e.g. as an embedded library.

But maybe it's not even worth it. In Perl the main benefit is that it's
a programming language with DESTROY handlers etc., so destruction can
often be expensive; turning it off entirely can also be buggy, imagine
relying on destructors to free temporary files etc.

We have that issue in theory with the interaction of atexit() handlers
and e.g. things that would behave differently at a distance if certain
thing were free()'d already, but in practice we probably don't.

But maybe it's not even worth pursuing. Have you (or anyone else) tried
e.g. benchmarking git's tests or t/perf tests where free() is defined to
be some noop stub? I'd expect it not to matter, but maybe I'm wrong...

>> Anyway, getting to the "default tests" point. I fixed a memory leak, and
>> wanted to it tested that the specific command doesn't leak in git's
>> default tests.
>> Do we have such a thing, if not why not?
>> The closest I got to getting this was:
>>      GIT_VALGRIND_MODE=memcheck
>> GIT_VALGRIND_OPTIONS="--leak-check=full
>> --errors-for-leak-kinds=definite --error-exitcode=123" <SOME TEST>
>> --valgrind
>
> It's easy to perform leak-checking runs *if* you're OK recompiling
> with LSAN, instead of using valgrind. My usual recipe for running
> against a range of tests is something like:

I thought valgrind would be a better approach since we might rely on it
just being there, so we could run some known-good commands that don't
leak even in a "normal" test run, but...

>   make SANITIZE=address,leak
>   ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" CFLAGS="-Og -g" 
> T="\$(wildcard t00[0-9][0-9]-*.sh)" test
>
> Additionally: I usually specify CC=clang, although gcc+LSAN has mostly
> been stable enough in my experience so you might be able to skip that.
> (I've found ASAN+LSAN to be more stable than LSAN by itself, which is
> why I specify address+leak, but adding ASAN in turn requires
> overriding ASAN_OPTIONS to reenable leak checking.)
>
> I don't know whether or not Valgrind is more/less effective at finding
> leaks, so being able to run the test suite under valgrind would be
> nice for comparison purposes though.

I didn't know how to set that up, that seems easy enough.

This works for me:

    make CC=clang SANITIZE=address,leak CFLAGS="-00 -g"
    (cd t && make ASAN_OPTIONS="<what you said>" [...])

I.e. it's just SANITIZE & flags that's important at compile-time. You
doubtless knew that, mainly for my own notes & others following along.

I ran it, noted the failing tests, produced a giant GIT_SKIP_TESTS list
and hacked ci/ to run that as a new linux-clang-SANITIZE job. That messy
WIP code is currently running at:
https://github.com/avar/git/runs/2793150092

Wouldn't it be a good idea to have such a job and slowly work on the
exclusion list?

E.g. I saw that t0004 failed, which was trivially fixed with a single
strbuf_release(), and we could guard against regressions.

Anyway, I can submit some cleaned-up patches for that. I was just
fishing for whether there was some good reason not to do it, since there
seemed to have been interest in leak fixes, but it hadn't made it into
CI / some "blessed" GIT_TEST_* mode or whatever. I.e. maybe the reports
were unstable or unreliable...


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-10 10:56   ` Ævar Arnfjörð Bjarmason
@ 2021-06-10 13:38     ` Jeff King
  2021-06-10 15:32       ` Andrzej Hunt
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2021-06-10 13:38 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Andrzej Hunt, git, Lénaïc Huard, Derrick Stolee

On Thu, Jun 10, 2021 at 12:56:55PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > More significantly: I get the impression it's easier to do leak
> > checking using LSAN, which requires recompiling git anyway - at which
> > point you get the flag for free - so how often will people actually
> > perform leak checking with Valgrind in the first place?
> 
> *Nod*, I didn't investigate the runtime penalty you and Jeff point
> out. In any case, it seems that can also be done with valgrind exclusion
> rules and/or manually ignoring these cases in the test wrapper.

I had trouble using valgrind's exclusions; there's more discussion in
0e5bba53af (add UNLEAK annotation for reducing leak false positives,
2017-09-08), but the gist of it is that it's awkward to annotate the
point of leak, rather than the point of allocation (so you have to
provide the complete callstack to the allocation, which is a maintenance
headache).

Of course, if you find ways to make useful annotations with valgrind,
I'm all for it. We have a few in t/valgrind already.

> But maybe it's not even worth pursuing. Have you (or anyone else) tried
> e.g. benchmarking git's tests or t/perf tests where free() is defined to
> be some noop stub? I'd expect it not to matter, but maybe I'm wrong...

I haven't. Even though I originated UNLEAK(), I'm not really all that
concerned about the cost of free() in general. My motivation for
introducing it (versus adding free() calls) was mostly about convenience
(complex data structures that don't have an easy free/release function,
but also the fact that you can still access data after marking it with
unleak).

The fact that it also preempts any arguments about the performance of
calling free() was just a bonus. ;)

To be clear, I could easily be convinced by real numbers that the cost
of free() at program end matters. I am just saying I am not one of the
people who is going to argue that position in the meantime.

> I didn't know how to set that up, that seems easy enough.
> 
> This works for me:
> 
>     make CC=clang SANITIZE=address,leak CFLAGS="-00 -g"
>     (cd t && make ASAN_OPTIONS="<what you said>" [...])
> 
> I.e. it's just SANITIZE & flags that's important at compile-time. You
> doubtless knew that, mainly for my own notes & others following along.

It should Just Work with:

  make SANITIZE=leak test

for both gcc and clang. You do need ASAN_OPTIONS if you're asking ASan
to do leak-checking (since we usually suppress that for the obvious
reason that almost every test fails). I'm not sure if using both ASan
and LSan together confuses LSan there (if so, it may be reasonable for
test-lib.sh to modify its ASAN_OPTIONS setting if LSan is enabled).

> I ran it, noted the failing tests, produced a giant GIT_SKIP_TESTS list
> and hacked ci/ to run that as a new linux-clang-SANITIZE job. That messy
> WIP code is currently running at:
> https://github.com/avar/git/runs/2793150092
> 
> Wouldn't it be a good idea to have such a job and slowly work on the
> exclusion list?
> 
> E.g. I saw that t0004 failed, which was trivially fixed with a single
> strbuf_release(), and we could guard against regressions.

I don't mind that. My intent was to get the whole suite clean
eventually, and then start worrying about regressions. But that may take
a while.

I do think it would be worth splitting out ASan from leak-checking. The
whole suite should run clean with regular ASan already, and we'd want to
find regressions there even in the tests that aren't leak-clean. I do
periodic ASan runs already; the main argument against doing it for every
CI run is just that's a lot more CPU. But maybe not enough to be
prohibitive? It's probably still way cheaper than running the test suite
on Windows.

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-10 13:38     ` Jeff King
@ 2021-06-10 15:32       ` Andrzej Hunt
  2021-06-10 16:36         ` Jeff King
  0 siblings, 1 reply; 10+ messages in thread
From: Andrzej Hunt @ 2021-06-10 15:32 UTC (permalink / raw)
  To: Jeff King, Ævar Arnfjörð Bjarmason
  Cc: git, Lénaïc Huard, Derrick Stolee



On 10/06/2021 15:38, Jeff King wrote:
>> I ran it, noted the failing tests, produced a giant GIT_SKIP_TESTS list
>> and hacked ci/ to run that as a new linux-clang-SANITIZE job. That messy
>> WIP code is currently running at:
>> https://github.com/avar/git/runs/2793150092
>>
>> Wouldn't it be a good idea to have such a job and slowly work on the
>> exclusion list?
>>
>> E.g. I saw that t0004 failed, which was trivially fixed with a single
>> strbuf_release(), and we could guard against regressions.
> 
> I don't mind that. My intent was to get the whole suite clean
> eventually, and then start worrying about regressions. But that may take
> a while.
> 
> I do think it would be worth splitting out ASan from leak-checking. The
> whole suite should run clean with regular ASan already, and we'd want to
> find regressions there even in the tests that aren't leak-clean. I do
> periodic ASan runs already; the main argument against doing it for every
> CI run is just that's a lot more CPU. But maybe not enough to be
> prohibitive? It's probably still way cheaper than running the test suite
> on Windows.

I've been running tests with ASAN in the Github Actions environment, and 
a single run takes just over 30 minutes [1] - which I believe is similar 
to the normal test jobs (they do run the test suite twice in that time I 
think).

I've been doing the same with UBSAN, and that's even faster at 15-20 
minutes [2]. However I get the impression that ASAN issues are both more 
common (at least on seen), and more impactful - so I would argue that 
ASAN should be prioritised if there's spare capacity. (I have no idea if 
ASAN+UBSAN can be combined, but I suspect that doing so would make the 
tests slower?)

I'm also running LSAN tests in CI to try and catch regressions, but I've 
only enabled a handful of tests so far. My much simpler approach was to 
specify the range of tests to run as 0-X, and as we make progress on 
fixing leaks, X will slowly approach 9999 (currently we're at something 
like X~=5, although I'm not too far off sending out some patches to 
boost that to 99). The skip-tests approach seems much more useful!

ATB,

   Andrzej

[1] https://github.com/ahunt/git/runs/2789921851?check_suite_focus=true
[2] https://github.com/ahunt/git/runs/2760632000?check_suite_focus=true


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-10 15:32       ` Andrzej Hunt
@ 2021-06-10 16:36         ` Jeff King
  2021-06-11 15:44           ` Andrzej Hunt
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2021-06-10 16:36 UTC (permalink / raw)
  To: Andrzej Hunt
  Cc: Ævar Arnfjörð Bjarmason, git,
	Lénaïc Huard, Derrick Stolee

On Thu, Jun 10, 2021 at 05:32:41PM +0200, Andrzej Hunt wrote:

> > I do think it would be worth splitting out ASan from leak-checking. The
> > whole suite should run clean with regular ASan already, and we'd want to
> > find regressions there even in the tests that aren't leak-clean. I do
> > periodic ASan runs already; the main argument against doing it for every
> > CI run is just that's a lot more CPU. But maybe not enough to be
> > prohibitive? It's probably still way cheaper than running the test suite
> > on Windows.
> 
> I've been running tests with ASAN in the Github Actions environment, and a
> single run takes just over 30 minutes [1] - which I believe is similar to
> the normal test jobs (they do run the test suite twice in that time I
> think).
> 
> I've been doing the same with UBSAN, and that's even faster at 15-20 minutes
> [2]. However I get the impression that ASAN issues are both more common (at
> least on seen), and more impactful - so I would argue that ASAN should be
> prioritised if there's spare capacity. (I have no idea if ASAN+UBSAN can be
> combined, but I suspect that doing so would make the tests slower?)

I routinely do SANITIZE=address,undefined since they are both useful
(and we do not trigger either in the current test suite). I never
measured the time of their combined use versus just one, but surely it's
faster the two-at-once approach is faster than running the test suite
twice.

> I'm also running LSAN tests in CI to try and catch regressions, but I've
> only enabled a handful of tests so far. My much simpler approach was to
> specify the range of tests to run as 0-X, and as we make progress on fixing
> leaks, X will slowly approach 9999 (currently we're at something like X~=5,
> although I'm not too far off sending out some patches to boost that to 99).
> The skip-tests approach seems much more useful!

Depending how fine-grained you get with skip-tests, it can create a
hassle as individual tests are removed or reordered (and now somebody
has to maintain the skip list). Doing it with whole scripts (whether
saying "these ones are OK" or "these ones are known bad") seems like
less maintenance overall. The results aren't as fine-grained, but for
something that is meant to be a transitional step, I'm not sure it's
worth the trouble to get more specific.

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-09 14:38 UNLEAK(), leak checking in the default tests etc Ævar Arnfjörð Bjarmason
  2021-06-09 17:44 ` Andrzej Hunt
@ 2021-06-10 19:01 ` SZEDER Gábor
  1 sibling, 0 replies; 10+ messages in thread
From: SZEDER Gábor @ 2021-06-10 19:01 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Andrzej Hunt, git, Andrzej Hunt, Jeff King,
	Lénaïc Huard, Derrick Stolee

On Wed, Jun 09, 2021 at 04:38:52PM +0200, Ævar Arnfjörð Bjarmason wrote:
>     GIT_VALGRIND_MODE=memcheck GIT_VALGRIND_OPTIONS="--leak-check=full --errors-for-leak-kinds=definite --error-exitcode=123" <SOME TEST> --valgrind
> 
> But as t/README notes it implies --verbose so we can't currently run it
> under the test harness (although I have out-of-tree patches to fix that
> in general).

'--valgrind' doesn't imply '--verbose' if '--verbose-log' was given,
and that works with the test harness just fine; see 88c6e9d31c
(test-lib: --valgrind should not override --verbose-log, 2017-09-05).


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UNLEAK(), leak checking in the default tests etc.
  2021-06-10 16:36         ` Jeff King
@ 2021-06-11 15:44           ` Andrzej Hunt
  0 siblings, 0 replies; 10+ messages in thread
From: Andrzej Hunt @ 2021-06-11 15:44 UTC (permalink / raw)
  To: Jeff King
  Cc: Ævar Arnfjörð Bjarmason, git,
	Lénaïc Huard, Derrick Stolee



On 10/06/2021 18:36, Jeff King wrote:
> On Thu, Jun 10, 2021 at 05:32:41PM +0200, Andrzej Hunt wrote:
> 
>>> I do think it would be worth splitting out ASan from leak-checking. The
>>> whole suite should run clean with regular ASan already, and we'd want to
>>> find regressions there even in the tests that aren't leak-clean. I do
>>> periodic ASan runs already; the main argument against doing it for every
>>> CI run is just that's a lot more CPU. But maybe not enough to be
>>> prohibitive? It's probably still way cheaper than running the test suite
>>> on Windows.
>>
>> I've been running tests with ASAN in the Github Actions environment, and a
>> single run takes just over 30 minutes [1] - which I believe is similar to
>> the normal test jobs (they do run the test suite twice in that time I
>> think).
>>
>> I've been doing the same with UBSAN, and that's even faster at 15-20 minutes
>> [2]. However I get the impression that ASAN issues are both more common (at
>> least on seen), and more impactful - so I would argue that ASAN should be
>> prioritised if there's spare capacity. (I have no idea if ASAN+UBSAN can be
>> combined, but I suspect that doing so would make the tests slower?)
> 
> I routinely do SANITIZE=address,undefined since they are both useful
> (and we do not trigger either in the current test suite). I never
> measured the time of their combined use versus just one, but surely it's
> faster the two-at-once approach is faster than running the test suite
> twice.

I'm seeing 33 minutes for SANITIZE=address,undefined - which is no 
slower than SANITIZE=address by itself (disclaimer: it's only one 
measurement):
https://github.com/ahunt/git/runs/2795642716?check_suite_focus=true
(The job's name is wrong but if you look in the logs you can confirm 
that it's using address+undefined.)

The usual linux and mac test-jobs are actually from 24 to 30 minutes 
(the numbers seem a bit variable) - with the exception of one faster 10 
minute job:
https://github.com/git/git/actions/runs/925771097
vs
https://github.com/git/git/actions/runs/927729395

So to summarise: adding an ASAN+UBSAN job would make things a bit 
slower, but not a huge amount slower.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-06-11 15:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 14:38 UNLEAK(), leak checking in the default tests etc Ævar Arnfjörð Bjarmason
2021-06-09 17:44 ` Andrzej Hunt
2021-06-09 20:36   ` Felipe Contreras
2021-06-10 10:46   ` Jeff King
2021-06-10 10:56   ` Ævar Arnfjörð Bjarmason
2021-06-10 13:38     ` Jeff King
2021-06-10 15:32       ` Andrzej Hunt
2021-06-10 16:36         ` Jeff King
2021-06-11 15:44           ` Andrzej Hunt
2021-06-10 19:01 ` SZEDER Gábor

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git