Re: [PATCH 0/2] UNLEAK style fixes

From: Jeff King <peff@peff.net>
Date: Aug 14, 2020, 10:34 UTC
Message-ID: <20200814103451.GD3312240@coredump.intra.peff.net>
In-Reply-To: <CAPig+cTOcQymWWtSY3UN73_fpaWUs3u66+EZWBp1SvXeUrgsQQ@mail.gmail.com>

On Thu, Aug 13, 2020 at 03:32:56PM -0400, Eric Sunshine wrote:

Show 18 quoted lines

> On Thu, Aug 13, 2020 at 11:54 AM Jeff King <peff@peff.net> wrote:
> > (As a side note, if we want to declare UNLEAK() a failure because nobody
> > cares enough to really use it, I'm OK with that, too).
> 
> Perhaps the reason that UNLEAK() has not been particularly successful,
> in general, is that it requires extra knowledge and reasoning to know
> when to use it and how to do so properly. Couple that with the fact
> that the scope of cases where it can be used is quite narrow compared
> to sum total of all code in project for which we simply free resources
> when we're done with them. So, it's hard to keep the specialized
> UNLEAK() knowledge in one's head.
> 
> Speaking from personal experience, the several times I have had to
> deal with UNLEAK(), I had to re-learn it from scratch each time. That
> meant studying the header comment, studying the implementation, and
> studying existing callers before things "clicked" enough to be able to
> feel confident about how to use it (assuming it wasn't false
> confidence).

I think this is really the meat of it. I never intended UNLEAK() to be something people dealt with unless they were trying to get LSAN or valgrind to run without complaining.

Show 5 quoted lines

> That all represents a lot of cognitive overhead versus the common
> practice of simply freeing resources when you're done with them, which
> requires no extra cognitive load since it is something we think about
> _always_ when working with a language like C with no built-in garbage
> collection.

To be clear, I have no problem with _actually_ freeing resources if that's an option. The point of UNLEAK() was:

  - to help with structs that don't have an easy way to free all
    elements (e.g., rev_info)

  - to preempt arguments about whether calling free(buf) right before
    programming exit is wasted effort. Whereas UNLEAK() is true
    zero-cost for non-leak-checking builds.

  - to avoid asking people to rewrite:

      return foo(bar);

     into:

       ret = foo(bar);
       free(bar);
       return ret;

So we could go that direction, but I'd wait on it until somebody feels like sinking some time into getting us leak-checker-clean.

In the meantime, I have a slight preference to leave UNLEAK() there as a potential tool for somebody digging into leak-checkers. But we almost certainly shouldn't be asking new authors to use it in reviews, etc. TBH, I'm not sure why people starting sprinkling UNLEAK() around in the first place. ;)

-Peff