From: hno <>
Date: Sun, 3 Sep 2006 10:09:35 +0000 (+0000)
Subject: Valgrind support. Mainly rearranging of cbdata to make malloc debugging
X-Git-Tag: SQUID_3_0_PRE5~130
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b4bab919c085820924b560e2b9e1346decff61e5;p=thirdparty%2Fsquid.git

Valgrind support. Mainly rearranging of cbdata to make malloc debugging
easier.

Also adds a new environment variable MEMPOOLS replacing the -mc command line option
for disabling the use of memory pools. A lot of the pools is created
automatically before main() is started so command line options was
a bit too late.
---

diff --git a/configure.in b/configure.in
index 0c0df110ee..ba94fd4a75 100644
--- a/configure.in
+++ b/configure.in
@@ -1,7 +1,7 @@
 
 dnl  Configuration input file for Squid
 dnl
-dnl  $Id: configure.in,v 1.432 2006/09/02 16:18:22 serassio Exp $
+dnl  $Id: configure.in,v 1.433 2006/09/03 04:09:35 hno Exp $
 dnl
 dnl
 dnl
@@ -11,7 +11,7 @@ AM_CONFIG_HEADER(include/autoconf.h)
 AC_CONFIG_AUX_DIR(cfgaux)
 AC_CONFIG_SRCDIR([src/main.cc])
 AM_INIT_AUTOMAKE([tar-ustar])
-AC_REVISION($Revision: 1.432 $)dnl
+AC_REVISION($Revision: 1.433 $)dnl
 AC_PREFIX_DEFAULT(/usr/local/squid)
 AM_MAINTAINER_MODE
 
@@ -1643,6 +1643,26 @@ if test -n "$EXTERNAL_ACL_HELPERS"; then
 fi
 AC_SUBST(EXTERNAL_ACL_HELPERS)
 
+AC_ARG_WITH(valgrind,
+[  --with-valgrind       Include debug instrumentation for use with valgrind],
+[ case $withval in
+  yes)
+	valgrind=1
+	;;
+  no)
+	valgrind=
+	;;
+  *)
+	CPPFLAGS="$CPPFLAGS -I${enableval}/include"
+	valgrind=1
+	;;
+  esac
+  if test $valgrind; then
+	AC_DEFINE(WITH_VALGRIND, 1, [Valgrind memory debugger support])
+	echo "Valgrind debug support enabled"
+  fi
+])
+
 dnl Disable "memPools" code
 AC_DEFINE(DISABLE_POOLS, 0, [Define if you have problems with memPools and want to disable Pools.])
 AC_ARG_ENABLE(mempools,
diff --git a/include/config.h b/include/config.h
index efd0dc0635..fa59c77705 100644
--- a/include/config.h
+++ b/include/config.h
@@ -1,5 +1,5 @@
 /*
- * $Id: config.h,v 1.22 2006/09/02 13:30:54 serassio Exp $
+ * $Id: config.h,v 1.23 2006/09/03 04:09:35 hno Exp $
  *
  * AUTHOR: Duane Wessels
  *
@@ -421,4 +421,31 @@ typedef union {
 #define PRINTF_FORMAT_ARG3
 #endif
 
+/*
+ * Determine if this is a leak check build or standard
+ */
+#if PURIFY
+#define LEAK_CHECK_MODE 1
+#elif WITH_VALGRIND
+#define LEAK_CHECK_MODE 1
+#elif XMALLOC_TRACE
+#define LEAK_CHECK_MODE 1
+#endif
+
+/*
+ * valgrind debug support
+ */
+#if WITH_VALGRIND
+#include <valgrind/memcheck.h>
+#else
+#define VALGRIND_MAKE_NOACCESS(a,b) (0)
+#define VALGRIND_MAKE_WRITABLE(a,b) (0)
+#define VALGRIND_MAKE_READABLE(a,b) (0)
+#define VALGRIND_CHECK_WRITABLE(a,b) (0)
+#define VALGRIND_CHECK_READABLE(a,b) (0)
+#define VALGRIND_MALLOCLIKE_BLOCK(a,b,c,d)
+#define VALGRIND_FREELIKE_BLOCK(a,b)
+#define RUNNING_ON_VALGRIND 0
+#endif /* WITH_VALGRIND */
+
 #endif /* SQUID_CONFIG_H */
diff --git a/lib/MemPool.cc b/lib/MemPool.cc
index 79f8df6e13..85d51e2d28 100644
--- a/lib/MemPool.cc
+++ b/lib/MemPool.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: MemPool.cc,v 1.3 2006/06/18 08:56:32 serassio Exp $
+ * $Id: MemPool.cc,v 1.4 2006/09/03 04:09:35 hno Exp $
  *
  * DEBUG: section 63    Low Level Memory Pool Management
  * AUTHOR: Alex Rousskov, Andres Kroonmaa, Robert Collins
@@ -208,7 +208,9 @@ MemChunk::MemChunk(MemPool *aPool)
 
     for (int i = 1; i < pool->chunk_capacity; i++) {
 	*Free = (void *) ((char *) Free + pool->obj_size);
-	Free = (void **)*Free;
+	void **nextFree = (void **)*Free;
+	(void) VALGRIND_MAKE_NOACCESS(Free, pool->obj_size);
+	Free = nextFree;
     }
     nextFreeChunk = pool->nextFreeChunk;
     pool->nextFreeChunk = this;
@@ -273,6 +275,7 @@ MemPool::push(void *obj)
     Free = (void **)obj;
     *Free = freeCache;
     freeCache = obj;
+    (void) VALGRIND_MAKE_NOACCESS(obj, obj_size);
 }
 
 /*
@@ -289,6 +292,7 @@ MemPool::get()
     /* first, try cache */
     if (freeCache) {
 	Free = (void **)freeCache;
+	(void) VALGRIND_MAKE_READABLE(Free, obj_size);
 	freeCache = *Free;
 	*Free = NULL;
 	return Free;
@@ -311,6 +315,7 @@ MemPool::get()
 	/* last free in this chunk, so remove us from perchunk freelist chain */
 	nextFreeChunk = chunk->nextFreeChunk;
     }
