Re: [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 27, 2021, 21:50 UTC
Message-ID: <20211027215053.2257548-1-jonathantanmy@google.com>
In-Reply-To: <cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com>

Show 12 quoted lines

> What started as a set of small memory leak fixes is now adding and
> usinge a die_message() function. This non-fatal-die() is useful to
> various callers that want to print "fatal: " before exiting, but don't
> want to call die() for whatever reason.
> 
> I wasn't planning to submit that now, but these were incomplete
> patches I had lying around, and make the 5th and 6th patch much nicer,
> in response to comments on v1 and v2 to the effect that managing
> free()-ing around die() functions was rather nasty.
> 
> This doesn't conflict with anything in-flight, and the changes
> themselves are rather simple.

Is this mainly to make a CI work, or just so that a certain set of tools work? (You mention an old version of GCC in patch 5.) If for CI, I think that there might be a sufficient version to fix this, but if not, I would think that something less intrusive would be better (e.g. a comment that certain versions do not work).

As for patches 5 and 6, I think that any leak detection we use has to consider any pointers still on the stack as "live" - if not we wouldn't be able to die without returning back to the topmost function since any intermediate function could have allocations (unless I'm mistaking something). (Unless die() is somehow overwriting the stackframe through some tail call optimization or something - in which case maybe what we should do is to disable the tail call optimization when we are checking for leaks.)