Thread (2 messages) 2 messages, 2 authors, 2022-02-18

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

From: Junio C Hamano <hidden>
Date: 2022-02-16 18:30:25

Junio C Hamano [off-list ref] writes:
Ævar Arnfjörð Bjarmason  [off-list ref] writes:
quoted
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help