Re: [PATCH 2/2] commit: use strbuf_release() instead of UNLEAK()

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Convert the UNLEAK() added in 0e5bba53af7 (add UNLEAK annotation for
>> reducing leak false positives, 2017-09-08) to release the memory using
>> strbuf_release() instead.
>>
>> The tests being marked as passing with
>> "TEST_PASSES_SANITIZE_LEAK=true" already passed before due to the
>> UNLEAK(), but now they really don't leak memory, so let's mark them as
>> such.
>
> That smells like a brave move.
>
> Specifically, the cited commit turned an existing strbuf_release()
> on &err into UNLEAK().  If that and the other strbuf (sb) were so
> easily releasable, why didn't we do so back then already?

I suspect that the answer to the above question is because these
allocations are in the top-level cmd_commit() function, which is
never called recursively or repeatedly as a subroutine.  The only
significant thing that happens after we return from it is to exit.

In such a code path, marking a variable as UNLEAK() is a better
thing to do than calling strbuf_release().  Both will work as a way
to squelch sanitizers from reporting a leak that does not matter,
but calling strbuf_release() means we'd spend extra cycles to return
pieces of memory to the pool, even though we know that the pool
itself will be cleaned immediately later at exit.

We already have UNLEAK to tell sanitizers not to distract us from
spotting and plugging real leaks by reporting these apparent leaks
that do not matter.  It is of somewhat dubious value to do a "we
care too much about pleasing sanitizer and spend extra cycles at
runtime while real users are doing real work" change.