Re: [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity

From: Jeff King <peff@peff.net>
Date: Oct 26, 2021, 08:53 UTC
Message-ID: <YXfCH7I1XwH+Vetu@coredump.intra.peff.net>
In-Reply-To: <patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com>

On Fri, Oct 22, 2021 at 08:19:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On my GCC version (10.2.1-6), but not the clang I have available t0017
> will fail under SANITIZE=leak on optimization levels higher than -O0,
> which is annoying when combined with the change in 956d2e4639b (tests:
> add a test mode for SANITIZE=leak, run it in CI, 2021-09-23).

This one really makes me sad. The resulting code is more complicated, and what guarantee do we have that we won't run into similar problems with other die() calls?

If we're getting false positives, I'd rather see us work around them with annotations, or a better compiler (I couldn't reproduce with gcc 10.3.0 or 11.2.0 from Debian, so I doubt there is even much point in reporting it upstream).

Show 6 quoted lines

> We really do have a memory leak here in either case, as e.g. running
> the pre-image under valgrind(1) will reveal. It's documented
> SANITIZE=leak (and "address", which exhibits the same behavior) might
> interact with compiler optimization in this way in some cases. Since
> this function is called recursively it's going to be especially
> interesting as an optimization target.

I don't see how we have a leak. If we hit this die code-path then we never exit the function. I can't reproduce the problem, but it sounds like -O2 is reusing the stack space of "expanded" to prepare for the die() call? IMHO that is not an actual leak. It is still in scope from the perspective of C, and anyway we are about to exit from within the die().

If we were to do anything in the code itself, I'd much prefer to hit it with an UNLEAK().

-Peff