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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 26, 2022, 08:56 UTC
Message-ID: <220926.86tu4u33m2.gmgdl@evledraar.gmail.com>
In-Reply-To: <Yy4eo6500C0ijhk+@coredump.intra.peff.net>

On Fri, Sep 23 2022, Jeff King wrote:

Show 11 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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