common/memleak: simplify and document API.

1. Rename memleak_enter_allocations to memleak_find_allocations.
2. Unify scanning for pointers into memleak_remove_region / memleak_remove_pointer.
3. Document the functions.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld/channeld.c

@@ -3088,10 +3088,10 @@ static void handle_dev_memleak(struct peer *peer, const u8 *msg)
 	struct htable *memtable;
 	bool found_leak;
 
-	memtable = memleak_enter_allocations(tmpctx, msg, msg);
+	memtable = memleak_find_allocations(tmpctx, msg, msg);
 
 	/* Now delete peer and things it has pointers to. */
-	memleak_remove_referenced(memtable, peer);
+	memleak_remove_region(memtable, peer, tal_bytelen(peer));
 
 	found_leak = dump_memleak(memtable);
 	wire_sync_write(MASTER_FD,

closingd/closingd.c

@@ -581,14 +581,12 @@ static void closing_dev_memleak(const tal_t *ctx,
 {
 	struct htable *memtable;
 
-	memtable = memleak_enter_allocations(tmpctx,
-					     scriptpubkey[LOCAL],
-					     scriptpubkey[REMOTE]);
+	memtable = memleak_find_allocations(tmpctx, NULL, NULL);
 
-	/* Now delete known pointers (these aren't really roots, just
-	 * pointers we know are referenced).*/
-	memleak_remove_referenced(memtable, ctx);
-	memleak_remove_referenced(memtable, funding_wscript);
+	memleak_remove_pointer(memtable, ctx);
+	memleak_remove_pointer(memtable, scriptpubkey[LOCAL]);
+	memleak_remove_pointer(memtable, scriptpubkey[REMOTE]);
+	memleak_remove_pointer(memtable, funding_wscript);
 
 	dump_memleak(memtable);
 }
@@ -653,7 +651,7 @@ int main(int argc, char *argv[])
 		master_badmsg(WIRE_CLOSINGD_INIT, msg);
 
 	/* stdin == requests, 3 == peer, 4 = gossip, 5 = gossip_store, 6 = hsmd */
-	per_peer_state_set_fds(pps, 3, 4, 5);
+	per_peer_state_set_fds(notleak(pps), 3, 4, 5);
 
 	snprintf(fee_negotiation_step_str, sizeof(fee_negotiation_step_str),
 		 "%" PRIu64 "%s", fee_negotiation_step,

common/memleak.c

@@ -1,3 +1,24 @@
+/* Hello friends!
+ *
+ * You found me!  This is the inner, deep magic.  Right here.
+ *
+ * To help development, we have a complete set of routines to scan for
+ * tal-memory leaks (valgrind will detect non-tal memory leaks at exit,
+ * but tal hierarchies tends to get freed at exit, so we need something
+ * more sophisticated).
+ *
+ * Memleak detection is only active if DEVELOPER is set.  It does several
+ * things:
+ * 1. attaches a backtrace list to every allocation, so we can tell
+ *    where it came from.
+ * 2. when memleak_find_allocations() is called, walks the entire tal
+ *    tree and saves a pointer to all the objects it finds, with
+ *    a few internal exceptions (including everything under 'tmpctx').
+ *    It then calls registered helpers, which can remove opaque things
+ *    and handles notleak() objects.
+ * 3. provides a routine to access any remaining pointers in the
+ *    table: these are the leaks.
+ */
 #include <assert.h>
 #include <backtrace.h>
 #include <ccan/crypto/siphash24/siphash24.h>
@@ -102,8 +123,8 @@ static void scan_for_pointers(struct htable *memtable,
 	}
 }
 
-void memleak_scan_region(struct htable *memtable,
-			 const void *ptr, size_t bytelen)
+void memleak_remove_region(struct htable *memtable,
+			   const void *ptr, size_t bytelen)
 {
 	pointer_referenced(memtable, ptr);
 	scan_for_pointers(memtable, ptr, bytelen);
@@ -118,15 +139,6 @@ static void remove_with_children(struct htable *memtable, const tal_t *p)
 		remove_with_children(memtable, i);
 }
 
-void memleak_remove_referenced(struct htable *memtable, const void *root)
-{
-	/* Now delete the ones which are referenced. */
-	memleak_scan_region(memtable, root, tal_bytelen(root));
-
-	/* Remove memtable itself */
-	pointer_referenced(memtable, memtable);
-}
-
 /* memleak can't see inside hash tables, so do them manually */
 void memleak_remove_htable(struct htable *memtable, const struct htable *ht)
 {
@@ -134,7 +146,7 @@ void memleak_remove_htable(struct htable *memtable, const struct htable *ht)
 	const void *p;
 
 	for (p = htable_first(ht, &i); p; p = htable_next(ht, &i))
-		memleak_scan_region(memtable, p, tal_bytelen(p));
+		memleak_remove_region(memtable, p, tal_bytelen(p));
 }
 
 /* FIXME: If uintmap used tal, this wouldn't be necessary! */
@@ -144,7 +156,7 @@ void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m)
 	intmap_index_t i;
 
 	for (p = intmap_first_(m, &i); p; p = intmap_after_(m, &i))
-		memleak_scan_region(memtable, p, tal_bytelen(p));
+		memleak_remove_region(memtable, p, tal_bytelen(p));
 }
 
 static bool ptr_match(const void *candidate, void *ptr)
