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
