Everybody knows that valgrind is great.

Well, I was observing a problem in some MySQL code, it looked like we were writing over some memory that we weren’t meant to be (as the structure hadn’t been initialised yet). But, seeing as this was memory that had been allocated off a MEM_ROOT (one of our memory allocators), valgrind wasn’t gonig to spit out anything.

This is because this bit of memory had already been allocated and subsequently “freed”, but then reallocated. The “free”ing overwrites the memory with garbage (which is what the MEM_ROOT code does) so that you should see crashes (and a pattern) when you do something bad.

The traditional way to troubleshoot this in to modify your memory allocator so that it just calls malloc() and free() all the time (and valgrind will trap them). We have some code in there to do that too. However, this requires changing some ifdefs and probably not being as efficient.

Valgrind has some macros you can insert into your memory allocator code that tell valgrind that you have “freed” this memory (VALGRIND_MAKE_NOACCESS) or have allocated it (VALGRIND_MAKE_WRITABLE) or have written valid data to it (VALGRIND_MAKE_READABLE). These are semi-documented in the valgrind/valgrind.h file.

These are designed to only add a few CPU instructions to your code, so it should be possible to always have them in your build (you can disable them donig anything by building with -DNVALGRIND IIRC).

(I haven’t done any benchmarks on the code to see if there is any impact though).

Valgrind also has a great (largely undocumented) feature of being able to integrate with memory pools. Since our MEM_ROOT is largely just this, we can get some added benefits here too (one should be better valgrind warnings when we do some bad stuff).
It lets you associate memory with a memory pool, and then just say “this pool has been freed”. Saves you having to keep track of each pointer in the code to pass to “free”. It also can give you valgrind warnings when you try and allocate memory to something that hasn’t been initialised as a memory pool.

The most interesting thing of writing the patch was finding some false positive warnings. Namely, a trick used in a couple of places (i see 2) in the code is to create a temporary memory root on the stack, allocate a larger block of memory and then “swap” the memory roots to be based in this block of memory. I had to write a swap_root function to implement this as valgrind doesn’t export a “swap memory pool” function. It would be a useful addition, maybe I’ll go and suggest it to the developers.

Anyway, I got over that hurdle and now have this patch which seems to work pretty well. I still get a couple of (possible) false positives. We’ll see if this finds any neat bugs. Also, a good exercise would be to see how many extra instructions are really generated and if this has any affect on performance at all.

===== include/my_sys.h 1.196 vs edited =====
--- 1.196/include/my_sys.h 2006-05-22 20:04:34 +10:00
+++ edited/include/my_sys.h 2006-05-26 16:22:11 +10:00
@@ -804,6 +804,7 @@
extern void set_prealloc_root(MEM_ROOT *root, char *ptr);
extern void reset_root_defaults(MEM_ROOT *mem_root, uint block_size,
uint prealloc_size);
+extern void swap_root(MEM_ROOT* new_root, MEM_ROOT* old);
extern char *strdup_root(MEM_ROOT *root,const char *str);
extern char *strmake_root(MEM_ROOT *root,const char *str,uint len);
extern char *memdup_root(MEM_ROOT *root,const char *str,uint len);
===== mysys/my_alloc.c 1.33 vs edited =====
--- 1.33/mysys/my_alloc.c 2005-11-24 07:44:54 +11:00
+++ edited/mysys/my_alloc.c 2006-05-26 19:21:12 +10:00
@@ -22,6 +22,8 @@
#undef EXTRA_DEBUG
#define EXTRA_DEBUG
+#include "valgrind/valgrind.h"
+#include "valgrind/memcheck.h"

/*
Initialize memory root
@@ -66,9 +68,12 @@
mem_root->free->size= pre_alloc_size+ALIGN_SIZE(sizeof(USED_MEM));
mem_root->free->left= pre_alloc_size;
mem_root->free->next= 0;
+ VALGRIND_MAKE_NOACCESS(mem_root->free+ALIGN_SIZE(sizeof(USED_MEM)),
+ pre_alloc_size);
}
}
#endif
+ VALGRIND_CREATE_MEMPOOL(mem_root,0,0);
DBUG_VOID_RETURN;
}

@@ -217,6 +222,9 @@
mem_root->first_block_usage= 0;
}
DBUG_PRINT(“exit”,(“ptr: 0x%lx”, (ulong) point));
+// fprintf(stderr,”root: %lx point: %lx size:%lx\n”,mem_root,point,Size);
+ VALGRIND_MEMPOOL_ALLOC(mem_root,point,Size);
+ VALGRIND_MAKE_WRITABLE(point,Size);
DBUG_RETURN(point);
#endif
}
@@ -286,7 +294,8 @@
for (next= root->free; next; next= *(last= &next->next))
{
next->left= next->size – ALIGN_SIZE(sizeof(USED_MEM));
– TRASH_MEM(next);
+ VALGRIND_MAKE_NOACCESS(next+ALIGN_SIZE(sizeof(USED_MEM)),next->left);
+// TRASH_MEM(next);
}

/* Combine the free and the used list */
@@ -296,7 +305,8 @@
for (; next; next= next->next)
{
next->left= next->size – ALIGN_SIZE(sizeof(USED_MEM));
– TRASH_MEM(next);
+ VALGRIND_MAKE_NOACCESS(next+ALIGN_SIZE(sizeof(USED_MEM)),next->left);
+// TRASH_MEM(next);
}

