[PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise

[Jonathan Nieder <jrnieder@gmail.com>]

The current lockfile API often requires leaking memory:

 - the list of lockfiles to remove at exit is maintained in a
   singly linked list, so one cannot easily remove a lockfile
   record when it is no longer needed

 - some lockfile records are statically allocated and others are on
   the heap, so the remove_lock_file() atexit handler cannot easily
   free them at exit (not to mention that it would be a waste of time
   since _exit() takes care of freeing them anyway)

As the git libification effort proceeds, maybe this will become worth
fixing.  For now, since there are so few lock files used by a given
process, it is simpler to tolerate the leak.

Add a alloc_lock_file() helper to make such tolerance easier.
Allocations made through this helper function will not be reported as
leaks by valgrind even if there is no corresponding free().  The
((optimize("-fno-optimize-sibling-calls"))) attribute is needed on
platforms with GCC to ensure that the stack frame for
alloc_lock_file() is not replaced by the stack frame for xcalloc().

For illustration, this patch updates a few example xcalloc(1,
sizeof(struct lockfile)) call sites to use the new wrapper.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/technical/api-lockfile.txt |    9 +++++++++
 builtin/update-index.c                   |    4 +---
 cache-tree.c                             |    7 +------
 cache.h                                  |    1 +
 config.c                                 |    2 +-
 lockfile.c                               |    6 ++++++
 t/valgrind/default.supp                  |    8 ++++++++
 7 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index dd89404..8eac22a 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -22,6 +22,15 @@ The lockfile API serves two purposes:
 The functions
 -------------
 
+alloc_lock_file::
+
+	Allocate and clear a `struct lock_file`.  Because the
+	structure is used in an `atexit(3)` handler, its
+	storage has to stay throughout the life of the
+	program.  Using this allocation function instead of
+	`malloc` allows profiling tools like valgrind to keep
+	track of such storage and avoid reporting it as a leak.
+
 hold_lock_file_for_update::
 
 	Take a pointer to `struct lock_file`, the filename of
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3ab214d..7697631 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -591,9 +591,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
+	lock_file = alloc_lock_file();
 	newfd = hold_locked_index(lock_file, 0);
 	if (newfd < 0)
 		lock_error = errno;
diff --git a/cache-tree.c b/cache-tree.c
index f755590..81cf785 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -550,12 +550,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 	int entries, was_valid, newfd;
 	struct lock_file *lock_file;
 
-	/*
-	 * We can't free this memory, it becomes part of a linked list
-	 * parsed atexit()
-	 */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
+	lock_file = alloc_lock_file();
 	newfd = hold_locked_index(lock_file, 1);
 
 	entries = read_cache();
diff --git a/cache.h b/cache.h
index 2ef2fa3..9ee61a2 100644
--- a/cache.h
+++ b/cache.h
@@ -517,6 +517,7 @@ struct lock_file {
 #define LOCK_NODEREF 2
 extern int unable_to_lock_error(const char *path, int err);
 extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern struct lock_file *alloc_lock_file(void);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
diff --git a/config.c b/config.c
index 4b0a820..3f30e2b 100644
--- a/config.c
+++ b/config.c
@@ -1158,7 +1158,7 @@ int git_config_set_multivar(const char *key, const char *value,
 	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	lock = xcalloc(sizeof(struct lock_file), 1);
+	lock = alloc_lock_file();
 	fd = hold_lock_file_for_update(lock, config_filename, 0);
 	if (fd < 0) {
 		error("could not lock config file %s: %s", config_filename, strerror(errno));
diff --git a/lockfile.c b/lockfile.c
index b0d74cd..161dea8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -184,6 +184,12 @@ NORETURN void unable_to_lock_index_die(const char *path, int err)
 	die("%s", unable_to_lock_message(path, err));
 }
 
+__attribute__((optimize("-fno-optimize-sibling-calls")))
+struct lock_file *alloc_lock_file(void)
+{
+	return xcalloc(1, sizeof(struct lock_file));
+}
+
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 9e013fa..7789edd 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -43,3 +43,11 @@
 	fun:write_buffer
 	fun:write_loose_object
 }
+
+{
+	see api-lockfile.txt
+	Memcheck:Leak
+	fun:calloc
+	fun:xcalloc
+	fun:alloc_lock_file
+}
-- 
1.7.2.3