On Tue, Sep 05, 2017 at 03:05:12PM -0700, Stefan Beller wrote: > On Tue, Sep 5, 2017 at 6:05 AM, Jeff King <p...@peff.net> 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