/* Now everything is set; Indicate that nothing is used anymore */
@@ -357,12 +367,55 @@
{
root->free=root->pre_alloc;
root->free->left=root->pre_alloc->size-ALIGN_SIZE(sizeof(USED_MEM));
– TRASH_MEM(root->pre_alloc);
+ //TRASH_MEM(root->pre_alloc);
root->free->next=0;
}
root->block_num= 4;
root->first_block_usage= 0;
+ VALGRIND_DESTROY_MEMPOOL(root);
+ VALGRIND_CREATE_MEMPOOL(root,0,0);
+ VALGRIND_MAKE_READABLE(root,sizeof(MEM_ROOT));
+ if(root->pre_alloc)
+ {
+ VALGRIND_MAKE_READABLE(root->pre_alloc, ALIGN_SIZE(sizeof(USED_MEM)));
+ VALGRIND_MEMPOOL_ALLOC(root,root->pre_alloc,root->pre_alloc->size);
+ VALGRIND_MAKE_READABLE(root->pre_alloc, ALIGN_SIZE(sizeof(USED_MEM)));
+ }
DBUG_VOID_RETURN;
+}
+
+void swap_root(MEM_ROOT* new_root, MEM_ROOT* old)
+{
+ memcpy((char*) new_root, (char*) old, sizeof(MEM_ROOT));
+ VALGRIND_DESTROY_MEMPOOL(old);
+ VALGRIND_CREATE_MEMPOOL(new_root,0,0);
+
+ reg1 USED_MEM *next;
+
+ VALGRIND_MEMPOOL_ALLOC(new_root,new_root,sizeof(MEM_ROOT));
+ VALGRIND_MAKE_READABLE(new_root,sizeof(MEM_ROOT));
+
+ /* iterate through (partially) free blocks */
+ next= new_root->free;
+ do
+ {
+ if(!next)
+ break;
+ VALGRIND_MEMPOOL_ALLOC(new_root,next,next->size-next->left);
+ VALGRIND_MAKE_READABLE(next,next->size-next->left);
+ next= next->next;
+ } while(1);
+
+ /* now go through the used blocks and mark them free */
+ next= new_root->used;
+ do
+ {
+ if(!next)
+ break;
+ VALGRIND_MEMPOOL_ALLOC(new_root,next,next->size-next->left);
+ VALGRIND_MAKE_READABLE(next,next->size-next->left);
+ next= next->next;
+ } while(1);
}

/*
===== sql/table.cc 1.215 vs edited =====
— 1.215/sql/table.cc 2006-05-23 05:54:55 +10:00
+++ edited/sql/table.cc 2006-05-26 18:12:21 +10:00
@@ -150,7 +150,8 @@

#endif

– memcpy((char*) &share->mem_root, (char*) &mem_root, sizeof(mem_root));
+// memcpy((char*) &share->mem_root, (char*) &mem_root, sizeof(mem_root));
+ swap_root(&share->mem_root,&mem_root);
pthread_mutex_init(&share->mutex, MY_MUTEX_INIT_FAST);
pthread_cond_init(&share->cond, NULL);
}
@@ -252,7 +253,7 @@
hash_free(&share->name_hash);

/* We must copy mem_root from share because share is allocated through it */
– memcpy((char*) &mem_root, (char*) &share->mem_root, sizeof(mem_root));
+ swap_root(&mem_root,&share->mem_root);//memcpy((char*) &mem_root, (char*) &share->mem_root, sizeof(mem_root));
free_root(&mem_root, MYF(0)); // Free’s share
DBUG_VOID_RETURN;
}
===== storage/ndb/src/kernel/blocks/dbdict/Dbdict.cpp 1.87 vs edited =====
— 1.87/storage/ndb/src/kernel/blocks/dbdict/Dbdict.cpp 2006-04-25 22:02:07 +10:00
+++ edited/storage/ndb/src/kernel/blocks/dbdict/Dbdict.cpp 2006-05-26 12:15:43 +10:00
@@ -3148,9 +3148,23 @@

CreateTableRecordPtr createTabPtr;
ndbrequire(c_opCreateTable.find(createTabPtr, callbackData));
–
– //@todo check error
– ndbrequire(createTabPtr.p->m_errorCode == 0);
+
+ if(createTabPtr.p->m_errorCode != 0)
+ {
+ char buf[255];
+ TableRecordPtr tabPtr;
+ c_tableRecordPool.getPtr(tabPtr, createTabPtr.p->m_tablePtrI);
+
+ BaseString::snprintf(buf, sizeof(buf),
+ “Unable to restart, fail while creating table %d”
+ ” error: %d. Most likely change of configuration”,
+ tabPtr.p->tableId,
+ createTabPtr.p->m_errorCode);
+ progError(__LINE__,
+ NDBD_EXIT_INVALID_CONFIG,
+ buf);
+ ndbrequire(createTabPtr.p->m_errorCode == 0);
+ }

Callback callback;
callback.m_callbackData = callbackData;