+    (void) VALGRIND_MAKE_READABLE(Free, obj_size);
     return Free;
 }
 
@@ -351,8 +356,11 @@ MemPool::createChunk()
  * MemPools::GetInstance().setDefaultPoolChunking() can be called.
  */
 MemPools::MemPools() : pools(NULL), mem_idle_limit(2 * MB),
-  poolCount (0), defaultIsChunked (!DISABLE_POOLS)
+  poolCount (0), defaultIsChunked (!DISABLE_POOLS && !RUNNING_ON_VALGRIND)
 {
+    char *cfg = getenv("MEMPOOLS");
+    if (cfg)
+	defaultIsChunked = atoi(cfg);
 #if HAVE_MALLOPT && M_MMAP_MAX
     mallopt(M_MMAP_MAX, MEM_MAX_MMAP_CHUNKS);
 #endif
@@ -535,6 +543,7 @@ void
 MemImplementingAllocator::free(void *obj)
 {
     assert(obj != NULL);
+    (void) VALGRIND_CHECK_WRITABLE(obj, obj_size);
     deallocate(obj);
     ++free_calls;
 }
@@ -573,8 +582,10 @@ MemPool::convertFreeCacheToChunkFreeCache()
 	assert(splayLastResult == 0);
 	assert(chunk->inuse_count > 0);
 	chunk->inuse_count--;
+	(void) VALGRIND_MAKE_READABLE(Free, sizeof(void *));
 	freeCache = *(void **)Free;	/* remove from global cache */
 	*(void **)Free = chunk->freeList;	/* stuff into chunks freelist */
+	(void) VALGRIND_MAKE_NOACCESS(Free, sizeof(void *));
 	chunk->freeList = Free;
 	chunk->lastref = squid_curtime;
     }
