git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Oct 2, 2010, 08:32 UTC
Message-ID
<20101002083216.GC29638@burratino>
In-Reply-To
<20101002082752.GA29638@burratino>
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
Previous: Jonathan NiederNext: Andreas Ericsson
Message 13 of 33 in “memory leak reported by valgrind”
  1. memory leak reported by valgrindIvan Kanis, Aug 9, 2010
  2. Junio C HamanoAug 9, 2010
  3. Erik Faye-LundAug 9, 2010
  4. Alex RiesenAug 9, 2010
  5. Jonathan NiederAug 10, 2010
  6. 1/3 core: Stop leaking ondisk_cache_entrysJonathan Nieder, Aug 10, 2010
  7. 2/3 write-tree: Avoid leak when index refers to an invalid objectJonathan Nieder, Aug 10, 2010
  8. 3/3 read-tree: stop leaking tree objectsJonathan Nieder, Aug 10, 2010
  9. 0/7 Re: [PATCH] memory leak reported by valgrindJonathan Nieder, Oct 2, 2010
  10. 1/7 init: plug tiny one-time memory leakJonathan Nieder, Oct 2, 2010
  11. Junio C HamanoOct 3, 2010
  12. init: plug tiny one-time memory leakJonathan Nieder, Oct 4, 2010
  13. 2/7 lockfile: introduce alloc_lock_file() to avoid valgrind noiseJonathan Nieder, Oct 2, 2010
  14. Andreas EricssonOct 2, 2010
  15. Jonathan NiederOct 2, 2010
  16. Junio C HamanoOct 6, 2010
  17. Jonathan NiederOct 6, 2010
  18. Junio C HamanoOct 6, 2010
  19. Jonathan NiederOct 6, 2010
  20. 3/7 environment.c: remove unused variableJonathan Nieder, Oct 2, 2010
  21. 4/7 setup: make sure git dir path is in a permanent bufferJonathan Nieder, Oct 2, 2010
  22. Nguyen Thai Ngoc DuyOct 4, 2010
  23. 5/7 Introduce malloc/strdup/pathdup variants for permanent allocationsJonathan Nieder, Oct 2, 2010
  24. 6/7 environment: use alloc_permanent() for computed git_dir and coJonathan Nieder, Oct 2, 2010
  25. 7/7 commit-tree: free commit message before exitingJonathan Nieder, Oct 2, 2010
  26. Sverre RabbelierOct 2, 2010
  27. Jonathan NiederOct 2, 2010
  28. Sverre RabbelierOct 2, 2010
  29. Jonathan NiederOct 2, 2010
  30. Junio C HamanoOct 4, 2010
  31. Jonathan NiederOct 4, 2010
  32. Junio C HamanoOct 4, 2010
  33. Jonathan NiederOct 4, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.