On Fri, Sep 23 2022, Jeff King wrote:

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

quoted

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? 

Not all the time, yes, and I've been getting closer to compiling my
daily driver with it..

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.

Yeah, I know it sucks, but I use it for interactive use, "git push" and
the like, so I usually don't care if it's ~2x slower. I even run with
SANITIZE=address sometimes.

Wanting to have non-O0 there is less about thinking the higher -On
helps, and more to avoid it being a special snowflake, i.e. if I run
with -O2 -g usually I'd like to just add -fsanitize=leak to that, and
not have to also change the optimization level.

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.

I think this (and to your "conter-example" below) is correct on your
part. I.e. I don't see a good reason for why this would happen.

I have been able to reliably reproduce some leaks as being flaky (and
have avoided adding them to the tests). I wonder if that explains it,
i.e. there was another underlying issue, and the optimization level
happened to trigger some race (or whatever was going on there, I haven't
looked in any depth into those either...).

quoted

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.

Probably not the only spot, but the only spot under the
TEST_PASSES_SANITIZE_LEAK umbrella.

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.

FWIW I'm not sure it's NORETURN, and haven't had time to dig...