PATCH: make valgrind understand zsh heaps

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

This teaches valgrind about zsh heaps so it should be able to analyse
errors in them better when configured with --enable-zsh-valgrind.

It seems to be basically OK --- I don't get any unexpected errors ---
but it's not ideal because in hrealloc() I'm marking chunks as freed and
then allocated again since I couldn't get it to work using
VALGRIND_MEMPOOL_CHANGED(), although I have fixed one bug since then.
Feel free to have a go.

Valgrind apparently got confused by a zero-length allocation, so that's
now a NULL pointer, but only if ZSH_VALGRIND is defined.

I played with the heap allocation in lex.c to confirm errors I was having
earlier on were to do with hrealloc, so that change isn't needed any
more, but I've left the size #define'd for convenience and clarity.

diff --git a/Src/lex.c b/Src/lex.c
index d0f8450..8e9a49f 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -30,6 +30,8 @@
 #include "zsh.mdh"
 #include "lex.pro"
 
+#define LEX_HEAP_SIZE (32)
+
 /* tokens */
 
 /**/
@@ -721,7 +723,7 @@ gettok(void)
     /* word includes the last character read and possibly \ before ! */
     if (dbparens) {
 	len = 0;
-	bptr = tokstr = (char *) hcalloc(bsiz = 32);
+	bptr = tokstr = (char *) hcalloc(bsiz = LEX_HEAP_SIZE);
 	hungetc(c);
 	cmdpush(CS_MATH);
 	c = dquote_parse(infor ? ';' : ')', 0);
@@ -775,7 +777,7 @@ gettok(void)
 
 	if (lexflags & LEXFLAGS_COMMENTS_KEEP) {
 	    len = 0;
-	    bptr = tokstr = (char *)hcalloc(bsiz = 32);
+	    bptr = tokstr = (char *)hcalloc(bsiz = LEX_HEAP_SIZE);
 	    add(c);
 	}
 	hwend();
@@ -877,7 +879,7 @@ gettok(void)
 	    }
 	    if (incmdpos || (isset(SHGLOB) && !isset(KSHGLOB))) {
 		len = 0;
-		bptr = tokstr = (char *) hcalloc(bsiz = 32);
+		bptr = tokstr = (char *) hcalloc(bsiz = LEX_HEAP_SIZE);
 		switch (cmd_or_math(CS_MATH)) {
 		case 1:
 		    return DINPAR;
@@ -1027,7 +1029,7 @@ gettokstr(int c, int sub)
     peek = STRING;
     if (!sub) {
 	len = 0;
-	bptr = tokstr = (char *) hcalloc(bsiz = 32);
+	bptr = tokstr = (char *) hcalloc(bsiz = LEX_HEAP_SIZE);
     }
     for (;;) {
 	int act;
diff --git a/Src/mem.c b/Src/mem.c
index f198177..a8f0c37 100644
--- a/Src/mem.c
+++ b/Src/mem.c
@@ -226,6 +226,9 @@ old_heaps(Heap old)
 #else
 	zfree(h, HEAPSIZE);
 #endif
+#ifdef ZSH_VALGRIND
+	VALGRIND_DESTROY_MEMPOOL((char *)h);
+#endif
     }
     heaps = old;
 #ifdef ZSH_HEAP_DEBUG
@@ -347,6 +350,10 @@ freeheap(void)
 	hn = h->next;
 	if (h->sp) {
 #ifdef ZSH_MEM_DEBUG
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MAKE_MEM_UNDEFINED((char *)arena(h) + h->sp->used,
+					h->used - h->sp->used);
+#endif
 	    memset(arena(h) + h->sp->used, 0xff, h->used - h->sp->used);
 #endif
 	    h->used = h->sp->used;
@@ -369,12 +376,18 @@ freeheap(void)
 		h->heap_id = new_id;
 	    }
 #endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MEMPOOL_TRIM((char *)h, (char *)arena(h), h->used);
+#endif
 	} else {
 #ifdef USE_MMAP
 	    munmap((void *) h, h->size);
 #else
 	    zfree(h, HEAPSIZE);
 #endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_DESTROY_MEMPOOL((char *)h);
+#endif
 	}
     }
     if (hl)