@@ -157,6 +169,9 @@ const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace)
 	struct htable_iter it;
 	const tal_t *i, *p;
 
+	/* Remove memtable itself */
+	pointer_referenced(memtable, memtable);
+
 	i = htable_first(memtable, &it);
 	if (!i)
 		return NULL;
@@ -239,7 +254,7 @@ static void call_memleak_helpers(struct htable *memtable, const tal_t *p)
 				remove_with_children(memtable, p);
 			else
 				pointer_referenced(memtable, p);
-			memleak_scan_region(memtable, p, tal_bytelen(p));
+			memleak_remove_region(memtable, p, tal_bytelen(p));
 		} else if (name && strends(name, "_notleak")) {
 			pointer_referenced(memtable, i);
 			call_memleak_helpers(memtable, i);
@@ -249,9 +264,9 @@ static void call_memleak_helpers(struct htable *memtable, const tal_t *p)
 	}
 }
 
-struct htable *memleak_enter_allocations(const tal_t *ctx,
-					 const void *exclude1,
-					 const void *exclude2)
+struct htable *memleak_find_allocations(const tal_t *ctx,
+					const void *exclude1,
+					const void *exclude2)
 {
 	struct htable *memtable = tal(ctx, struct htable);
 	htable_init(memtable, hash_ptr, NULL);

common/memleak.h

@@ -8,26 +8,48 @@
 
 struct htable;
 
+/**
+ * memleak_init: Initialize memleak detection; you call this at the start!
+ *
+ * notleak() won't have an effect if called before this (but naming
+ * tal objects with suffix _notleak works).
+ */
+void memleak_init(void);
+
+/**
+ * notleak: remove a false-positive tal object.
+ * @p: the tal allocation.
+ *
+ * This marks a tal pointer (and anything it refers to) as not being
+ * leaked.  Think hard before using this!
+ */
+#define notleak(p) ((memleak_typeof(p))notleak_((p), false))
+
+/* Mark a pointer and all its tal children as not being leaked.
+ * You don't want this; it's for simplifying handling of the incoming
+ * command which asks lightningd to do the dev check. */
+#define notleak_with_children(p) ((memleak_typeof(p))notleak_((p), true))
+
 #if HAVE_TYPEOF
 #define memleak_typeof(var) typeof(var)
 #else
 #define memleak_typeof(var) void *
 #endif /* !HAVE_TYPEOF */
 
-/* Mark a pointer as not being leaked. */
-#define notleak(p) ((memleak_typeof(p))notleak_((p), false))
-
-/* Mark a pointer and all its tal children as not being leaked. */
-#define notleak_with_children(p) ((memleak_typeof(p))notleak_((p), true))
-
 void *notleak_(const void *ptr, bool plus_children);
 
-/* Mark a helper to be called to scan this structure for mem references */
-/* For update-mock: memleak_add_helper_ mock empty */
-void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable,
-						    const tal_t *));
-
 #if DEVELOPER