diff --git a/src/cbdata.cc b/src/cbdata.cc
index ea9ddd70de..8d45e520fc 100644
--- a/src/cbdata.cc
+++ b/src/cbdata.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: cbdata.cc,v 1.72 2006/08/21 00:50:41 robertc Exp $
+ * $Id: cbdata.cc,v 1.73 2006/09/03 04:09:36 hno Exp $
  *
  * DEBUG: section 45    Callback Data Registry
  * ORIGINAL AUTHOR: Duane Wessels
@@ -53,6 +53,10 @@
 #endif
 #include "Generic.h"
 
+#if WITH_VALGRIND
+#define HASHED_CBDATA 1
+#endif
+
 static int cbdataCount = 0;
 #if CBDATA_DEBUG
 dlink_list cbdataEntries;
@@ -77,15 +81,24 @@ public:
 
 class cbdata
 {
-
+    /* TODO: examine making cbdata templated on this - so we get type
+     * safe access to data - RBC 20030902 */
 public:
+#if HASHED_CBDATA
+    hash_link hash;	// Must be first
+#endif
+
 #if CBDATA_DEBUG
 
     void dump(StoreEntry *)const;
 #endif
 
+#if !HASHED_CBDATA
     void *operator new(size_t size, void *where);
     void operator delete(void *where, void *where2);
+#else
+    MEMPROXY_CLASS(cndata);
+#endif
 
     ~cbdata();
     int valid;
@@ -109,20 +122,21 @@ public:
 
     /* cookie used while debugging */
     long cookie;
-    /* MUST be the last per-instance member */
-    /* TODO: examine making cbdata templated on this - so we get type
-     * safe access to data - RBC 20030902 */
-    void *data;
-void check(int line) const {assert(cookie == ((long)this ^ Cookie));}
+    void check(int line) const {assert(cookie == ((long)this ^ Cookie));}
+    static const long Cookie;
 
+#if !HASHED_CBDATA
     size_t dataSize() const { return sizeof(data);}
-
-    static const long Cookie;
     static long MakeOffset();
     static const long Offset;
+    /* MUST be the last per-instance member */
+    void *data;
+#endif
+
 };
 
 const long cbdata::Cookie((long)0xDEADBEEF);
+#if !HASHED_CBDATA
 const long cbdata::Offset(MakeOffset());
 
 void *
@@ -148,6 +162,9 @@ cbdata::MakeOffset()
     void **dataOffset = &zero->data;
     return (long)dataOffset;
 }
+#else
+MEMPROXY_CLASS_INLINE(cbdata)
+#endif
 
 static OBJH cbdataDump;
 #ifdef CBDATA_DEBUG
@@ -156,13 +173,30 @@ static OBJH cbdataDumpHistory;
 
 struct CBDataIndex
 {
-    MemAllocatorProxy *pool;
+    MemAllocator *pool;
     FREE *free_func;
 }
 
 *cbdata_index = NULL;
 int cbdata_types = 0;
 
+#if HASHED_CBDATA
+static hash_table *cbdata_htable = NULL;
+
+static int
+cbdata_cmp(const void *p1, const void *p2)
+{
+    return (char *) p1 - (char *) p2;
+}
+
+static unsigned int
+cbdata_hash(const void *p, unsigned int mod)
+{
+    return ((unsigned long) p >> 8) % mod;
+}
+#endif
+
+
 cbdata::~cbdata()
 {
 #if CBDATA_DEBUG
@@ -175,8 +209,14 @@ cbdata::~cbdata()
 
     FREE *free_func = cbdata_index[type].free_func;
 
+#if HASHED_CBDATA
+    void *p = hash.key;
+#else
+    void *p = &data;
+#endif
+
     if (free_func)
-        free_func(&data);
+        free_func(p);
 }
 
 static void
