|
From: Nicholas N. <nj...@cs...> - 2006-08-19 05:03:13
|
On Fri, 18 Aug 2006, Bart Van Assche wrote: > A question for the Valgrind developers: I have made changes to Valgrind's > core in order to get the drd tool working. How should I proceed to get these > changes applied to the official Valgrind distribution ? Should I split up > the patch ? Should I wait until after release 3.x.y ? I just looked again through your patch at http://home.scarlet.be/~bvassche/drd/valgrind-5999.patch. Much of it looks good and if Julian is also happy I don't see why it can't be added now -- it's almost all code that shouldn't affect existing tools. More specifically: - include/pub_{core,tool}_tooliface.h - changing void* params to Addr seems fine - adding the extra mutex/cond events seems fine - include/pub_{core,tool}_threadstate.[hc], include/pub_tool_basics.h: - the extra thread ID getters seem fine - new PosixThreadId type -- what's it for? Does it assume that pthread_t is an integer also? - include/pub_tool_libcprint.h: - not sure about the format __attribute__ changes, I don't understand the __GNUC__ and CHECK_FORMAT_STRINGS preprocessor stuff - include/pub_tool_debuginfo.[ch]: - adding VG_(seginfo_ec_kind_name) seems fine - incude/valgrind.h, coregrind/m_scheduler/scheduler.c: - adding the extra client requests for the thread events seems ok, but I'm not expert on how that stuff works, would be good if Julian ok'd it - coregrind/vg_preloaded.c: - I don't think this is the right place for all this thread stuff. Valgrind's malloc-replacement stuff is in a separate module (m_replacemalloc) and tools can incorporate that module by calling VG_(needs_malloc_replacement) and by linking with $(LIBREPLACEMALLOC_LDFLAGS_*) -- see massif/ms_main.c and massif/Makefile.am. This setup is a little fiddly, but has the crucial property that the original malloc is used for tools that don't need malloc replacement. We may want the same behaviour for the thread function stuff, ie. DRD and Helgrind would use it, but Memcheck and Cachegrind wouldn't. If we do this, then the thread events wouldn't have their own events the samw way heap allocs/deallocs aren't seen by threads by using events, but instead via VG_(needs_malloc_replacement). Hmm, now I'm not sure. Maybe this is different to the malloc case because the thread functions are being wrapped but not replaced outright. One thing is certain: this core file definitely shouldn't #include drd/drd_client_req.h - coregrind/m_syswrap/syswrap-linux.c: - why have you commented out some of the asm lines? - coregrind/m_threadmodel.c: - I see your changes are only minor... thinking more broadly, what's this file for? IIRC Jeremy wrote it and m_pthreadmodel.c in an attempt to get the basic thread checking that was present in 2.x working again, but it never really worked. Do we even need this file? It seems like it would make sense to have the basic pthread checks in DRD and/or Helgrind, rather than in the core. Once DRD is in a state where you think it's ready to be used by others, it would be good to post something to the users list explaining what it is and how it works... hopefully you'll get some people trying it out, because enough people have complained about Helgrind not working. I don't know how close you are to that point, though. Nick |