@@ -406,6 +419,10 @@ popheap(void)
 	if ((hs = h->sp)) {
 	    h->sp = hs->next;
 #ifdef ZSH_MEM_DEBUG
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MAKE_MEM_UNDEFINED((char *)arena(h) + hs->used,
+					h->used - hs->used);
+#endif
 	    memset(arena(h) + hs->used, 0xff, h->used - hs->used);
 #endif
 	    h->used = hs->used;
@@ -417,6 +434,9 @@ popheap(void)
 	    }
 	    h->heap_id = hs->heap_id;
 #endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MEMPOOL_TRIM((char *)h, (char *)arena(h), h->used);
+#endif
 	    if (!fheap && h->used < ARENA_SIZEOF(h))
 		fheap = h;
 	    zfree(hs, sizeof(*hs));
@@ -428,6 +448,9 @@ popheap(void)
 #else
 	    zfree(h, HEAPSIZE);
 #endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_DESTROY_MEMPOOL((char *)h);
+#endif
 	}
     }
     if (hl)
@@ -499,6 +522,12 @@ zhalloc(size_t size)
 {
     Heap h;
     size_t n;
+#ifdef ZSH_VALGRIND
+    size_t req_size = size;
+
+    if (size == 0)
+	return NULL;
+#endif
 
     size = (size + H_ISIZE - 1) & ~(H_ISIZE - 1);
 
@@ -526,6 +555,9 @@ zhalloc(size_t size)
 			HEAPID_FMT ".\n", h->heap_id);
 	    }
 #endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MEMPOOL_ALLOC((char *)h, (char *)ret, req_size);
+#endif
 	    return ret;
 	}
     }
@@ -564,6 +596,12 @@ zhalloc(size_t size)
 		    h->heap_id);
 	}
 #endif
+#ifdef ZSH_VALGRIND
+	VALGRIND_CREATE_MEMPOOL((char *)h, 0, 0);
+	VALGRIND_MAKE_MEM_NOACCESS((char *)arena(h),
+				   n - ((char *)arena(h)-(char *)h));
+	VALGRIND_MEMPOOL_ALLOC((char *)h, (char *)arena(h), req_size);
+#endif
 
 	if (hp)
 	    hp->next = h;
@@ -589,13 +627,21 @@ hrealloc(char *p, size_t old, size_t new)
 {
     Heap h, ph;
 
+#ifdef ZSH_VALGRIND
+    size_t new_req = new;
+#endif
+
     old = (old + H_ISIZE - 1) & ~(H_ISIZE - 1);
     new = (new + H_ISIZE - 1) & ~(H_ISIZE - 1);
 
     if (old == new)
 	return p;
     if (!old && !p)
+#ifdef ZSH_VALGRIND
+	return zhalloc(new_req);
+#else
 	return zhalloc(new);
+#endif
 
     /* find the heap with p */
 
@@ -618,14 +664,33 @@ hrealloc(char *p, size_t old, size_t new)
      */
     if (p + old < arena(h) + h->used) {
 	if (new > old) {
+#ifdef ZSH_VALGRIND
+	    char *ptr = (char *) zhalloc(new_req);
+#else
 	    char *ptr = (char *) zhalloc(new);
+#endif
 	    memcpy(ptr, p, old);
 #ifdef ZSH_MEM_DEBUG
 	    memset(p, 0xff, old);
 #endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MEMPOOL_FREE((char *)h, (char *)p);
+	    /*
+	     * zhalloc() marked h,ptr,new as an allocation so we don't
+	     * need to do that here.
+	     */
+#endif
 	    unqueue_signals();
 	    return ptr;
 	} else {
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MEMPOOL_FREE((char *)h, (char *)p);
+	    if (p) {
+		VALGRIND_MEMPOOL_ALLOC((char *)h, (char *)p,
+				       new_req);
+		VALGRIND_MAKE_MEM_DEFINED((char *)h, (char *)p);
+	    }
+#endif
 	    unqueue_signals();
 	    return new ? p : NULL;
 	}
@@ -660,10 +725,14 @@ hrealloc(char *p, size_t old, size_t new)
 #else
 	    zfree(h, HEAPSIZE);
 #endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_DESTROY_MEMPOOL((char *)h);
+#endif
 	    unqueue_signals();
 	    return NULL;
 	}
 	if (new > ARENA_SIZEOF(h)) {
+	    Heap hnew;
 	    /*
 	     * Not enough memory in this heap.  Allocate a new
 	     * one of sufficient size.
@@ -685,17 +754,23 @@ hrealloc(char *p, size_t old, size_t new)
 		 * a mmap'd segment be extended, so simply allocate
 		 * a new one and copy.
 		 */
-		Heap hnew;
-
 		hnew = mmap_heap_alloc(&n);
 		/* Copy the entire heap, header (with next pointer) included */
 		memcpy(hnew, h, h->size);
 		munmap((void *)h, h->size);
-		h = hnew;
 	    }
 #else
-	    h = (Heap) realloc(h, n);
+	    hnew = (Heap) realloc(h, n);
+#endif
+#ifdef ZSH_VALGRIND
+	    VALGRIND_MEMPOOL_FREE((char *)h, p);
+	    VALGRIND_DESTROY_MEMPOOL((char *)h);
+	    VALGRIND_CREATE_MEMPOOL((char *)hnew, 0, 0);
+	    VALGRIND_MEMPOOL_ALLOC((char *)hnew, (char *)arena(hnew),
+				   new_req);
+	    VALGRIND_MAKE_MEM_DEFINED((char *)hnew, (char *)arena(hnew));
 #endif
+	    h = hnew;
 
 	    h->size = n;
 	    if (ph)
@@ -703,6 +778,13 @@ hrealloc(char *p, size_t old, size_t new)
 	    else
 		heaps = h;
 	}
