Re: UNLEAK(), leak checking in the default tests etc.

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

quoted

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