|
From: unbutun <un...@so...> - 2012-02-20 11:43:41
|
Hi, guys This is Gavin in China, first, forgive my poor english please Now, we wrote a dynamic lib with a set of malloc and free, and we build the uclibc and our lib in a elf. The malloc in uclibc is weak symbol, our is strong symbol, so the elf could run with no error but when we use valgrind, we found that valgrind is worked with nothing report The dynamic lib which we wrote is to do something we need and uclibc could not provide My boss tell me the problem must be resolved, could you give me some advice of how to fix that(add some code in valgrind or do some other thing to fix it) ? Any advice would be much appreciated, thanks advance Regards, Gavin |
|
From: Philippe W. <phi...@sk...> - 2012-02-20 20:24:55
|
On Mon, 2012-02-20 at 19:43 +0800, unbutun wrote: > Now, we wrote a dynamic lib with a set of malloc and free, and we > build the uclibc and our lib in a elf. > The malloc in uclibc is weak symbol, our is strong symbol, so the elf > could run with no error > but when we use valgrind, we found that valgrind is worked with > nothing report > > The dynamic lib which we wrote is to do something we need and uclibc > could not provide I assume that your library contains implementations of malloc/free/... which have the same behaviour as the "standard" malloc/free/... To have Valgrind properly tracking the memory malloc-ed/free-d by your lib, you must tell that these functions have to be replaced for e.g. memcheck. For this, you must modify the file coregrind/m_replacemalloc/vg_replace_malloc.c For example, for malloc, you currently have: ALLOC_or_NULL(VG_Z_LIBSTDCXX_SONAME, malloc, malloc); ALLOC_or_NULL(VG_Z_LIBC_SONAME, malloc, malloc); I believe if you add a line ALLOC_or_NULL(VG_Z_YOURLIB_SONAME, malloc, malloc); where VG_Z_YOURLIB_SONAME is to be defined similarly to others in include/pub_tool_redir.h, then this should work. (of course, you must do that for all the "malloc" related functions you have implemented in your library). Alternatively, if you add a line ALLOC_or_NULL(NONE, malloc, malloc); then malloc will be replaced whatever the lib (static or dynamic) where they are (re-)defined. Philippe |
|
From: Andrew C. M. <and...@gm...> - 2012-02-28 21:39:01
Attachments:
vg.replace_malloc.patch
|
On Mon, Feb 20, 2012 at 3:25 PM, Philippe Waroquiers <phi...@sk...> wrote: > > > For this, you must modify the file > coregrind/m_replacemalloc/vg_replace_malloc.c > > For example, for malloc, you currently have: > ALLOC_or_NULL(VG_Z_LIBSTDCXX_SONAME, malloc, malloc); > ALLOC_or_NULL(VG_Z_LIBC_SONAME, malloc, malloc); > > I believe if you add a line > ALLOC_or_NULL(VG_Z_YOURLIB_SONAME, malloc, malloc); > where VG_Z_YOURLIB_SONAME is to be defined similarly to others > in include/pub_tool_redir.h, then this should work. > I have done this a few times, most often to add support for Google's tcmalloc library to valgrind. The current structure of vg_replace_malloc.c means you must either make many small and widely separated edits to different areas of the file, one for each new interception, or not honor the organization of the file. If you do follow the current organization, then each edit site is complex, having nested #ifdefs, commented out regions, varying mangled names, etc. This makes maintaining a patch to support an alternative allocator harder, since there are often multiple non-trivial conflicts when upstream vg_replace_malloc.c changes. I've attached a sketch for a patch that moves all of the interception applications to a common area at the end of vg_replace_malloc.c, right before 'init'. In addition, the patch merges the relevant VGO_linux and VGO_darwin conditional compilation sections, macro-izes the mangled new/delete signatures to abstract away VG_WORDSIZE variation, and groups the interceptions for each platform by library. The file seems much simpler this way, and it could be simplified further if it is determined that the many currently commented out Darwin interceptions can be removed. I left the Darwin conditional compilation things alone because I was unsure about the intended semantics. Arranged this way, I think it is much easier to see the overall structure of the interceptions for each platform and library. This should make it easier to see how and where to add support for other allocators and to maintain the resulting patch as vg_replace_malloc.c evolves. Thoughts? Thanks, Andrew |
|
From: Philippe W. <phi...@sk...> - 2012-02-29 20:45:01
|
On Tue, 2012-02-28 at 16:38 -0500, Andrew C. Morrow wrote:
> Arranged this way, I think it is much easier to see the overall
> structure of the interceptions for each platform and library. This
> should make it easier to see how and where to add support for other
> allocators and to maintain the resulting patch as vg_replace_malloc.c
> evolves. Thoughts?
I think there are 2 possible "groupings":
grouping by function (the current setup)
(making it easier to compare how a certain function
is replaced on different platforms).
grouping by platform/library.
(separating each platform from the others, making it
easier to add a new lib such as tcmalloc).
I have a very slight preference for the 'lib' based structure,
but so slight to not push for it :).
At work, we are using tcmalloc (static linking), and we have
to patch this file.
I have discussed integrating the needed replacements for libtcmalloc
(the shared version).
Such a patch should be ok, but I have not taken the time to finalize
it.
I would like to find a solution to have a replacement for
static linking, but I understand this is more of kludge
(at least if the replacement is unconditional).
Philippe
|
|
From: Julian S. <js...@ac...> - 2012-03-07 08:34:38
|
> I've attached a sketch for a patch that moves all of the interception > applications to a common area at the end of vg_replace_malloc.c, right > before 'init'. In addition, the patch merges the relevant VGO_linux > and VGO_darwin conditional compilation sections, We used to have combined linux and darwin intercepts in most of the file. I separated them all out some time last year. The all-merged- together scheme looks good at first glance, but leads to maintenance and validation problems: I found it hard to figure out if a change to the intercept schemes for (eg) darwin would break linux, or vice versa. This got particularly bad when dealing with memmove-vs-memcpy complexity in Linux last year. In the end decided to make them independent, even if it meant a bit of code duplication. From where I'm sitting, one of the biggest problems with we have is not so much one of "fix bug X" or "add feature Y", but simply that of validating (usually around release time) that what we have actually works correctly. As the number of supported architectures, OSs, OS versions, libc versions, other C libraries etc increases, the validation problem expands exponentially. J |
|
From: Andrew C. M. <and...@gm...> - 2012-03-08 17:22:45
|
2012/3/7 Julian Seward <js...@ac...>: > >> I've attached a sketch for a patch that moves all of the interception >> applications to a common area at the end of vg_replace_malloc.c, right >> before 'init'. In addition, the patch merges the relevant VGO_linux >> and VGO_darwin conditional compilation sections, > > We used to have combined linux and darwin intercepts in most of the > file. I separated them all out some time last year. The all-merged- > together scheme looks good at first glance, but leads to maintenance > and validation problems: I found it hard to figure out if a change to > the intercept schemes for (eg) darwin would break linux, or vice > versa. This got particularly bad when dealing with memmove-vs-memcpy > complexity in Linux last year. In the end decided to make them > independent, even if it meant a bit of code duplication. > > From where I'm sitting, one of the biggest problems with we have is > not so much one of "fix bug X" or "add feature Y", but simply that > of validating (usually around release time) that what we have actually > works correctly. As the number of supported architectures, OSs, > OS versions, libc versions, other C libraries etc increases, the > validation problem expands exponentially. > > J Thank you for the detailed reply. If I'd realized that my patch amounted to backing out your earlier work I would not have proposed it. I do have a few follow up questions though: - My motivation for the reorganization was to make it easier to maintain my own custom valgrind fork that adds support for tcmalloc. I'm not the only one doing this. I realize that adding support for even one third-party allocator opens the door for everyone to clamor for support for their personal favorite, and in addition makes the release-time validation work that you mentioned above that much harder. But providing some best effort level of support for one or two widely used third-party allocators would be a popular feature. In addition, a well integrated solution would provide guidance for others who need to add support for other custom allocators in local forks. Would a patch that integrated optional support for tcmalloc have a shot at inclusion? If so I'm willing to work on it. - If the answer is yes, do you have any thoughts on how best to structure it? I don't think that just adding a big #ifdef'ed block of tcmalloc specific interceptions at the end of vg_replace_malloc.c is at all the right way to go. Scattering tcmalloc interceptions throughout the file and further complicating the platform #ifdef blocks doesn't seem right either. It might be nice to have all of the tcmalloc specific items in a separate file, but that doesn't work because 'init' in vg_replace_malloc.c needs to see all of the interceptions. One possibility would be to place the tcmalloc interceptions in a separate 'vg_replace_tcmalloc.c' file, and then conditionally include it near the end of vg_replace_malloc.c. But #including .c files is not particularly appealing either. Any suggestions? - The patch that I posted provided per-VG_WORDSIZE macros for the mangled C++ names (to remove repeating the easy to misread or mistype mangled names), created VG_WORDSIZE independent macros from those, and then collapsed the conditional compilation around VG_WORDSIZE for the C++ interceptions by using the new macros. Would there be interest in a patch that did just this (or maybe just the first part of it), or do you prefer to keep the variation along this axis explicit as well? Sorry about the long questions... Thanks, Andrew |
|
From: Philippe W. <phi...@sk...> - 2012-03-08 18:15:25
|
On Thu, 2012-03-08 at 12:22 -0500, Andrew C. Morrow wrote: > Would a patch that integrated optional support for tcmalloc have a > shot at inclusion? If so I'm willing to work on it. At work, we are using tcmalloc and are maintaining a patch to have tcmalloc properly supported (IIRC, derived from the patch in bug 219156). I discussed the integration of tcmalloc support with Julian at FOSDEM. No objection to the patch supporting tcmalloc shared lib. (at work, we are using static linking. Integrating support for statically linked library is not ok, see below). I have on my (too big) list of things to do to push this patch in svn. > > - If the answer is yes, do you have any thoughts on how best to > structure it? I don't think that just adding a big #ifdef'ed block of > tcmalloc specific interceptions at the end of vg_replace_malloc.c is > at all the right way to go. Scattering tcmalloc interceptions > throughout the file and further complicating the platform #ifdef > blocks doesn't seem right either. It might be nice to have all of the > tcmalloc specific items in a separate file, but that doesn't work > because 'init' in vg_replace_malloc.c needs to see all of the > interceptions. One possibility would be to place the tcmalloc > interceptions in a separate 'vg_replace_tcmalloc.c' file, and then > conditionally include it near the end of vg_replace_malloc.c. But > #including .c files is not particularly appealing either. Any > suggestions? IIRC the patch we have at work (which I should commit one of these days/weeks/months :() is doing the following : * add the name of tcmalloc library in pub_tool_redir.h * add one line in vg_replace_malloc.c for each of the allocation/free functions (__builtin_new, malloc, etc). To my knoweldge, there is no need for any #ifdef, as the replacement mechanism will only replace these functions in a library matching the specific library name you give. I am not sure of the reason why replacement with NONE is not desirable/a bad thing: I believe it is because "static" replacement, using NONE as lib, would replace any function matching the given function name, which could cause problems for some applications. It would be nice if a statically linked object would "remember" from which lib it is coming from. Then we could do "safe static" replacement of e.g. tcmalloc and any other statically linked malloc library. I do not think such "remembering" is possible. I have also not found a way to activate static replacement with a command line option. > > - The patch that I posted provided per-VG_WORDSIZE macros for the > mangled C++ names (to remove repeating the easy to misread or mistype > mangled names), created VG_WORDSIZE independent macros from those, and > then collapsed the conditional compilation around VG_WORDSIZE for the > C++ interceptions by using the new macros. Would there be interest in > a patch that did just this (or maybe just the first part of it), or do > you prefer to keep the variation along this axis explicit as well? Can't answer to that one, as I have not looked in detail at your patch, and I am not familiar with C++ mangling and similar. Philippe |
|
From: Julian S. <js...@ac...> - 2012-03-08 18:25:55
|
On Thursday, March 08, 2012, Philippe Waroquiers wrote: > On Thu, 2012-03-08 at 12:22 -0500, Andrew C. Morrow wrote: > > Would a patch that integrated optional support for tcmalloc have a > > shot at inclusion? If so I'm willing to work on it. > > At work, we are using tcmalloc and are maintaining a patch > to have tcmalloc properly supported [...] Philippe, is your patch in the bug tracker somewhere? J |
|
From: Philippe W. <phi...@sk...> - 2012-03-08 19:22:54
|
On Thu, 2012-03-08 at 19:16 +0100, Julian Seward wrote: > On Thursday, March 08, 2012, Philippe Waroquiers wrote: > > On Thu, 2012-03-08 at 12:22 -0500, Andrew C. Morrow wrote: > > > Would a patch that integrated optional support for tcmalloc have a > > > shot at inclusion? If so I'm willing to work on it. > > > > At work, we are using tcmalloc and are maintaining a patch > > to have tcmalloc properly supported [...] > > Philippe, is your patch in the bug tracker somewhere? We started from the patch in Bug 219156 - Valgrind does not handle custom allocation functions. But this patch is doing a lot more than the tcmalloc shared: * it replaces any static usage of malloc/free/... * it replaces some "bash" allocators * it replaces some python allocators. So, someone at work prepared a simplified patch, updated for 3.8.0 and only containing replacement for tcmalloc shared. But this patch is lingering in my mailbox at work since some weeks now :(. I will look at this again tomorrow, and attach it to 219156. Philippe |
|
From: Philippe W. <phi...@sk...> - 2012-03-10 08:08:50
|
On Thu, 2012-03-08 at 19:16 +0100, Julian Seward wrote: > On Thursday, March 08, 2012, Philippe Waroquiers wrote: > > On Thu, 2012-03-08 at 12:22 -0500, Andrew C. Morrow wrote: > > > Would a patch that integrated optional support for tcmalloc have a > > > shot at inclusion? If so I'm willing to work on it. > > > > At work, we are using tcmalloc and are maintaining a patch > > to have tcmalloc properly supported [...] > > Philippe, is your patch in the bug tracker somewhere? I have just attached the patch to https://bugs.kde.org/show_bug.cgi?id=219156 Philippe |
|
From: Andrew C. M. <and...@gm...> - 2012-03-08 19:35:36
Attachments:
acm_tcmalloc.patch
|
On Thu, Mar 8, 2012 at 1:16 PM, Julian Seward <js...@ac...> wrote: > On Thursday, March 08, 2012, Philippe Waroquiers wrote: >> On Thu, 2012-03-08 at 12:22 -0500, Andrew C. Morrow wrote: >> > Would a patch that integrated optional support for tcmalloc have a >> > shot at inclusion? If so I'm willing to work on it. >> >> At work, we are using tcmalloc and are maintaining a patch >> to have tcmalloc properly supported [...] > > Philippe, is your patch in the bug tracker somewhere? > > J For what it is worth, here is the patch that I am currently using. It installs new-style tc_* interceptions if --enable-gperftools=yes is provided to configure, and adds additional old-style interceptions if --enable-gperftools=old is set. You don't need the old-style interceptions if you are using newer gperftools (1.8 or better I think). My autotools knowledge is quite limited so this may not be the best way to do this. You can also specify a Z encoded SONAME on the command line via --enable-gperftools-zso. If the flag is provided, it uses NONE. Thanks, Andrew |
|
From: Philippe W. <phi...@sk...> - 2012-03-10 08:30:00
|
On Thu, 2012-03-08 at 14:35 -0500, Andrew C. Morrow wrote: > For what it is worth, here is the patch that I am currently using. It > installs new-style tc_* interceptions if --enable-gperftools=yes is > provided to configure, and adds additional old-style interceptions if > --enable-gperftools=old is set. You don't need the old-style > interceptions if you are using newer gperftools (1.8 or better I > think). My autotools knowledge is quite limited so this may not be the > best way to do this. > > You can also specify a Z encoded SONAME on the command line via > --enable-gperftools-zso. If the flag is provided, it uses NONE. Hello Andrew, I quickly look at your patch. In terms of replacement, there are some differences with what I put in https://bugs.kde.org/show_bug.cgi?id=219156 E.g., malloc replacement is: ALLOC_or_NULL(VG_Z_LIBTCMALLOC_SONAME, malloc, malloc); compared to ALLOC_or_NULL(GPERFTOOLS_INTERCEPT_ZSONAME, tc_malloc, malloc); Checking on libtcmalloc.so, tc_malloc and malloc symbols have the same address in the lib, so the final redirection effect will be the same. However, this introduces a dependency to the (to my knowledge) non documented symbols tc_malloc and similar. => using the "standard" names looks preferrable to me. The SONAME configure time option is interesting. I will see if this can't be done dynamically with a command line option (i.e. at valgrind startup, rather than at configure time). Philippe |
|
From: Andrew C. M. <and...@gm...> - 2012-03-10 19:06:04
|
> > Hello Andrew, > > I quickly look at your patch. > > In terms of replacement, there are some differences with what I > put in https://bugs.kde.org/show_bug.cgi?id=219156 > E.g., malloc replacement is: > ALLOC_or_NULL(VG_Z_LIBTCMALLOC_SONAME, malloc, malloc); > compared to > ALLOC_or_NULL(GPERFTOOLS_INTERCEPT_ZSONAME, tc_malloc, malloc); > > Checking on libtcmalloc.so, tc_malloc and malloc symbols have the same > address in the lib, so the final redirection effect will be the same. > > However, this introduces a dependency to the (to my knowledge) > non documented symbols tc_malloc and similar. > => using the "standard" names looks preferrable to me. My patch imitated Google's valgrind-variant fork, which intercepts both the normal names and the tc_ names. But there is a comment from kcc that suggests that Google prefers intercepting via the tc_ names and that they would like to eventually remove the non tc_ interceptions: http://code.google.com/p/valgrind-variant/source/browse/trunk/valgrind/coregrind/m_replacemalloc/vg_replace_malloc.c#1067 Perhaps someone on the list from Google would be willing to provide some insight? > > The SONAME configure time option is interesting. > I will see if this can't be done dynamically > with a command line option (i.e. at valgrind startup, rather than > at configure time). That would be pretty cool if it works. Thanks for taking the time to look over my patch, Andrew |
|
From: Konstantin S. <kon...@gm...> - 2012-03-12 18:35:22
|
We have been using a hacked vg_replace_malloc.c for years. In most our programs tcmalloc is linked into the main executable, so there is no separate library with a soname, so we had to intercept malloc with soname=NONE. I remember just one problem with this approach: bash source has it's own redefined malloc/free functions, as well as sh_malloc/sh_free. And there are cases when a pointer allocated by sh_malloc is passed to free, which greatly confused our version of valgrind (which intercepted free, but not sh_malloc). So, we also had to intercept sh_malloc/sh_free. :( http://code.google.com/p/valgrind-variant/source/browse/trunk/valgrind/coregrind/m_replacemalloc/vg_replace_malloc.c#1097 For this particular case, intercepting tc_malloc/tc_free would help. hth, --kcc On Sat, Mar 10, 2012 at 11:05 AM, Andrew C. Morrow < and...@gm...> wrote: > > > > Hello Andrew, > > > > I quickly look at your patch. > > > > In terms of replacement, there are some differences with what I > > put in https://bugs.kde.org/show_bug.cgi?id=219156 > > E.g., malloc replacement is: > > ALLOC_or_NULL(VG_Z_LIBTCMALLOC_SONAME, malloc, malloc); > > compared to > > ALLOC_or_NULL(GPERFTOOLS_INTERCEPT_ZSONAME, tc_malloc, malloc); > > > > Checking on libtcmalloc.so, tc_malloc and malloc symbols have the same > > address in the lib, so the final redirection effect will be the same. > > > > However, this introduces a dependency to the (to my knowledge) > > non documented symbols tc_malloc and similar. > > => using the "standard" names looks preferrable to me. > > My patch imitated Google's valgrind-variant fork, which intercepts > both the normal names and the tc_ names. But there is a comment from > kcc that suggests that Google prefers intercepting via the tc_ names > and that they would like to eventually remove the non tc_ > interceptions: > > > http://code.google.com/p/valgrind-variant/source/browse/trunk/valgrind/coregrind/m_replacemalloc/vg_replace_malloc.c#1067 > > Perhaps someone on the list from Google would be willing to provide > some insight? > > > > > The SONAME configure time option is interesting. > > I will see if this can't be done dynamically > > with a command line option (i.e. at valgrind startup, rather than > > at configure time). > > That would be pretty cool if it works. > > Thanks for taking the time to look over my patch, > Andrew > > > ------------------------------------------------------------------------------ > Virtualization & Cloud Management Using Capacity Planning > Cloud computing makes use of virtualization - but cloud computing > also focuses on allowing computing to be delivered as a service. > http://www.accelacomm.com/jaw/sfnl/114/51521223/ > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > |