On Tue, Sep 05, 2017 at 03:05:12PM -0700, Stefan Beller wrote:
> On Tue, Sep 5, 2017 at 6:05 AM, Jeff King <[email protected]> wrote:
>
> > int main(void)
>
> nit of the day:
> s/void/int argc, char *argv/ or in case we do not
> want to emphasize the argument list s/void//
> as that adds no uninteresting things.
That really is a nit. I chose not to provide argv because it's longer
than "void" and I wasn't going to use the arguments. And I chose not to
use an empty argument list because it violates our style (as well as
arguably the C standard, though it leaves room for implementations to
take other forms of main).
> > In other words, you can do:
> >
> > int main(void)
> > {
> > char *p = some_function();
> > printf("%s", p);
> > UNLEAK(p);
> > return 0;
> > }
> >
> > to annotate "p" and suppress the leak report.
>
> This sounds really cool so far.
>
> After having a sneak peak at the implementation
> it is O(1) in runtime for each added element, and the
> space complexity is O(well).
I'm not sure if your "well" is "this does well" or "well, it could be
quite a lot". :)
It certainly has the potential to grow the heap without bound (since
after all, it's whole point is to make a giant list of variables that
are going out of scope). But in practice we'd sprinkle this over a
handful of variables just before program exit (and remember that it's
copying only what's on the stack already; so pointers get copied, not
whole heap-allocated blocks).
Plus it does nothing at all when not compiled with leak-checking. So I'm
not too worried about the extra memory usage or performance.
> > 1. It can be compiled conditionally. There's no need in
> > normal runs to do this free(), and it just wastes time.
> > By using a macro, we can get the benefit for leak-check
> > builds with zero cost for normal builds (this patch
> > uses a compile-time check, though we could clearly also
> > make it a run-time check at very low cost).
> >
> > Of course one could also hide free() behind a macro, so
> > this is really just arguing for having UNLEAK(), not
> > for its particular implementation.
>
> This is only a real argument in combination with (2), or in other
> words you seem to hint at situations like these:
Well, the numbered list was meant to be a set of arguments, each of
which contributes to the overall conclusion. :) I agree that (1) is the
weakest. Since both you and Martin seemed to get hung up on it, I'll
re-organize it a bit for the re-roll.
> 5. It's not just about worrying if we can call UNLEAK
> once (in 4), but we also do not have to worry about
> calling it twice, or recursively. (This argument can be bad
> for cargo cult programmers, but we don't have these ;-)
True. I didn't come across that case in any of the ones I converted. As
a more general rule, UNLEAK() doesn't access any pointed-to memory at
all. So it's fine with already-freed or even uninitialized memory (which
of course is technically wrong according to the standard, but in
practice would be fine, as we'd copy garbage that does not match a heap
block).
> > +#ifdef SUPPRESS_ANNOTATED_LEAKS
> > +extern void unleak_memory(const void *ptr, size_t len);
> > +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));
>
> As always with macros we have to be careful about its arguments.
>
> UNLEAK(a++)
> UNLEAK(baz())
>
> won't work as intended.
Yes, I intended this to be used only for actual variables. I couldn't
think of a way to enforce that at compile time with some kind of
BUILD_ASSERT (even requiring an lvalue isn't quite strict enough).
-Peff