+#ifdef ZSH_VALGRIND
+	else {
+	    VALGRIND_MEMPOOL_FREE((char *)h, (char *)p);
+	    VALGRIND_MEMPOOL_ALLOC((char *)h, (char *)p, new_req);
+	    VALGRIND_MAKE_MEM_DEFINED((char *)h, (char *)p);
+	}
+#endif
 	h->used = new;
 #ifdef ZSH_HEAP_DEBUG
 	h->heap_id = heap_id;
@@ -716,6 +798,11 @@ hrealloc(char *p, size_t old, size_t new)
     if (h->used + (new - old) <= ARENA_SIZEOF(h)) {
 	h->used += new - old;
 	unqueue_signals();
+#ifdef ZSH_VALGRIND
+	VALGRIND_MEMPOOL_FREE((char *)h, (char *)p);
+	VALGRIND_MEMPOOL_ALLOC((char *)h, (char *)p, new_req);
+	VALGRIND_MAKE_MEM_DEFINED((char *)h, (char *)p);
+#endif
 	return p;
     } else {
 	char *t = zhalloc(new);
@@ -724,6 +811,10 @@ hrealloc(char *p, size_t old, size_t new)
 #ifdef ZSH_MEM_DEBUG
 	memset(p, 0xff, old);
 #endif
+#ifdef ZSH_VALGRIND
+	VALGRIND_MEMPOOL_FREE((char *)h, (char *)p);
+	/* t already marked as allocated by zhalloc() */
+#endif
 	unqueue_signals();
 	return t;
     }
diff --git a/Src/zsh_system.h b/Src/zsh_system.h
index 6887a13..601de69 100644
--- a/Src/zsh_system.h
+++ b/Src/zsh_system.h
@@ -877,3 +877,8 @@ extern short ospeed;
 #  endif
 # endif
 #endif
+
+#ifdef ZSH_VALGRIND
+# include "valgrind/valgrind.h"
+# include "valgrind/memcheck.h"
+#endif
diff --git a/configure.ac b/configure.ac
index a2a6b9e..0f87a6c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -117,6 +117,17 @@ AC_HELP_STRING([--enable-zsh-heap-debug],
   AC_DEFINE(ZSH_HEAP_DEBUG)
 fi])
 
+dnl Do you want to allow Valgrind to debug heap allocation?
+ifdef([zsh-valgrind],[undefine([zsh-valgrind])])dnl
+AH_TEMPLATE([ZSH_VALGRIND],
+[Define to 1 if you want to add code for valgrind to debug heap memory.])
+AC_ARG_ENABLE(zsh-valgrind,
+AC_HELP_STRING([--enable-zsh-valgrind],
+[turn on support for valgrind debugging of heap memory]),
+[if test x$enableval = xyes;  then
+  AC_DEFINE(ZSH_VALGRIND)
+fi])
+
 dnl Do you want debugging information on internal hash tables.
 dnl This turns on the `hashinfo' builtin command.
 ifdef([zsh-hash-debug],[undefine([zsh-hash-debug])])dnl

-- 
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/