@@ -193,11 +233,19 @@ cbdataInternalInitType(cbdata_type type, const char *name, int size, FREE * free
 
     snprintf(label, strlen(name) + 20, "cbdata %s (%d)", name, (int) type);
 
+#if !HASHED_CBDATA
     assert((size_t)cbdata::Offset == (sizeof(cbdata) - ((cbdata *)NULL)->dataSize()));
+    size += cbdata::Offset;
+#endif
 
-    cbdata_index[type].pool = new MemAllocatorProxy(label, size + cbdata::Offset);
+    cbdata_index[type].pool = MemPools::GetInstance().create(label, size);
 
     cbdata_index[type].free_func = free_func;
+
+#if HASHED_CBDATA
+    if (!cbdata_htable)
+	cbdata_htable = hash_create(cbdata_cmp, 1 << 12, cbdata_hash);
+#endif
 }
 
 cbdata_type
@@ -234,29 +282,38 @@ cbdataInternalAllocDbg(cbdata_type type, const char *file, int line)
 cbdataInternalAlloc(cbdata_type type)
 #endif
 {
-    cbdata *p;
+    cbdata *c;
+    void *p;
     assert(type > 0 && type <= cbdata_types);
     /* placement new: the pool alloc gives us cbdata + user type memory space
      * and we init it with cbdata at the start of it
      */
-    p = new (cbdata_index[type].pool->alloc()) cbdata;
+#if HASHED_CBDATA
+    c = new cbdata;
+    p = cbdata_index[type].pool->alloc();
+    c->hash.key = p;
+    hash_join(cbdata_htable, &c->hash);
+#else
+    c = new (cbdata_index[type].pool->alloc()) cbdata;
+    p = (void *)&c->data;
+#endif
 
-    p->type = type;
-    p->valid = 1;
-    p->locks = 0;
-    p->cookie = (long) p ^ cbdata::Cookie;
+    c->type = type;
+    c->valid = 1;
+    c->locks = 0;
+    c->cookie = (long) c ^ cbdata::Cookie;
     cbdataCount++;
 #if CBDATA_DEBUG
 
-    p->file = file;
-    p->line = line;
-    p->calls = Stack<CBDataCall *> ();
-    p->addHistory("Alloc", file, line);
-    dlinkAdd(p, &p->link, &cbdataEntries);
-    debug(45, 3) ("cbdataAlloc: %p %s:%d\n", &p->data, file, line);
+    c->file = file;
+    c->line = line;
+    c->calls = Stack<CBDataCall *> ();
+    c->addHistory("Alloc", file, line);
+    dlinkAdd(c, &c->link, &cbdataEntries);
+    debug(45, 3) ("cbdataAlloc: %p %s:%d\n", p, file, line);
 #endif
 
-    return (void *) &p->data;
+    return p;
 }
 
 void *
@@ -267,7 +324,11 @@ cbdataInternalFree(void *p)
 #endif
 {
     cbdata *c;
+#if HASHED_CBDATA
+    c = (cbdata *) hash_lookup(cbdata_htable, p);
+#else
     c = (cbdata *) (((char *) p) - cbdata::Offset);
+#endif
 #if CBDATA_DEBUG
 
     debug(45, 3) ("cbdataFree: %p %s:%d\n", p, file, line);
@@ -297,9 +358,6 @@ cbdataInternalFree(void *p)
     dlinkDelete(&c->link, &cbdataEntries);
 #endif
 
-    cbdata_type theType = c->type;
-    c->cbdata::~cbdata();
-
     /* This is ugly. But: operator delete doesn't get
      * the type parameter, so we can't use that 
      * to free the memory.
@@ -310,8 +368,15 @@ cbdataInternalFree(void *p)
      * we could use the normal delete operator
      * and it would Just Work. RBC 20030902
      */
-    assert ( cbdata_index[theType].pool );
+    cbdata_type theType = c->type;
+#if HASHED_CBDATA
+    hash_remove_link(cbdata_htable, &c->hash);
+    delete c;
+    cbdata_index[theType].pool->free((void *)p);
+#else
+    c->cbdata::~cbdata();
     cbdata_index[theType].pool->free(c);
+#endif
     return NULL;
 }
 
@@ -327,7 +392,11 @@ cbdataInternalLock(const void *p)
     if (p == NULL)
         return;
 
