|
From: Nicholas N. <nj...@ca...> - 2004-06-27 21:26:43
Attachments:
diff.memory
|
Hi, I've trawled through Valgrind's memory allocation functions, and refactored and simplified things a bit. I haven't really made any progress on the goal of having just client and Valgrind areas; that looks (surprise surprise) harder than I first thought. Patch is attached, diff'd against current HEAD. ChangeLog is below. I'd be interested to hear comments, esp. from Jeremy. Thanks. N Changed replacement malloc() so that it returns 0 if it fails, rather than aborting with an assertion failure. This is consistent with the non-replacement malloc() used by eg. Nulgrind. Removed the 'place-holder' behaviour of VG_(mmap). Previously, VG_(mmap) would add a segment mapping to the segment skip-list, and then often the caller of VG_(mmap) would do another one for the same segment, just to change the SF_* flags. Now VG_(mmap) gets passed the appropriate SF_* flags so it can do it directly. This simplifies things, and allows VG_(client_alloc) and VG_(client_free) to be removed. In vg_syscalls.c, inlined mprotect_segment and munmap_segment because it's more concise and easier to understand that way. Strengthened checking in VG_(mmap), POST(mmap), POST(mmap2) -- now if the result is not in the right place, it aborts rather than unmapping and continuing. This is because if it's not in the right place, something bad has gone wrong. Partly reordered main(), so that some related things are together more, and fixed up some of the comments describing ordering dependencies there. Removed an unnecessary VG_(unmap_range)() call from do_brk(). Added a lot of checking for calls to mmap() and munmap() which were assumed to be succeeding. |
|
From: Nicholas N. <nj...@ca...> - 2004-06-29 08:50:27
|
On Mon, 28 Jun 2004, Jeremy Fitzhardinge wrote:
> I'm picking through it now,
Great, thanks for having a look.
> but I remember one potential problem in
> getting rid of Valgrind's heap. There's some code in there near startup
> which "knows" that the address space range for the heap has already been
> reserved in the segment map, but the mmap area is not covered by the
> segment map. This is a pretty fragile area, because if memory gets
> allocated without being in the segment map, it will all look OK until it
> gets mmaped over...
>
> Hence the comment:
> + // Build segment map -- first, do valgrind segments only, with no
> + // symbols. This is so we know where the free space is before we sta=
rt
> + // using mmap when reading symbols, in the second stage (Nb: using he=
ap
> + // is ok before this point).
>
> Why remove this comment?
It just got moved into setup_segments_list().
You're right about the heap; I stumbled across the startup delicacy=20
involving it pretty quickly. Without it, there's a real bootstrapping=20
problem, in that V needs the segment list initialised before it can=20
allocate any memory for itself, but it also needs to allocate memory in=20
order to initialise the segment list. That's why I've done this partial=20
overhaul rather than the full thing, because I can't see how it can be=20
done easily.
> This kind of stuff is absolutely evil:
>
> @@ -1103,7 +1104,7 @@ Addr open_debug_file( Char* name, UInt c
> VG_(close)(fd);
>
> if (calc_gnu_debuglink_crc32(0, (UChar*)addr, *size) !=3D crc) {
> - VG_(munmap)((void*)addr, *size);
> + vg_assert(0 =3D=3D VG_(munmap)((void*)addr, *size));
> return 0;
> }
>
> You should *never* put an expression with side-effects into an assert-
> style statement. Perhaps we should have another kind of syscall-retval
> checker function (which could also print a useful message with the
> errno).
Hmm, on one hand I agree, but on the other hand I'm not so sure -- since=20
we are using our own vg_assert() which is never turned off... how would a=
=20
new syscall-retval-checking function be any different, other than not=20
having "assert" in the name? I guess the short question is: what's the=20
definition of "assert-style"?
> I'm not too happy about losing VG_(client_alloc/free). If we decide to
> change the way that client allocations are done, it would be useful if
> all the client allocations are coming through the same common place.
> What's the gain in dropping them?
Less code, and consistency. They weren't all coming through the same=20
place anyway.
VG_(client_alloc) was being used for:
- new thread stacks
- client malloc()
It wasn't being used (VG_(mmap)(VKI_MAP_CLIENT) was) for
- client brk()
- extending stacks
I could put them back in. But it was exactly this kind of thing I was=20
trying to get rid of -- having two ways of doing the same thing, without=20
an apparent (or documented) reason for why one was preferred in any place.=
=20
Even with my changes, the call graph is still annoyingly convoluted. I'd=
=20
still love to get rid of Valgrind's heap; AFAICT the startup delicacy is=
=20
the only compelling reason to keep it. Maybe static memory could be used=
=20
to hold segment list nodes alloc'd before VG_(mmap) is safe to use.
I'd also like PRE(mmap) and PRE(munmap) to call VG_(mmap) and VG_(munmap)=
=20
rather than working stuff out for themselves, since the actions in both=20
function pairs is basically the same; the main obstacle for this is=20
*%^$=A3! mremap(), but I could move much of PRE(mremap)'s workings into=20
VG_(mremap). Then the only functions working directly with the=20
segment-list (eg. find_map_space, map_segment, unmap_range) would be=20
VG_(mmap), VG_(munmap), and main().
Both of these desires have the same goal -- for all allocations to go via=
=20
VG_(mmap) and VG_(munmap) (and VG_(mremap), unavoidably).
Thanks for the feedback.
N |
|
From: Nicholas N. <nj...@ca...> - 2004-06-29 15:07:13
|
On Tue, 29 Jun 2004, Nicholas Nethercote wrote: > You're right about the heap; I stumbled across the startup delicacy > involving it pretty quickly. Without it, there's a real bootstrapping > problem, in that V needs the segment list initialised before it can allocate > any memory for itself, but it also needs to allocate memory in order to > initialise the segment list. That's why I've done this partial overhaul > rather than the full thing, because I can't see how it can be done easily. I've worked out how to do this now. The solution is only mildly hacky -- the first superblock allocated by newSuperblock() is now done not with a mapping, but with a static block. This gives the startup code enough memory to play with in order to set up the segment list, and safely start allocating with VG_(mmap). I've got checking in place to ensure that this all works as it should (eg. that the static block is big enough, and that the segment-list-initialisation code is responsible for the first allocations). Using this, I've been able to remove VG_(brk) and Valgrind's heap, and am doing all V's allocations in its map-seg-area. So far, so good. Currently V itself (stage2) is always mapped at 0xb8000000 which is sucky (this is the 'kickstart_base' value). In my new version I'm putting it at 0xbf000000, just below V's stack (libs get put lower now). But the number is still hardwired, and based around the assumption that the stack starts at 0xc0000000. I thought an install-time test that determined where the stack normally goes could be useful for systems that put the stack at eg. 0xffff0000 -- I could put stage2 a short distance before the stack, and everything is done relative to that. Another problem is the libs used by Valgrind (ld.so, libc.so, libdl.so) which get loaded before main() begins. Basically, I'd like to jam them up just below stage2, but this requires knowing how big they are, and I can't see how to do that. Any ideas? Thanks. N |
|
From: Jeremy F. <je...@go...> - 2004-06-28 22:34:11
|
On Sun, 2004-06-27 at 22:26 +0100, Nicholas Nethercote wrote:
> I've trawled through Valgrind's memory allocation functions, and
> refactored and simplified things a bit. I haven't really made any
> progress on the goal of having just client and Valgrind areas; that looks
> (surprise surprise) harder than I first thought.
>
> Patch is attached, diff'd against current HEAD. ChangeLog is below. I'd
> be interested to hear comments, esp. from Jeremy.
I'm picking through it now, but I remember one potential problem in
getting rid of Valgrind's heap. There's some code in there near startup
which "knows" that the address space range for the heap has already been
reserved in the segment map, but the mmap area is not covered by the
segment map. This is a pretty fragile area, because if memory gets
allocated without being in the segment map, it will all look OK until it
gets mmaped over...
Hence the comment:
+ // Build segment map -- first, do valgrind segments only, with no
+ // symbols. This is so we know where the free space is before we start
+ // using mmap when reading symbols, in the second stage (Nb: using heap
+ // is ok before this point).
Why remove this comment?
--- coregrind/vg_memory.c 13 Mar 2004 02:06:58 -0000 1.52
+++ coregrind/vg_memory.c 27 Jun 2004 21:20:06 -0000
@@ -335,7 +335,6 @@ void VG_(map_file_segment)(Addr addr, UI
s = VG_(SkipList_Find)(&sk_segments, &addr);
if (s != NULL && s->addr == addr && s->len == len) {
- /* This probably means we're just updating the flags */
recycled = True;
This kind of stuff is absolutely evil:
@@ -1103,7 +1104,7 @@ Addr open_debug_file( Char* name, UInt c
VG_(close)(fd);
if (calc_gnu_debuglink_crc32(0, (UChar*)addr, *size) != crc) {
- VG_(munmap)((void*)addr, *size);
+ vg_assert(0 == VG_(munmap)((void*)addr, *size));
return 0;
}
You should *never* put an expression with side-effects into an assert-
style statement. Perhaps we should have another kind of syscall-retval
checker function (which could also print a useful message with the
errno).
I'm not too happy about losing VG_(client_alloc/free). If we decide to
change the way that client allocations are done, it would be useful if
all the client allocations are coming through the same common place.
What's the gain in dropping them?
Looks good, otherwise.
J
|
|
From: Jeremy F. <je...@go...> - 2004-06-29 17:09:42
|
On Tue, 2004-06-29 at 09:49 +0100, Nicholas Nethercote wrote: > Hmm, on one hand I agree, but on the other hand I'm not so sure -- sinc= e=20 > we are using our own vg_assert() which is never turned off... how would= a=20 > new syscall-retval-checking function be any different, other than not=20 > having "assert" in the name? I guess the short question is: what's the= =20 > definition of "assert-style"? Well, something with "assert" in the name. I would say that anything with assert in the name is expected to behave like assert(), even if we don't intend to allow it to be disabled. But aside from that, we should give more useful messages if we fail from out of memory. > VG_(client_alloc) was being used for: > - new thread stacks > - client malloc() >=20 > It wasn't being used (VG_(mmap)(VKI_MAP_CLIENT) was) for > - client brk() > - extending stacks But those are distinct operations. The former is placing an allocation at some arbitrary location in the client address space, whereas the latter are mapping memory at very specific places. > I'd also like PRE(mmap) and PRE(munmap) to call VG_(mmap) and VG_(munma= p)=20 > rather than working stuff out for themselves, since the actions in both= =20 > function pairs is basically the same; the main obstacle for this is=20 > *%^$=C2=A3! mremap(), but I could move much of PRE(mremap)'s workings i= nto=20 > VG_(mremap). Then the only functions working directly with the=20 > segment-list (eg. find_map_space, map_segment, unmap_range) would be=20 > VG_(mmap), VG_(munmap), and main(). Cool, it would be nice to unify these. I seem to remember there were some subtleties (aren't there ever), but it should be OK. The plan I've had in the back of my mind for allocation is to expose memory-allocating functions (specifically, malloc, calloc, realloc, free, new, delete, mmap, munmap and maybe brk/sbrk) so that 1) we can use them internally (maybe) and 2) third-party libraries can use them. I think we could get a lot of power from moving to C++ as an implementation language, and a lot of the stuff in the STL is a good match for what we're doing. Memory isn't the only problem for using 3rd-party libraries - we have to worry about all the other resources they can allocate. For example, if a library creates a file-descriptor, we need to make sure its out of reach of the client range. I've been thinking about completely virtualizing the FD namespace for clients, but I think that's a lot of complexity for not much gain. So the question is still open. J |
|
From: Nicholas N. <nj...@ca...> - 2004-07-01 13:12:22
|
On Tue, 29 Jun 2004, Jeremy Fitzhardinge wrote:
>> Hmm, on one hand I agree, but on the other hand I'm not so sure -- since
>> we are using our own vg_assert() which is never turned off... how would a
>> new syscall-retval-checking function be any different, other than not
>> having "assert" in the name? I guess the short question is: what's the
>> definition of "assert-style"?
>
> Well, something with "assert" in the name. I would say that anything
> with assert in the name is expected to behave like assert(), even if we
> don't intend to allow it to be disabled.
I notice that vg_assert() and sk_assert() are being used like this
frequently:
executable_name = strdup(buf);
vg_assert(NULL != executable_name);
Ie. using them not only for sanity checking (ie. if this fails we have a
bug) but also for error checking (ie. if this fails we ran out of memory).
In other words, the "no side-effects in assertions" rule is too loose; if
assertions can be removed, this sort of thing shouldn't be tolerated
either...
N
|