|
From: Joe M. <jo...@no...> - 2006-01-25 17:41:39
|
I'm implemented in a custom memory manager that has semantics like
malloc, free and realloc. malloc and free are easy to integrate with
valgrind using VALGRIND_MALLOCLIKE_BLOCK and VALGRIND_FREELIKE_BLOCK,
but I'm having some problems with realloc.
There are three cases:
1. Can't realloc in place - need to alloc new memory, copy the old chunk
to the new, and free the old chunk.
- Easy: just call VALGRIND_MALLOCLIKE_BLOCK after creating the new
chunk, then valgrind will notices the memcpy and update its vbits
accordingly, then call VALGRIND_FREELIKE_BLOCK on the old chunk.
2. Able to grow allocation in place - need to mark new area addressable.
- It's tempting to just use VALGRIND_MAKE_WRITABLE to mark the new area
as allocated, but of course then valgrind doesn't have the right size
for the malloc'd chunk: attempts to write past the end of the new
memory get the wrong error message ("Address 0x401F200 is not stack'd,
malloc'd or (recently) free'd" instead of "Address 0x401F200 is 0
bytes after a block of size 128 alloc'd"), and calling
VALGRIND_FREELIKE_BLOCK later will only mark the chunk unaddressable
up to its original size.
3. Able to shrink allocation - need to mark part of old area
unaddressable.
- Again, could just use VALGRIND_MAKE_NOACCESS, but then it wouldn't
correctly track the size of the malloc'd chunk.
The only other option I can see for the two in-place cases is to call
VALGRIND_FREELIKE_BLOCK(ptr); VALGRIND_MALLOCLIKE_BLOCK(ptr, newsize) -
but that throws away all the vbits for the area. I could follow it with
VALGRIND_MAKE_READABLE(ptr, max(oldsize, newsize)), but of course
that assumes that the original chunk was entirely in use.
The obvious thing to do is use VALGRIND_GET_VBITS to save the vbits
before the reallocation, and follow it up with VALGRIND_PUT_VBITS
(truncating the array if necessary). Except I see those two calls were
deprecated last year, and aren't even included valgrind-3.1.0-Debian.
So what are my options? Right now, if compiled in debug mode, I'm
adding an extra memcpy for in-place reallocs (copy the memory to a
holding area, realloc in place, then copy it all back) solely so that
valgrind will notice the copy and keep the vbits up to date, but that's
a horrible solution. Is there anything else I could do without
modifying valgrind? (I took a look at MAC_(realloc), but of course it
uses a lot of internal machinery that's not exposed to users.)
The easiest modification to valgrind would be to reinstate GET_VBITS and
SET_VBITS, I guess. Or I could add VALGRIND_REALLOCLIKE_BLOCK(oldaddr,
newaddr, newsize, rz, is_zeroed), where is_zeroed applies only to any
newly allocated memory. Or VALGRIND_RESIZE_MALLOCLIKE_BLOCK(addr,
newsize, rz, is_zeroed), which would only change the size of a malloc'd
block (so only useful for the in-place case). GET_VBITS/SET_VBITS is
most general, but RESIZE_MALLOCLIKE_BLOCK directly attacks the
shortcoming, which is that once a block is MALLOC'd it can't be changed
without discarding the vbits. And REALLOCLIKE_BLOCK preserves the
symmetry of providing hooks for the 3 standard malloc operations.
Any preferences?
Joe
|
|
From: Nicholas N. <nj...@cs...> - 2006-01-25 22:15:24
|
On Wed, 25 Jan 2006, Joe Mason wrote: > The easiest modification to valgrind would be to reinstate GET_VBITS and > SET_VBITS, I guess. Or I could add VALGRIND_REALLOCLIKE_BLOCK(oldaddr, > newaddr, newsize, rz, is_zeroed), where is_zeroed applies only to any > newly allocated memory. Or VALGRIND_RESIZE_MALLOCLIKE_BLOCK(addr, > newsize, rz, is_zeroed), which would only change the size of a malloc'd > block (so only useful for the in-place case). GET_VBITS/SET_VBITS is > most general, but RESIZE_MALLOCLIKE_BLOCK directly attacks the > shortcoming, which is that once a block is MALLOC'd it can't be changed > without discarding the vbits. And REALLOCLIKE_BLOCK preserves the > symmetry of providing hooks for the 3 standard malloc operations. GET_VBITS and SET_VBITS have been reinstated in the COMPVBITS branch, and will be present in 3.2.0, which will probably be out in a couple of months. However, having REALLOCLIKE_BLOCK as well would be good, because it could apply to any tool, whereas GET_VBITS/SET_VBITS are Memcheck-specific. One problem is that Valgrind's client requests currently have a maximum of four arguments. Perhaps the 'rz' parameter could be not used, and we could assume the redzone is the same size as in the original block. Nick |
|
From: Joe M. <jo...@no...> - 2006-01-26 22:39:09
|
On Wed, Jan 25, 2006 at 04:15:18PM -0600, Nicholas Nethercote wrote: > However, having REALLOCLIKE_BLOCK as well would be good, because it could > apply to any tool, whereas GET_VBITS/SET_VBITS are Memcheck-specific. > One problem is that Valgrind's client requests currently have a maximum of > four arguments. Perhaps the 'rz' parameter could be not used, and we > could assume the redzone is the same size as in the original block. Unfortunately, it doesn't look like the redzone size is saved anywhere, which is why it has to be passed into FREELIKE_BLOCK (it would have to be added to MAC_Chunk, I think, which'd add overhead to every malloc'd chunk). I dropped the is_zeroed parameter instead, since you can always follow with MAKE_READABLE to show that the new memory is initialized. I've gone ahead and implemented both REALLOCLIKE_BLOCK and MEMPOOL_REALLOC (which also takes four parameters: pool, oldaddr, newaddr, newsize, since the pool uses a constant redzone size). I can probably send the patch tomorrow. BTW, I'd just like to say how much I appreciate Valgrind's code - I just started looking at the internals a few days ago, and it's one of the easiest to follow I've ever seen. Joe |
|
From: Ashley P. <as...@qu...> - 2006-01-27 15:41:52
Attachments:
valgrind-redzone.patch
|
On Thu, 2006-01-26 at 17:39 -0500, Joe Mason wrote: > On Wed, Jan 25, 2006 at 04:15:18PM -0600, Nicholas Nethercote wrote: > > However, having REALLOCLIKE_BLOCK as well would be good, because it could > > apply to any tool, whereas GET_VBITS/SET_VBITS are Memcheck-specific. > > One problem is that Valgrind's client requests currently have a maximum of > > four arguments. Perhaps the 'rz' parameter could be not used, and we > > could assume the redzone is the same size as in the original block. > > Unfortunately, it doesn't look like the redzone size is saved anywhere, > which is why it has to be passed into FREELIKE_BLOCK (it would have to > be added to MAC_Chunk, I think, which'd add overhead to every malloc'd > chunk). I dropped the is_zeroed parameter instead, since you can always > follow with MAKE_READABLE to show that the new memory is initialized. As it happens this is one of the patches I've got sitting in my tree. It should probably be applied because of this comment in memcheck/mac_shared.c however as you say it adds overhead to every MAC_Chunk which might be a bad thing. // Nb: this is not quite right! It assumes that the heap block has // a redzone of size MAC_MALLOC_REDZONE_SZB. That's true for malloc'd // blocks, but not necessarily true for custom-alloc'd blocks. So // in some cases this could result in an incorrect description (eg. // saying "12 bytes after block A" when really it's within block B. // Fixing would require adding redzone size to MAC_Chunks, though. // Fixed - 03/01/1006 as...@qu... Ashley, |
|
From: Julian S. <js...@ac...> - 2006-02-01 12:54:22
|
> I've gone ahead and implemented both REALLOCLIKE_BLOCK and > MEMPOOL_REALLOC (which also takes four parameters: pool, oldaddr, > newaddr, newsize, since the pool uses a constant redzone size). I can > probably send the patch tomorrow. In principle I'd like to merge this so you have a working system. One thing I'm not clear about is how it interacts with Ashley's valgrind-redzone.patch (msg dated Fri Jan 27 15:41:28 2006). In any case I'll change the max client request arg count to 5 to give us more wiggle room. > BTW, I'd just like to say how much I appreciate Valgrind's code - I just > started looking at the internals a few days ago, and it's one of the > easiest to follow I've ever seen. Thanks. Over the past year a lot of effort went into modularising and tidying up, given that some of what goes on is pretty hairy, so I'm pleased to see it helps. J |
|
From: Julian S. <js...@ac...> - 2006-02-01 12:47:36
|
> GET_VBITS and SET_VBITS have been reinstated in the COMPVBITS branch, > and will be present in 3.2.0, which will probably be out in a couple of > months. Yup. They are too useful not to have :-) > However, having REALLOCLIKE_BLOCK as well would be good, because it could > apply to any tool, whereas GET_VBITS/SET_VBITS are Memcheck-specific. I agree. > One problem is that Valgrind's client requests currently have a maximum of > four arguments. Perhaps the 'rz' parameter could be not used, and we > could assume the redzone is the same size as in the original block. I'm inclined to change the arg count to 5 at this point, so it will be possible to merge in REALLOCLIKE_BLOCK later without breaking backwards compatibility of the interface a second time, since the first change already caused problems for Ashley. J |
|
From: Julian S. <js...@ac...> - 2006-01-25 22:45:42
|
> One problem is that Valgrind's client requests currently have a maximum of > four arguments. Pretty harmless. All the args are passed in a block in memory to avoid getting tangled in an args-in-registers style swamp, so that block can easily be expanded by one word to accommodate an extra argument. J |
|
From: Nicholas N. <nj...@cs...> - 2006-01-26 02:52:21
|
On Wed, 25 Jan 2006, Julian Seward wrote: >> One problem is that Valgrind's client requests currently have a maximum of >> four arguments. > > Pretty harmless. All the args are passed in a block in memory to > avoid getting tangled in an args-in-registers style swamp, so > that block can easily be expanded by one word to accommodate an > extra argument. But then VALGRIND_MAGIC_SEQUENCE would have 8 args instead of 7, which would screw up things for those existing uses of it in people's code, no? Nick |
|
From: Julian S. <js...@ac...> - 2006-01-29 20:54:52
|
On Thursday 26 January 2006 02:52, Nicholas Nethercote wrote: > On Wed, 25 Jan 2006, Julian Seward wrote: > >> One problem is that Valgrind's client requests currently have a maximum > >> of four arguments. > > > > Pretty harmless. All the args are passed in a block in memory to > > avoid getting tangled in an args-in-registers style swamp, so > > that block can easily be expanded by one word to accommodate an > > extra argument. > > But then VALGRIND_MAGIC_SEQUENCE would have 8 args instead of 7, which > would screw up things for those existing uses of it in people's code, no? But VALGRIND_MAGIC_SEQUENCE is a low-level macro used as a building block for the higher client-request macros that users should be using. So I don't thing there should be a problem. In any case I sure hope not since I recently renamed it to VALGRIND_DO_CLIENT_REQUEST as that's a more sensible name for it. J |
|
From: Ashley P. <as...@qu...> - 2006-01-30 17:19:29
|
On Sun, 2006-01-29 at 20:54 +0000, Julian Seward wrote: > But VALGRIND_MAGIC_SEQUENCE is a low-level macro used as a building > block for the higher client-request macros that users should be using. > So I don't thing there should be a problem. In any case I sure hope > not since I recently renamed it to VALGRIND_DO_CLIENT_REQUEST as that's > a more sensible name for it. That might explain a few things, I took a private copy of memcheck.h/valgrind.h a few weeks ago (on the 17th Jan IIRC) and just found today that they no longer work with valgrind from SVN. Copying in new valgrind header files and rebuilding fixed the problem. Ashley, |
|
From: Julian S. <js...@ac...> - 2006-01-30 18:21:53
|
On Monday 30 January 2006 17:01, Ashley Pittman wrote: > On Sun, 2006-01-29 at 20:54 +0000, Julian Seward wrote: > > But VALGRIND_MAGIC_SEQUENCE is a low-level macro used as a building > > block for the higher client-request macros that users should be using. > > So I don't thing there should be a problem. In any case I sure hope > > not since I recently renamed it to VALGRIND_DO_CLIENT_REQUEST as that's > > a more sensible name for it. > > That might explain a few things, I took a private copy of > memcheck.h/valgrind.h a few weeks ago (on the 17th Jan IIRC) and just > found today that they no longer work with valgrind from SVN. Oops, sorry about that. It didn't even occur to me I'd be breaking other people's code. Perhaps we should say, on each macro, whether they are suitable for end-user use or not. J |
|
From: Ashley P. <as...@qu...> - 2006-01-31 09:46:57
|
On Mon, 2006-01-30 at 18:21 +0000, Julian Seward wrote:
> On Monday 30 January 2006 17:01, Ashley Pittman wrote:
> > On Sun, 2006-01-29 at 20:54 +0000, Julian Seward wrote:
> > > But VALGRIND_MAGIC_SEQUENCE is a low-level macro used as a building
> > > block for the higher client-request macros that users should be using.
> > > So I don't thing there should be a problem. In any case I sure hope
> > > not since I recently renamed it to VALGRIND_DO_CLIENT_REQUEST as that's
> > > a more sensible name for it.
> >
> > That might explain a few things, I took a private copy of
> > memcheck.h/valgrind.h a few weeks ago (on the 17th Jan IIRC) and just
> > found today that they no longer work with valgrind from SVN.
>
> Oops, sorry about that. It didn't even occur to me I'd be breaking
> other people's code. Perhaps we should say, on each macro, whether
> they are suitable for end-user use or not.
It seems a bit more fundamental than that, even RUNNING_ON_VALGRIND has
broken, code compiled with the old headers always returns 0.
valgrind.h and memcheck.h already contain documentation on what to call
and what not to call from end-user code, basically anything except one
memcheck entry.
/* This is just for memcheck's internal use - don't use it */
_VG_USERREQ__MEMCHECK_RECORD_OVERLAP_ERROR
= VG_USERREQ_TOOL_BASE('M','C') + 256
Whilst we are on the subject I've noticed that when I include valgrind.h
in results in two extra symbols being exported, VALGRIND_PRINTF and
VALGRIND_PRINTF_BACKTRACE. This probably isn't a problem as they are
weak symbols but there is no explanation of what they do or how I might
use them.
Ashley,
|
|
From: Joe M. <jo...@no...> - 2006-02-02 20:05:59
Attachments:
realloc.patch
|
Ok, here is the patch for REALLOCLIKE_BLOCK. Sorry for the delay - I kept forgetting about it while I was at work. Joe |