[Valgrind-developers] Re: Bypassing valgrind-supplied malloc() ?

[Nicholas N. <nj...@ca...>]

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