+/**
+ * memleak_add_helper: help memleak look inside this tal object
+ * @p: the tal object
+ * @cb: the callback.
+ *
+ * memleak looks for tal pointers inside a tal object memory, but some
+ * structures which use bit-stealing on pointers or use non-tal allocations
+ * will need this.
+ *
+ * The callback usually calls memleak_remove_*.
+ */
 #define memleak_add_helper(p, cb)					\
 	memleak_add_helper_((p),					\
 			    typesafe_cb_preargs(void, const tal_t *,	\
@@ -38,32 +60,63 @@ void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable,
 #define memleak_add_helper(p, cb)
 #endif
 
-/* Initialize memleak detection */
-void memleak_init(void);
+/* For update-mock: memleak_add_helper_ mock empty */
+void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable,
+						    const tal_t *));
 
-/* Allocate a htable with all the memory we've allocated. */
-struct htable *memleak_enter_allocations(const tal_t *ctx,
-					 const void *exclude1,
-					 const void *exclude2);
+/**
+ * memleak_find_allocations:  allocate a htable with all tal objects;
+ * @ctx: the context to allocate the htable from
+ * @exclude1: one tal pointer to exclude from adding (if non-NULL)
+ * @exclude2: second tal pointer to exclude from adding (if non-NULL)
+ *
+ * Note that exclude1 and exclude2's tal children are also not added.
+ */
+struct htable *memleak_find_allocations(const tal_t *ctx,
+					const void *exclude1,
+					const void *exclude2);
 
-/* Remove any pointers to memory under root */
-void memleak_remove_referenced(struct htable *memtable, const void *root);
+/**
+ * memleak_remove_region - remove this region and anything it references
+ * @memtable: the memtable create by memleak_find_allocations.
+ * @p: the pointer to remove.
+ * @bytelen: the bytes within it to scan for more pointers.
+ *
+ * This removes @p from the memtable, then looks for any tal pointers
+ * inside between @p and @p + @bytelen and calls
+ * memleak_remove_region() on those if not already removed.
+ */
+void memleak_remove_region(struct htable *memtable,
+			   const void *p, size_t bytelen);
 
-/* Remove any pointers inside this htable (which is opaque to memleak). */
+/**
+ * memleak_remove_pointer - remove this pointer
+ * @memtable: the memtable create by memleak_find_allocations.
+ * @p: the pointer to remove.
+ *
+ * This removes @p from the memtable.
+ */
+#define memleak_remove_pointer(memtable, p) \
+	memleak_remove_region((memtable), (p), 0)
+
+/* Helper to remove objects inside this htable (which is opaque to memleak). */
 void memleak_remove_htable(struct htable *memtable, const struct htable *ht);
 
-/* Remove any pointers inside this uintmap (which is opaque to memleak). */
+/* Helper to remove objects inside this uintmap (which is opaque to memleak). */
 #define memleak_remove_uintmap(memtable, umap)		\
 	memleak_remove_intmap_(memtable, uintmap_unwrap_(umap))
 
 struct intmap;
 void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m);
 
-/* Mark this pointer as being referenced, and search within for more. */
-void memleak_scan_region(struct htable *memtable,
-			 const void *p, size_t bytelen);
-
-/* Get (and remove) a leak from memtable, or NULL */
+/**
+ * memleak_get: get (and remove) a leak from memtable, or NULL
+ * @memtable: the memtable after all known allocations removed.
+ * @backtrace: the backtrace to set if there is one.
+ *
+ * If this returns NULL, it means the @memtable was empty.  Otherwise
+ * it return a pointer to a leak (and removes it from @memtable)
+ */
 const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace);
 
 extern struct backtrace_state *backtrace_state;

