On Fri, Sep 23, 2022 at 10:28:29AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

In the long run, when all leaks are plugged, we'd want to ditch the
whole SANITIZE_LEAK system anyway. So we are better off preventing false
positives than trying to gloss over them.

In the long run when all leaks are plugged I'd prefer to be able to
compile a git with CFLAGS=-O3 and -fsanitize=leak, and have it run "git
config" without erroring out about a leak.

Why? Do you want to run a leak-checking copy of Git all the time? If so,
I have bad news for you performance-wise. Running the tests marked as
leak-passing with -O2 (but not -fsanitize=leak) takes ~101s of CPU.
Running with -O0 takes ~111s. Running with -fsanitize=leak takes ~241s.
So the improvement from compiler optimizations is not a big win there,
relatively speaking.

Or are you thinking that -O3 reveals new information we would not find
under other optimization levels? I don't think this is the case. While
that does sometimes find new opportunities for static analysis (via
inlining code, etc), I don't think it helps with run-time analysis. And
as we've seen here, it actively makes things _worse_ by introducing
false positives.

So I'd really prefer to keep this patch as-is. I'd agree with you if the
"whack-a-mole" was painful, but in this case it's really not. I think
it's just this one edge-case in the whole codebase (out of the things
marked leak-free).

Is it just this one spot? This is already the second one we've discussed
on the list, and I think you indicated there were more spots where you
intentionally held back on setting TEST_PASSES_SANITIZE_LEAK when you
saw hits under higher optimization levels.

It really is a potential problem anywhere we'd call a NORETURN function,
because the compiler (rightly) realizes there is no point in making sure
we can call a later free() that we'll never reach.

That's inching us closer to what I think should be the end goal,
i.e. this SANITIZE=leak is just a transition mechanism, so having it
enforce -O0 would mean that we can't have -fsanitize=leak Just Work in
the future (both for the tests & when manually compiled).

I think TEST_PASSES_SANITIZE_LEAK is a transition mechanism. But we'll
always want SANITIZE=leak, and I don't think it makes sense for ordinary
builds to use it.

This is just from vague recollection, and I couldn't find an example now
with some light digging (which was just seeing if any of the tests with
a "!SANITIZE_LEAK" prereq passed on -O0, but not -O3 with clang), but I
think I've run into cases where higher optimization levels have also
spotted genuine leaks that I've fixed.

If you have a counter-example, I'm all ears, but I have trouble
imagining how -O3 could actually find a leak not present in -O0. Again,
because we're testing runtime behavior, and the optimizations should not
be changing the visible behavior of our program (and the problem is the
leak checker itself is testing "invisible" things that may not actually
be important, like the state of a variable on the stack when we call
exit()).

-Peff