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

[Jeff King <peff@peff.net>]

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).

> 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