Re: [PATCH] tests: opt "git --config-env" test out of SANITIZE=leak

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Sep 23 2022, Jeff King wrote:

> On Fri, Sep 23, 2022 at 10:28:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > 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...).

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