+#if HASHED_CBDATA
+    c = (cbdata *) hash_lookup(cbdata_htable, p);
+#else
     c = (cbdata *) (((char *) p) - cbdata::Offset);
+#endif
 
 #if CBDATA_DEBUG
 
@@ -360,7 +429,11 @@ cbdataInternalUnlock(const void *p)
     if (p == NULL)
         return;
 
+#if HASHED_CBDATA
+    c = (cbdata *) hash_lookup(cbdata_htable, p);
+#else
     c = (cbdata *) (((char *) p) - cbdata::Offset);
+#endif
 
 #if CBDATA_DEBUG
 
@@ -395,10 +468,6 @@ cbdataInternalUnlock(const void *p)
 
 #endif
 
-    cbdata_type theType = c->type;
-
-    c->cbdata::~cbdata();
-
     /* This is ugly. But: operator delete doesn't get
      * the type parameter, so we can't use that 
      * to free the memory.
@@ -409,7 +478,15 @@ cbdataInternalUnlock(const void *p)
      * we could use the normal delete operator
      * and it would Just Work. RBC 20030902
      */
+    cbdata_type theType = c->type;
+#if HASHED_CBDATA
+    hash_remove_link(cbdata_htable, &c->hash);
+    delete c;
+    cbdata_index[theType].pool->free((void *)p);
+#else
+    c->cbdata::~cbdata();
     cbdata_index[theType].pool->free(c);
+#endif
 }
 
 int
@@ -422,7 +499,11 @@ cbdataReferenceValid(const void *p)
 
     debug(45, 9) ("cbdataReferenceValid: %p\n", p);
 
+#if HASHED_CBDATA
+    c = (cbdata *) hash_lookup(cbdata_htable, p);
+#else
     c = (cbdata *) (((char *) p) - cbdata::Offset);
+#endif
 
     c->check(__LINE__);
 
@@ -462,8 +543,13 @@ cbdataInternalReferenceDoneValid(void **pp, void **tp)
 void
 cbdata::dump(StoreEntry *sentry) const
 {
+#if HASHED_CBDATA
+    void *p = hash.key;
+#else
+    void *p = &data;
+#endif
     storeAppendPrintf(sentry, "%c%p\t%d\t%d\t%20s:%-5d\n", valid ? ' ' :
-                      '!', &data, type, locks, file, line);
+                      '!', p, type, locks, file, line);
 }
 
 struct CBDataDumper : public unary_function<cbdata, void>
@@ -493,10 +579,14 @@ cbdataDump(StoreEntry * sentry)
     storeAppendPrintf(sentry, "types\tsize\tallocated\ttotal\n");
 
     for (int i = 1; i < cbdata_types; i++) {
-        MemAllocatorProxy *pool = cbdata_index[i].pool;
+        MemAllocator *pool = cbdata_index[i].pool;
 
         if (pool) {
+#if HASHED_CBDATA
+            int obj_size = pool->objectSize();
+#else
             int obj_size = pool->objectSize() - cbdata::Offset;
+#endif
             storeAppendPrintf(sentry, "%s\t%d\t%ld\t%ld\n", pool->objectType() + 7, obj_size, (long int)pool->getMeter().inuse.level, (long int)obj_size * pool->getMeter().inuse.level);
         }
     }
diff --git a/src/comm_epoll.cc b/src/comm_epoll.cc
index 8453fdb75f..26c0fcf296 100644
--- a/src/comm_epoll.cc
+++ b/src/comm_epoll.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: comm_epoll.cc,v 1.12 2006/09/02 10:39:53 adrian Exp $
+ * $Id: comm_epoll.cc,v 1.13 2006/09/03 04:09:36 hno Exp $
  *
  * DEBUG: section 5    Socket functions
  *