connectd/connectd.c

@@ -1571,10 +1571,10 @@ static struct io_plan *dev_connect_memleak(struct io_conn *conn,
 	struct htable *memtable;
 	bool found_leak;
 
-	memtable = memleak_enter_allocations(tmpctx, msg, msg);
+	memtable = memleak_find_allocations(tmpctx, msg, msg);
 
 	/* Now delete daemon and those which it has pointers to. */
-	memleak_remove_referenced(memtable, daemon);
+	memleak_remove_region(memtable, daemon, sizeof(daemon));
 
 	found_leak = dump_memleak(memtable);
 	daemon_conn_send(daemon->master,

gossipd/gossipd.c

@@ -1346,10 +1346,10 @@ static struct io_plan *dev_gossip_memleak(struct io_conn *conn,
 	struct htable *memtable;
 	bool found_leak;
 
-	memtable = memleak_enter_allocations(tmpctx, msg, msg);
+	memtable = memleak_find_allocations(tmpctx, msg, msg);
 
 	/* Now delete daemon and those which it has pointers to. */
-	memleak_remove_referenced(memtable, daemon);
+	memleak_remove_region(memtable, daemon, sizeof(*daemon));
 
 	found_leak = dump_memleak(memtable);
 	daemon_conn_send(daemon->master,

hsmd/hsmd.c

@@ -1799,17 +1799,18 @@ static struct io_plan *handle_memleak(struct io_conn *conn,
 	bool found_leak;
 	u8 *reply;
 
-	memtable = memleak_enter_allocations(tmpctx, msg_in, msg_in);
+	memtable = memleak_find_allocations(tmpctx, msg_in, msg_in);
 
 	/* Now delete clients and anything they point to. */
-	memleak_remove_referenced(memtable, c);
-	memleak_scan_region(memtable,
-			    dbid_zero_clients, sizeof(dbid_zero_clients));
+	memleak_remove_region(memtable, c, tal_bytelen(c));
+	memleak_remove_region(memtable,
+			      dbid_zero_clients, sizeof(dbid_zero_clients));
 	memleak_remove_uintmap(memtable, &clients);
-	memleak_scan_region(memtable, status_conn, tal_bytelen(status_conn));
+	memleak_remove_region(memtable,
+			      status_conn, tal_bytelen(status_conn));
 
-	memleak_scan_region(memtable, dev_force_privkey, 0);
-	memleak_scan_region(memtable, dev_force_bip32_seed, 0);
+	memleak_remove_pointer(memtable, dev_force_privkey);
+	memleak_remove_pointer(memtable, dev_force_bip32_seed);
 
 	found_leak = dump_memleak(memtable);
 	reply = towire_hsmd_dev_memleak_reply(NULL, found_leak);

lightningd/memdump.c

@@ -132,7 +132,7 @@ static bool handle_strmap(const char *member, void *p, void *memtable_)
 {
 	struct htable *memtable = memtable_;
 
-	memleak_scan_region(memtable, p, tal_bytelen(p));
+	memleak_remove_region(memtable, p, tal_bytelen(p));
 
 	/* Keep going */
 	return true;
@@ -154,7 +154,7 @@ static void scan_mem(struct command *cmd,
 	const uintptr_t *backtrace;
 
 	/* Enter everything, except this cmd and its jcon */
-	memtable = memleak_enter_allocations(cmd, cmd, cmd->jcon);
+	memtable = memleak_find_allocations(cmd, cmd, cmd->jcon);
 
 	/* First delete known false positives. */
 	memleak_remove_htable(memtable, &ld->topology->txwatches.raw);
@@ -164,7 +164,7 @@ static void scan_mem(struct command *cmd,
 	memleak_remove_htable(memtable, &ld->htlc_sets.raw);
 
 	/* Now delete ld and those which it has pointers to. */
-	memleak_remove_referenced(memtable, ld);
+	memleak_remove_region(memtable, ld, sizeof(*ld));
 
 	json_array_start(response, "leaks");
 	while ((i = memleak_get(memtable, &backtrace)) != NULL) {

onchaind/onchaind.c

@@ -2087,24 +2087,13 @@ static void handle_preimage(struct tracked_output **outs,
 #if DEVELOPER
 static void memleak_remove_globals(struct htable *memtable, const tal_t *topctx)
 {
-	/* memleak_scan_region is overkill if these are simple pointers to
-	 * objects which don't contain pointers, but it works. */
 	if (keyset)
-		memleak_scan_region(memtable, keyset, sizeof(*keyset));
-
-	if (remote_per_commitment_point)
-		memleak_scan_region(memtable, remote_per_commitment_point,
-				    sizeof(*remote_per_commitment_point));
-
-	if (remote_per_commitment_secret)
-		memleak_scan_region(memtable, remote_per_commitment_secret,
-				    sizeof(*remote_per_commitment_secret));
-
-	/* top-level context args */
-	memleak_scan_region(memtable, topctx, 0);
-
-	memleak_scan_region(memtable, missing_htlc_msgs,
-			    tal_bytelen(missing_htlc_msgs));
+		memleak_remove_region(memtable, keyset, sizeof(*keyset));
+	memleak_remove_pointer(memtable, remote_per_commitment_point);
+	memleak_remove_pointer(memtable, remote_per_commitment_secret);
+	memleak_remove_pointer(memtable, topctx);
+	memleak_remove_region(memtable,
+			      missing_htlc_msgs, tal_bytelen(missing_htlc_msgs));
 }
 
 static bool handle_dev_memleak(struct tracked_output **outs, const u8 *msg)
@@ -2115,10 +2104,10 @@ static bool handle_dev_memleak(struct tracked_output **outs, const u8 *msg)
 	if (!fromwire_onchaind_dev_memleak(msg))
 		return false;
 
-	memtable = memleak_enter_allocations(tmpctx, msg, msg);
+	memtable = memleak_find_allocations(tmpctx, msg, msg);
 	/* Top-level context is parent of outs */
 	memleak_remove_globals(memtable, tal_parent(outs));
-	memleak_remove_referenced(memtable, outs);
+	memleak_remove_region(memtable, outs, tal_bytelen(outs));
 
 	found_leak = dump_memleak(memtable);
 	wire_sync_write(REQ_FD,

onchaind/test/run-grind_feerate-bug.c

@@ -112,18 +112,15 @@ struct bitcoin_tx *htlc_success_tx(const tal_t *ctx UNNEEDED,
 /* Generated stub for master_badmsg */
 void master_badmsg(u32 type_expected UNNEEDED, const u8 *msg)
 { fprintf(stderr, "master_badmsg called!\n"); abort(); }
-/* Generated stub for memleak_enter_allocations */
-struct htable *memleak_enter_allocations(const tal_t *ctx UNNEEDED,
-					 const void *exclude1 UNNEEDED,
-					 const void *exclude2 UNNEEDED)
-{ fprintf(stderr, "memleak_enter_allocations called!\n"); abort(); }
-/* Generated stub for memleak_remove_referenced */
-void memleak_remove_referenced(struct htable *memtable UNNEEDED, const void *root UNNEEDED)
-{ fprintf(stderr, "memleak_remove_referenced called!\n"); abort(); }
-/* Generated stub for memleak_scan_region */
-void memleak_scan_region(struct htable *memtable UNNEEDED,
-			 const void *p UNNEEDED, size_t bytelen UNNEEDED)
-{ fprintf(stderr, "memleak_scan_region called!\n"); abort(); }
+/* Generated stub for memleak_find_allocations */
+struct htable *memleak_find_allocations(const tal_t *ctx UNNEEDED,
+					const void *exclude1 UNNEEDED,
+					const void *exclude2 UNNEEDED)
+{ fprintf(stderr, "memleak_find_allocations called!\n"); abort(); }
+/* Generated stub for memleak_remove_region */
+void memleak_remove_region(struct htable *memtable UNNEEDED,
+			   const void *p UNNEEDED, size_t bytelen UNNEEDED)
+{ fprintf(stderr, "memleak_remove_region called!\n"); abort(); }
 /* Generated stub for new_coin_chain_fees */
 struct chain_coin_mvt *new_coin_chain_fees(const tal_t *ctx UNNEEDED,
 					   const char *account_name UNNEEDED,

onchaind/test/run-grind_feerate.c

@@ -126,18 +126,15 @@ struct bitcoin_tx *htlc_timeout_tx(const tal_t *ctx UNNEEDED,
 /* Generated stub for master_badmsg */
 void master_badmsg(u32 type_expected UNNEEDED, const u8 *msg)
 { fprintf(stderr, "master_badmsg called!\n"); abort(); }
-/* Generated stub for memleak_enter_allocations */
-struct htable *memleak_enter_allocations(const tal_t *ctx UNNEEDED,
-					 const void *exclude1 UNNEEDED,
-					 const void *exclude2 UNNEEDED)
-{ fprintf(stderr, "memleak_enter_allocations called!\n"); abort(); }
-/* Generated stub for memleak_remove_referenced */
-void memleak_remove_referenced(struct htable *memtable UNNEEDED, const void *root UNNEEDED)
-{ fprintf(stderr, "memleak_remove_referenced called!\n"); abort(); }
-/* Generated stub for memleak_scan_region */
-void memleak_scan_region(struct htable *memtable UNNEEDED,
-			 const void *p UNNEEDED, size_t bytelen UNNEEDED)
-{ fprintf(stderr, "memleak_scan_region called!\n"); abort(); }
+/* Generated stub for memleak_find_allocations */
+struct htable *memleak_find_allocations(const tal_t *ctx UNNEEDED,
+					const void *exclude1 UNNEEDED,
+					const void *exclude2 UNNEEDED)
+{ fprintf(stderr, "memleak_find_allocations called!\n"); abort(); }
+/* Generated stub for memleak_remove_region */
+void memleak_remove_region(struct htable *memtable UNNEEDED,
+			   const void *p UNNEEDED, size_t bytelen UNNEEDED)
+{ fprintf(stderr, "memleak_remove_region called!\n"); abort(); }
 /* Generated stub for new_coin_chain_fees */
 struct chain_coin_mvt *new_coin_chain_fees(const tal_t *ctx UNNEEDED,
 					   const char *account_name UNNEEDED,

openingd/dualopend.c

@@ -1223,10 +1223,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg)
 
 	/* Populate a hash table with all our allocations (except msg, which
 	 * is in use right now). */
-	memtable = memleak_enter_allocations(tmpctx, msg, msg);
+	memtable = memleak_find_allocations(tmpctx, msg, msg);
 
 	/* Now delete state and things it has pointers to. */
-	memleak_remove_referenced(memtable, state);
+	memleak_remove_region(memtable, state, tal_bytelen(state));
 
 	/* If there's anything left, dump it to logs, and return true. */
 	found_leak = dump_memleak(memtable);

openingd/openingd.c

@@ -1262,10 +1262,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg)
 
 	/* Populate a hash table with all our allocations (except msg, which
 	 * is in use right now). */
-	memtable = memleak_enter_allocations(tmpctx, msg, msg);
+	memtable = memleak_find_allocations(tmpctx, msg, msg);
 
 	/* Now delete state and things it has pointers to. */
-	memleak_remove_referenced(memtable, state);
+	memleak_remove_region(memtable, state, sizeof(*state));
 
 	/* If there's anything left, dump it to logs, and return true. */
 	found_leak = dump_memleak(memtable);