|
From: Nicholas N. <nj...@ca...> - 2003-03-27 14:16:46
|
Hi,
We've been discussing ways for skins to not use Valgrind's version of
malloc(). My supervisor Alan pointed out something that should have been
obvious to me. Valgrind currently defines malloc() et al and memcpy() et
al in the core, so all skins have to use them.
KEY IDEA: Why can't they just be defined in the skins that use them?
They would override the glibc version as required, but only for the skins
that require them. An easy example: memcpy() is currently overridden by
Valgrind for Memcheck, because the super-optimised glibc version confuses
Memcheck. So move coregrind/vg_clientfuncs.c:memcpy() into Memcheck...
and hey presto, Memcheck stays unconfused, but all other skins can use
glibc's memcpy() as normal -- even Addrcheck, since the confusion doesn't
involve A bits.
I think this is simpler than any of the other options we've discussed. I
just tried it with memcpy(), it works fine.
malloc() et al are a bit tricker... if they are running on the simulated
CPU, they have to use a client request (VG_USERREQ__MALLOC).
[I don't understand exactly why this client request route must be taken,
although things screw up if it's not.]
In this case, happily there's already the request VG_USERREQ__MALLOC; in
general, there could be a VG_USERREQ__CALL_FN request that a skin could
use to call any function it wanted this way.
A few things currently only core-visible would have to be made
skin-visible (eg. VG_(running_on_simd_CPU)), but that's no big deal.
The --trace-malloc and --sloppy-malloc command line args would also be
moved out of core into Memcheck.
The "core_errors" need would no longer check for silly (eg. negative) args
to malloc -- that's controlled by the skin.
The following events could be removed altogether:
track_new_mem_heap
track_copy_mem_heap
track_ban_mem_heap
track_die_mem_heap
since they would be directly visible to a skin if it wanted, by providing
its own version of malloc().
The ShadowChunk stuff -- recording extra information about heap blocks --
would also be moved out of the core.
These events:
track_bad_free
track_mismatched_free
would also be removed from core, since they can be detected by the skin.
Different skins could provide different wrappers for malloc() -- eg.
helgrind only cares about the track_new_mem_heap event, so its wrapper
would be much simpler than Memcheck's, and could even call __libc_malloc()
since it doesn't need redzones.
I just tried this with malloc() (I cheated a bit to make it simple), it
too works fine.
The only downside to all this I can see is this: you can't just provide a
wrapper, your replacement has to actually do the job of the replaced
function, unless you have an alternative name with which to call a
function. Eg. malloc() has the alternative name __libc_malloc() , so you
could define a wrapper:
void* malloc(...)
{
// wrapper code here
return __libc_malloc(...)
}
but it wouldn't work for, say, memcpy(), because there is no
__libc_memcpy() alternative name to call the original version (AFAIK).
But you can effectively wrap memcpy() by instrumenting the function's
entry, and it's what you have to do currently anyway.
[nb: yes GCC uses its own memcpy() at high optimisation levels... but
ignore that for this discussion, it applies for any other function]
The leak checker could also be easily moved out of core into
Memcheck/Addrcheck (they already share some code, so no problem there).
Currently it's in core because some of the ShadowChunk stuff makes it
painful to move out.
----
This seems to me a good idea. It would make the core/skin split a lot
cleaner. The not-quite-right parts of the core/skin split are very much
involved with malloc() -- ShadowChunks, track_bad_free, malloc() always
using redzones, etc -- where Valgrind's need to support Memcheck was
compromising other skins.
Can anyone see any problems with this? Julian? Just to be provocative,
since version 2.0.0 is supposed to officially herald the core/skin split,
I think this change should go in before 2.0.0 is released, since without
it I don't think the core/skin split is complete... I'm happy to implement
it.
N
|
|
From: Tom H. <th...@cy...> - 2003-03-27 14:33:11
|
In message <Pin...@gr...>
Nicholas Nethercote <nj...@ca...> wrote:
> but it wouldn't work for, say, memcpy(), because there is no
> __libc_memcpy() alternative name to call the original version (AFAIK).
> But you can effectively wrap memcpy() by instrumenting the function's
> entry, and it's what you have to do currently anyway.
You can find it using dlsym though, either like this:
dlsym(RTLD_NEXT, "memcpy")
which requires you to define _GNU_SOURCE to get RTLD_NEXT, or by
doing a dlopen of libc.so and then using dlsym on that handle.
Tom
--
Tom Hughes (th...@cy...)
Software Engineer, Cyberscience Corporation
http://www.cyberscience.com/
|
|
From: Nicholas N. <nj...@ca...> - 2003-03-27 15:04:17
|
On 27 Mar 2003, Tom Hughes wrote: > > but it wouldn't work for, say, memcpy(), because there is no > > __libc_memcpy() alternative name to call the original version (AFAIK). > > But you can effectively wrap memcpy() by instrumenting the function's > > entry, and it's what you have to do currently anyway. > > You can find it using dlsym though, either like this: > > dlsym(RTLD_NEXT, "memcpy") > > which requires you to define _GNU_SOURCE to get RTLD_NEXT, or by > doing a dlopen of libc.so and then using dlsym on that handle. Except Valgrind doesn't use glibc at all. Maybe such a search wouldn't be too hard to write from scratch, I don't know. N |
|
From: Tom H. <th...@cy...> - 2003-03-27 15:45:24
|
In message <Pin...@gr...>
Nicholas Nethercote <nj...@ca...> wrote:
> On 27 Mar 2003, Tom Hughes wrote:
>
> > > but it wouldn't work for, say, memcpy(), because there is no
> > > __libc_memcpy() alternative name to call the original version (AFAIK).
> > > But you can effectively wrap memcpy() by instrumenting the function's
> > > entry, and it's what you have to do currently anyway.
> >
> > You can find it using dlsym though, either like this:
> >
> > dlsym(RTLD_NEXT, "memcpy")
> >
> > which requires you to define _GNU_SOURCE to get RTLD_NEXT, or by
> > doing a dlopen of libc.so and then using dlsym on that handle.
>
> Except Valgrind doesn't use glibc at all. Maybe such a search wouldn't be
> too hard to write from scratch, I don't know.
I suspect it would be quite hard. Strictly speaking dlsym is libdl
rather than libc but I guess that's all part of glibc really.
Anyway, if it doesn't use libc how can it call __libc_malloc as you
were suggesting it should to pass on malloc?
Tom
--
Tom Hughes (th...@cy...)
Software Engineer, Cyberscience Corporation
http://www.cyberscience.com/
|
|
From: Nicholas N. <nj...@ca...> - 2003-03-27 15:57:41
|
On 27 Mar 2003, Tom Hughes wrote: > Anyway, if it doesn't use libc how can it call __libc_malloc as you > were suggesting it should to pass on malloc? Hmm, good point. It's kind of different to Valgrind's core using glibc functions, since the wrapper and the __libc_malloc call are (usually) happening on the simulated CPU. But glibc still probably shouldn't be used (we've had at least one mega-obscure bug cause by an accidental glibc dependency in the past). In which case you can't wrap a glibc function in this way; the skin's replacement version must replicate the functionality. But that's still ok -- for malloc() et al for Memcheck, that's no problem. And for other skins that might want to wrap malloc() but use the glibc version, they can still spot malloc()'s entry/exit points at instrumentation time to achieve the same effect. N |
|
From: Nicholas N. <nj...@ca...> - 2003-03-28 00:27:03
|
On Thu, 27 Mar 2003, Nicholas Nethercote wrote:
> > Anyway, if it doesn't use libc how can it call __libc_malloc as you
> > were suggesting it should to pass on malloc?
>
> Hmm, good point.
>
> It's kind of different to Valgrind's core using glibc functions, since the
> wrapper and the __libc_malloc call are (usually) happening on the
> simulated CPU. But glibc still probably shouldn't be used (we've had at
> least one mega-obscure bug cause by an accidental glibc dependency in the
> past).
More about this... I saw a comment in vg_libpthread.c, which is full of
functions that override the system definitions in this way:
<quote>
/* ALL THIS CODE RUNS ON THE SIMULATED CPU.
This is a replacement for the standard libpthread.so. It is loaded
as part of the client's image (if required) and directs pthread
calls through to Valgrind's request mechanism.
A couple of caveats.
<snip>
2. Since this runs as part of the client, there are no specific
restrictions on what headers etc we can include, so long as
this libpthread.so does not end up having dependencies on .so's
which the real one doesn't.
Later ... it appears we cannot call file-related stuff in libc here,
perhaps fair enough. Be careful what you call from here. Even exit()
doesn't work (gives infinite recursion and then stack overflow); hence
myexit(). Also fprintf doesn't seem safe.
*/
</quote>
vg_libpthread.c #includes several system headers.
So if the overridden functions for a skin (eg. malloc() et al) are kept
together in a single file, then maybe it's ok for that file to #include
the stuff necessary for it to call __libc_malloc(). Maybe.
N
|