@@ -136,6 +136,10 @@ commSetSelect(int fd, unsigned int type, PF * handler,
     debug(5, DEBUG_EPOLL ? 0 : 8) ("commSetSelect(FD %d,type=%u,handler=%p,client_data=%p,timeout=%ld)\n",
                                    fd,type,handler,client_data,timeout);
 
+    if (RUNNING_ON_VALGRIND) {
+	/* Keep valgrind happy.. complains about uninitialized bytes otherwise */
+	memset(&ev, 0, sizeof(ev));
+    }
     ev.events = 0;
     ev.data.fd = fd;
 
diff --git a/src/main.cc b/src/main.cc
index b1b7414110..9629de4e8a 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: main.cc,v 1.433 2006/09/02 08:43:37 serassio Exp $
+ * $Id: main.cc,v 1.434 2006/09/03 04:09:36 hno Exp $
  *
  * DEBUG: section 1     Startup and Main Loop
  * AUTHOR: Harvest Derived
@@ -408,9 +408,6 @@ mainParseOptions(int argc, char *argv[])
 
         case 'm':
             if (optarg) {
-                if (*optarg == 'c') {
-                    MemPools::GetInstance().setDefaultPoolChunking(0);
-                } else {
 #if MALLOC_DBG
                     malloc_debug_level = atoi(optarg);
 #else
@@ -418,8 +415,6 @@ mainParseOptions(int argc, char *argv[])
                     fatal("Need to add -DMALLOC_DBG when compiling to use -mX option");
 #endif
 
-                }
-
             } else {
 #if XMALLOC_TRACE
                 xmalloc_trace = !xmalloc_trace;
@@ -1690,7 +1685,7 @@ SquidShutdown()
     Store::Root().sync();		/* Flush log close */
     StoreFileSystem::FreeAllFs();
     DiskIOModule::FreeAllModules();
-#if PURIFY || XMALLOC_TRACE
+#if LEAK_CHECK_MODE
 
     configFreeMemory();
     storeFreeMemory();
diff --git a/src/mem.cc b/src/mem.cc
index 3446dee3d0..0fb9a568f0 100644
--- a/src/mem.cc
+++ b/src/mem.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: mem.cc,v 1.99 2006/08/07 02:28:22 robertc Exp $
+ * $Id: mem.cc,v 1.100 2006/09/03 04:09:36 hno Exp $
  *
  * DEBUG: section 13    High Level Memory Pool Management
  * AUTHOR: Harvest Derived
@@ -138,6 +138,21 @@ Mem::Stats(StoreEntry * sentry)
     Report(stream);
     memStringStats(stream);
     memBufStats(stream);
+#if WITH_VALGRIND
+    if (RUNNING_ON_VALGRIND) {
+	long int leaked = 0, dubious = 0, reachable = 0, suppressed = 0;
+	stream << "Valgrind Report:\n";
+	stream << "Type\tAmount\n";
+	debug(13, 1) ("Asking valgrind for memleaks\n");
+	VALGRIND_DO_LEAK_CHECK;
+	debug(13, 1) ("Getting valgrind statistics\n");
+	VALGRIND_COUNT_LEAKS(leaked, dubious, reachable, suppressed);
+	stream << "Leaked\t" << leaked << "\n";
+	stream << "Dubious\t" << dubious << "\n";
+	stream << "Reachable\t" << reachable << "\n";
+	stream << "Suppressed\t" << suppressed << "\n";
+    }
+#endif
     stream.flush();
 }
 
diff --git a/src/squid.h b/src/squid.h
index b40541f1e4..ca5c1a74ed 100644
--- a/src/squid.h
+++ b/src/squid.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: squid.h,v 1.258 2006/09/02 15:37:29 serassio Exp $
+ * $Id: squid.h,v 1.259 2006/09/03 04:09:36 hno Exp $
  *
  * AUTHOR: Duane Wessels
  *
@@ -308,7 +308,7 @@ SQUIDCEXTERN size_t getpagesize(void);
 #define SA_RESETHAND SA_ONESHOT
 #endif
 
-#if PURIFY
+#if LEACK_CHECK_MODE
 #define LOCAL_ARRAY(type,name,size) \
         static type *local_##name=NULL; \
         type *name = local_##name ? local_##name : \