Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Jérémie Galarneau]

Hi Stefan,

I'm inlining your patch to make the review easier, see the following remarks.

> From 78696689a46c13cdd2625c59348cbb1c03a32b0e Mon Sep 17 00:00:00 2001
> From: Stefan Seefeld <stefan_seefeld@mentor.com>
> Date: Sat, 27 Jul 2013 01:10:23 -0400
> Subject: [PATCH] Add trace support for calloc and realloc.
>
> Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
> ---
>  liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 68 ++++++++++++++++++++++++++++
>  liblttng-ust-libc-wrapper/ust_libc.h         | 18 ++++++++
>  2 files changed, 86 insertions(+)
>
> diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> index 3212ff0..772663a 100644
> --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> @@ -57,3 +57,71 @@ void free(void *ptr)
>  	tracepoint(ust_libc, free, ptr);
>  	plibc_free(ptr);
>  }
> +
> +/*
> +   The following 'dummy' variables and functions are needed
> +   to break out of an endless recursion:
> +   dlsym itself appears to call calloc, so we need to provide
> +   an implementation for the very first call to calloc to use.
> +*/

Use a "*" on each line of the comment block; see the licence header
for reference.

> +
> +#define DUMMY_HEAP_SIZE 1024
> +static char dummy_heap[DUMMY_HEAP_SIZE] = {0};
> +static int dummy_end_of_heap = 0;

No need to initialize these static variables to zero, it's already
guaranteed by the C standard. These variables and definition should be
placed after the includes and before any function.

> +
> +static void *dummy_malloc(size_t size)
> +{
> +    if(size == 0)
> +	return NULL;
> +
> +    size_t dummy_end_of_heap_reserved = dummy_end_of_heap + size;
> +    if(dummy_end_of_heap_reserved >= DUMMY_HEAP_SIZE)
> +	return NULL;
> +

Applies to the whole patch: put braces around conditional blocks, even
if they only contain one statement. Also, please indent with tabs and
not spaces.

> +    void *loc = (void*) &dummy_heap[dummy_end_of_heap];
> +    dummy_end_of_heap = dummy_end_of_heap_reserved;
> +    return loc;
> +};
> +
> +static void *dummy_calloc(size_t nmemb, size_t size)
> +{
> +  return dummy_malloc(nmemb * size);
> +}
> +
> +void *calloc(size_t nmemb, size_t size)
> +{
> +        static void *(* volatile plibc_calloc)(size_t nmemb, size_t size) = NULL;
> +	void *retval;
> +
> +	if (plibc_calloc == NULL) {
> +	        /* Temporarily redirect to dummy_calloc,
> +		   until the dlsym lookup has completed. */
> +                plibc_calloc = dummy_calloc;

I'm not a fan of this approach although it was recommended on the
libc-help mailing list.
http://sourceware.org/ml/libc-help/2008-11/msg00000.html

Looking at the Glibc code, it appears dlsym() will cause the
allocation of a dl_action_result structure for each calling thread,
which will then be free'd when these threads terminate. I'm afraid
your dummy_calloc() may cause a crash on exit when free() is called on
the pointer to a static buffer it returns.

I may be missing something, but since this structure is allocated only
once per thread, subsequent calls from the same thread to dlsym()
should not result in another call to calloc(). That should make it
possible to call malloc() in dummy_calloc() without causing the
recursion.

Perhaps we could also forego this mechanism completely and initialize
the symbols from a constructor since this library is meant to be
LD_PRELOAD-ed?

Regards,
Jérémie

> +		plibc_calloc = dlsym(RTLD_NEXT, "calloc");
> +		if (plibc_calloc == NULL) {
> +			fprintf(stderr, "callocwrap: unable to find calloc\n");
> +			return NULL;
> +		}
> +	}
> +	retval = plibc_calloc(nmemb, size);
> +	tracepoint(ust_libc, calloc, nmemb, size, retval);
> +	return retval;
> +}
> +
> +void *realloc(void *ptr, size_t size)
> +{
> +        static void *(*plibc_realloc)(void *ptr, size_t size) = NULL;
> +	void *retval;
> +
> +	if (plibc_realloc == NULL) {
> +		plibc_realloc = dlsym(RTLD_NEXT, "realloc");
> +		if (plibc_realloc == NULL) {
> +			fprintf(stderr, "reallocwrap: unable to find realloc\n");
> +			return NULL;
> +		}
> +	}
> +	retval = plibc_realloc(ptr, size);
> +	tracepoint(ust_libc, realloc, ptr, size, retval);
> +	return retval;
> +}
> +
> diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
> index af705aa..6b03a4d 100644
> --- a/liblttng-ust-libc-wrapper/ust_libc.h
> +++ b/liblttng-ust-libc-wrapper/ust_libc.h
> @@ -47,6 +47,24 @@ TRACEPOINT_EVENT(ust_libc, free,
>  	)
>  )
>
> +TRACEPOINT_EVENT(ust_libc, calloc,
> +	TP_ARGS(size_t, nmemb, size_t, size, void *, ptr),
> +	TP_FIELDS(
> +		ctf_integer(size_t, nmemb, nmemb)
> +		ctf_integer(size_t, size, size)
> +		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
> +	)
> +)
> +
> +TRACEPOINT_EVENT(ust_libc, realloc,
> +	TP_ARGS(void *, in_ptr, size_t, size, void *, ptr),
> +	TP_FIELDS(
> +		ctf_integer_hex(unsigned long, in_ptr, (unsigned long) in_ptr)
> +		ctf_integer(size_t, size, size)
> +		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
> +	)
> +)
> +
>  #endif /* _TRACEPOINT_UST_LIBC_H */
>
>  #undef TRACEPOINT_INCLUDE
> --
> 1.8.3.1
>


On Sat, Jul 27, 2013 at 1:19 AM, Stefan Seefeld
<stefan_seefeld@mentor.com> wrote:
> The attached patch adds support for calloc and realloc to the
> lttng-ust-libc-wrapper library.
>
> The implementation for calloc is slightly tricky, as dlsym calls calloc
> internally, so the library needs to provide a 'dummy' implementation for
> calloc to avoid an endless recursion. Testing has shown however that the
> object(s) allocated at that point are small, so a dummy static 'heap'
> suffices as a workaround.
>
> --
> Stefan Seefeld
> CodeSourcery / Mentor Graphics
> http://www.mentor.com/embedded-software/
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

Hi Jérémie,

thanks for the quick review. I agree with the fragility of the calloc()
hack. However, we aren't the only ones hitting this circular dependency
(as google confirms).
Some more investigation reveals that dlsym() is in fact prepared for
calloc() to return NULL (in which case an internal static buffer is
used. And yes, it appears to check for that case during cleanup.)

Thus I have updated the patch, addressing all of the issues you raise,
and replaced the "dummy_malloc" business by a simple dummy_calloc()
returning NULL. This appears to be working fine.

Meanwhile, I'd like to understand your suggestion, so let me follow up
on that, too:

On 07/29/2013 07:19 PM, Jérémie Galarneau wrote:

> I may be missing something, but since this structure is allocated only
> once per thread, subsequent calls from the same thread to dlsym()
> should not result in another call to calloc(). That should make it
> possible to call malloc() in dummy_calloc() without causing the
> recursion.

You are right. But the problem is that, if I call malloc() in
dummy_calloc(), I indirectly end up calling dlsym() from within dlsym()...

> Perhaps we could also forego this mechanism completely and initialize
> the symbols from a constructor since this library is meant to be
> LD_PRELOAD-ed?

I don't quite understand what you are suggesting. dlsym() still calls
calloc(), so our wrapper still needs to check whether initialization has
already been completed, and if not, use a fallback.

The code may be reorganized, but that doesn't seem to make it any simpler.

Regards,
	Stefan

-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 3132 bytes --]

From fd9b458f0cee09cd17c0ff271ae0e2754aceb2fc Mon Sep 17 00:00:00 2001
From: Stefan Seefeld <stefan_seefeld@mentor.com>
Date: Sat, 27 Jul 2013 01:10:23 -0400
Subject: [PATCH] Add trace support for calloc and realloc.

Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
---
 liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 51 +++++++++++++++++++++++++++-
 liblttng-ust-libc-wrapper/ust_libc.h         | 18 ++++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
index 3212ff0..fd60e9d 100644
--- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
+++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
@@ -45,7 +45,7 @@ void *malloc(size_t size)
 
 void free(void *ptr)
 {
-	static void *(*plibc_free)(void *ptr) = NULL;
+	static void (*plibc_free)(void *ptr) = NULL;
 
 	if (plibc_free == NULL) {
 		plibc_free = dlsym(RTLD_NEXT, "free");
@@ -57,3 +57,52 @@ void free(void *ptr)
 	tracepoint(ust_libc, free, ptr);
 	plibc_free(ptr);
 }
+
+static void *dummy_calloc(size_t nmemb, size_t size)
+{
+	/*
+	 * This function is once by dlsym (which calls
+	 * calloc internally). Fortunately it can handle
+	 * a NULL return value.
+	 */
+	return NULL;
+}
+
+void *calloc(size_t nmemb, size_t size)
+{
+	static void *(* volatile plibc_calloc)(size_t nmemb, size_t size) = NULL;
+	void *retval;
+
+	if (plibc_calloc == NULL) {
+		/*
+		 * Temporarily redirect to dummy_calloc,
+		 * until the dlsym lookup has completed.
+		 */
+		plibc_calloc = dummy_calloc;
+		plibc_calloc = dlsym(RTLD_NEXT, "calloc");
+		if (plibc_calloc == NULL) {
+			fprintf(stderr, "callocwrap: unable to find calloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_calloc(nmemb, size);
+	tracepoint(ust_libc, calloc, nmemb, size, retval);
+	return retval;
+}
+
+void *realloc(void *ptr, size_t size)
+{
+	static void *(*plibc_realloc)(void *ptr, size_t size) = NULL;
+	void *retval;
+
+	if (plibc_realloc == NULL) {
+		plibc_realloc = dlsym(RTLD_NEXT, "realloc");
+		if (plibc_realloc == NULL) {
+			fprintf(stderr, "reallocwrap: unable to find realloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_realloc(ptr, size);
+	tracepoint(ust_libc, realloc, ptr, size, retval);
+	return retval;
+}
diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
index af705aa..6b03a4d 100644
--- a/liblttng-ust-libc-wrapper/ust_libc.h
+++ b/liblttng-ust-libc-wrapper/ust_libc.h
@@ -47,6 +47,24 @@ TRACEPOINT_EVENT(ust_libc, free,
 	)
 )
 
+TRACEPOINT_EVENT(ust_libc, calloc,
+	TP_ARGS(size_t, nmemb, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer(size_t, nmemb, nmemb)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+	)
+)
+
+TRACEPOINT_EVENT(ust_libc, realloc,
+	TP_ARGS(void *, in_ptr, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer_hex(unsigned long, in_ptr, (unsigned long) in_ptr)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+	)
+)
+
 #endif /* _TRACEPOINT_UST_LIBC_H */
 
 #undef TRACEPOINT_INCLUDE
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Jérémie Galarneau]

On Tue, Jul 30, 2013 at 5:12 PM, Stefan Seefeld
<stefan_seefeld@mentor.com> wrote:
> Hi Jérémie,
>
> thanks for the quick review. I agree with the fragility of the calloc()
> hack. However, we aren't the only ones hitting this circular dependency
> (as google confirms).
> Some more investigation reveals that dlsym() is in fact prepared for
> calloc() to return NULL (in which case an internal static buffer is
> used. And yes, it appears to check for that case during cleanup.)
>
> Thus I have updated the patch, addressing all of the issues you raise,
> and replaced the "dummy_malloc" business by a simple dummy_calloc()
> returning NULL. This appears to be working fine.
>

I have no doubt it works fine with a simple program, but this comment
in glibc/dlfcn/dlerror.c:143 worries me:

/* ... call to calloc ... */
if (result == NULL) {
/* We are out of memory.  Since this is no really critical
   situation we carry on by using the global variable.
   This might lead to conflicts between the threads but
   they soon all will have memory problems. */

> Meanwhile, I'd like to understand your suggestion, so let me follow up
> on that, too:
>
> On 07/29/2013 07:19 PM, Jérémie Galarneau wrote:
>
>> I may be missing something, but since this structure is allocated only
>> once per thread, subsequent calls from the same thread to dlsym()
>> should not result in another call to calloc(). That should make it
>> possible to call malloc() in dummy_calloc() without causing the
>> recursion.
>
> You are right. But the problem is that, if I call malloc() in
> dummy_calloc(), I indirectly end up calling dlsym() from within dlsym()...
>

You're right. I was thinking __dlerror_run() would have initialized
its dl_action_result structure by the point dlsym() was called a
second time... but it's not the case, hence the recursion... Oops!

>> Perhaps we could also forego this mechanism completely and initialize
>> the symbols from a constructor since this library is meant to be
>> LD_PRELOAD-ed?
>
> I don't quite understand what you are suggesting. dlsym() still calls
> calloc(), so our wrapper still needs to check whether initialization has
> already been completed, and if not, use a fallback.
>
> The code may be reorganized, but that doesn't seem to make it any simpler.
>

Indeed, I was basing this on the same false assumption.

The only other solution I can propose is calling __libc_calloc, which
is exported by glibc, instead of using dlsym to find calloc(). That
somehow appears cleaner to me, but I'm not sure how to handle building
against other implementations of libc...

Anyway, Mathieu will undoubtedly have a strong opinion on this matter ;-)
You may want to wait until he steps-in before continuing to work on this patch.

Regards,
Jérémie

> Regards,
>         Stefan
>
> --
> Stefan Seefeld
> CodeSourcery / Mentor Graphics
> http://www.mentor.com/embedded-software/



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Alexander Monakov]

Hi,

I can offer two more possible approaches:

1.  Use posix_memalign to obtain free()'able memory for calloc without dlsym.
Of course, when you want to override posix_memalign (and all of its synonyms)
as well, you're again in the same trap.

2.  Do not override malloc&co at all, but rather use malloc hooks (see "man
malloc_hooks").

Hope that helps,
Alexander

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

Hi Alexander,

thanks for offering your advice.

On 08/01/2013 12:26 PM, Alexander Monakov wrote:
> Hi,
> 
> I can offer two more possible approaches:
> 
> 1.  Use posix_memalign to obtain free()'able memory for calloc without dlsym.
> Of course, when you want to override posix_memalign (and all of its synonyms)
> as well, you're again in the same trap.

Right. I do indeed expect the list of instrumented functions to grow and
eventually cover the above as well.

> 2.  Do not override malloc&co at all, but rather use malloc hooks (see "man
> malloc_hooks").

These hooks have been deprecated as they are not thread-safe. They might
become if the hooks would be stored in thread-local storage, but that
may incur runtime overhead. In any case, that would require glibc
hacking, which I think is outside the scope of this work.

Thanks,
		Stefan

-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Alexander Monakov]

Okay, one more hack :)

Recurse *once* by calling dlsym inside your calloc, and terminate recursion by
obtaining memory from mmap.

dlsym calls your calloc
  your calloc calls dlsym
    dlsym calls your calloc, again, as it still needs to initialize
      your calloc obtains fresh memory from mmap, clears it and returns
    dlsym stores pointer to mmap'ed buffer, does its thing and returns
  your calloc now has a pointer to the real calloc, uses it, and returns
dlsym stores pointer to calloc'ed buffer, overwriting the pointer to mmap'ed
buffer, completing initialization for the second time and making the
subsequent free() safe.

You can even munmap the mmap'ed buffer in the outer calloc, avoiding leaks,
but you'd have to stash it in a thread-local storage.  You need a TLS flag to
detect recursion in calloc anyhow.

Instead of mmap you can use some other mechanism such as a static buffer, but
still you need to make it big enough, and thread-safe.

Alexander

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Alexander Monakov]

On Thu, 1 Aug 2013, Alexander Monakov wrote:

> Okay, one more hack :)
> 
> Recurse *once* by calling dlsym inside your calloc, and terminate recursion by
> obtaining memory from mmap.

Correction: since there's no guarantee that the very first calloc invocation
will happen from inside dlsym, you have to recurse twice, not once, to be sure
that there are two dlsyms on the stack.

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

On 08/01/2013 01:19 PM, Alexander Monakov wrote:
> 
> 
> On Thu, 1 Aug 2013, Alexander Monakov wrote:
> 
>> Okay, one more hack :)
>>
>> Recurse *once* by calling dlsym inside your calloc, and terminate recursion by
>> obtaining memory from mmap.
> 
> Correction: since there's no guarantee that the very first calloc invocation
> will happen from inside dlsym, you have to recurse twice, not once, to be sure
> that there are two dlsyms on the stack.

I don't think any of this works. The ultimate problem is the one Jérémie
reported initially: dlsym will eventually call free() on the memory
calloc() returned, so my calloc fallback needs to return something that
can actually be freed.

	Stefan

-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Alexander Monakov]

On Thu, Aug 1, 2013 at 9:28 PM, Stefan Seefeld
<stefan_seefeld@mentor.com> wrote:
> I don't think any of this works. The ultimate problem is the one Jérémie
> reported initially: dlsym will eventually call free() on the memory
> calloc() returned, so my calloc fallback needs to return something that
> can actually be freed.

I pointed out how free() will be safe specifically:

>> dlsym stores pointer to calloc'ed buffer, overwriting the pointer to mmap'ed
>> buffer, completing initialization for the second time and making the
>> subsequent free() safe.

Besides, you're intercepting free() in your LD_PRELOAD module as well,
so you can just catch attempts to free your mmap'ed buffers there and
munmap them safely.

Alexander

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Mathieu Desnoyers]

* Alexander Monakov (amonakov@ispras.ru) wrote:
> On Thu, Aug 1, 2013 at 9:28 PM, Stefan Seefeld
> <stefan_seefeld@mentor.com> wrote:
> > I don't think any of this works. The ultimate problem is the one Jérémie
> > reported initially: dlsym will eventually call free() on the memory
> > calloc() returned, so my calloc fallback needs to return something that
> > can actually be freed.
> 
> I pointed out how free() will be safe specifically:
> 
> >> dlsym stores pointer to calloc'ed buffer, overwriting the pointer to mmap'ed
> >> buffer, completing initialization for the second time and making the
> >> subsequent free() safe.
> 
> Besides, you're intercepting free() in your LD_PRELOAD module as well,
> so you can just catch attempts to free your mmap'ed buffers there and
> munmap them safely.

Hello!

I'm now back from vacation :)

Please have a look at my implementation of quick and dirty malloc/free
tracker here:

https://github.com/efficios/memleak-finder

file: memleak-finder.c

Implementing a work-around for calloc vs dlsym was indeed tricky. The
solution I got there works fine so far (static array used for this
special-case). I think we might get the free() case to be more solid by
checking if the address being freed is in the static buffer range.

If you could submit a patch that improves memleak-finder.c to get this
right, it would be very much appreciated.

Then, I recommend porting the code from memleak-finder.c to UST
liblttng-ust-libc-wrapper. The approach I used in memleak-finder.c seems
to work quite well, and is clearly more complete that what we find in
liblttng-ust-libc-wrapper currently.

Thanks!

Mathieu 

> 
> Alexander
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

[-- Attachment #1: Type: text/plain, Size: 2066 bytes --]

On 08/02/2013 09:18 PM, Mathieu Desnoyers wrote:
> Please have a look at my implementation of quick and dirty malloc/free
> tracker here:
>
> https://github.com/efficios/memleak-finder
>
> file: memleak-finder.c
>
> Implementing a work-around for calloc vs dlsym was indeed tricky. The
> solution I got there works fine so far (static array used for this
> special-case). I think we might get the free() case to be more solid by
> checking if the address being freed is in the static buffer range.

I played a little with this code (e.g. by removing the "constructor"
attribute from the do_init() function, to increase the chances that I
could see static_calloc() being used in my own code).
I found one bug (where the static buffer size adjustment was wrong, so
the second time static_calloc was called a pointer pointing into the
previous buffer would be returned).
And I did observe a crash without the appropriate adjustment to free().

> If you could submit a patch that improves memleak-finder.c to get this
> right, it would be very much appreciated.

Please find attached a small patch for this. As you can see, if the
pointer falls into the range of the static buffer, free() does nothing.
("freeing" the buffer properly would require much more work,
and arguably falls outside the scope of what is needed here, since
static_calloc() is really only required during startup, and thus should
never run out of space.


> Then, I recommend porting the code from memleak-finder.c to UST
> liblttng-ust-libc-wrapper. The approach I used in memleak-finder.c seems
> to work quite well, and is clearly more complete that what we find in
> liblttng-ust-libc-wrapper currently.

Yes, with the adjusted free() implementation as per this patch, I think
this is a robust solution. Do you think we need any form of multi-thread
protection ? Or can we assume that library initialization will have
completed by the time multiple threads could possibly call calloc() ?

    Stefan

-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/


[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 1178 bytes --]

From 858ae5805f3bea303edfcec8156494be2fa3580d Mon Sep 17 00:00:00 2001
From: Stefan Seefeld <stefan_seefeld@mentor.com>
Date: Mon, 5 Aug 2013 10:54:24 -0400
Subject: [PATCH] Fix typo and robustify free() implementation.

Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
---
 memleak-finder.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/memleak-finder.c b/memleak-finder.c
index 2f8b996..d3755ff 100644
--- a/memleak-finder.c
+++ b/memleak-finder.c
@@ -160,7 +160,7 @@ void *static_calloc(size_t nmemb, size_t size)
 	if (nmemb * size > sizeof(static_calloc_buf) - static_calloc_len)
 		return NULL;
 	prev_len = static_calloc_len;
-	static_calloc_len += nmemb + size;
+	static_calloc_len += nmemb * size;
 	return &static_calloc_buf[prev_len];
 }
 
@@ -341,6 +341,15 @@ free(void *ptr)
 {
 	const void *caller = __builtin_return_address(0);
 
+	/* Check whether the memory was allocated with
+	 * static_calloc, in which case there is nothing
+	 * to free.
+	 */
+	if ((char*)ptr >= static_calloc_buf &&
+	    (char*)ptr < static_calloc_buf + STATIC_CALLOC_LEN) {
+		return;
+	}
+
 	do_init();
 
 	if (thread_in_hook) {
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On 08/02/2013 09:18 PM, Mathieu Desnoyers wrote:

[...]

> Then, I recommend porting the code from memleak-finder.c to UST
> liblttng-ust-libc-wrapper. The approach I used in memleak-finder.c seems
> to work quite well, and is clearly more complete that what we find in
> liblttng-ust-libc-wrapper currently.

...and here is a revised patch to add calloc and realloc
instrumentation, using the same technique as memleak-finder.

I've ran the patch through the checkpatch.pl script, and made some minor
adjustments to other code to fix coding style errors.

Regards,
    Stefan

-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/


[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 3829 bytes --]

From 93da5d2be5ec3a3680449e186780ff7a08cec6d9 Mon Sep 17 00:00:00 2001
From: Stefan Seefeld <stefan_seefeld@mentor.com>
Date: Sat, 27 Jul 2013 01:10:23 -0400
Subject: [PATCH] Add trace support for calloc and realloc.

Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
---
 liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 68 +++++++++++++++++++++++++++-
 liblttng-ust-libc-wrapper/ust_libc.h         | 18 ++++++++
 2 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
index 3212ff0..81beb78 100644
--- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
+++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
@@ -26,9 +26,25 @@
 #define TRACEPOINT_CREATE_PROBES
 #include "ust_libc.h"
 
+#define STATIC_CALLOC_LEN 4096
+static char static_calloc_buf[STATIC_CALLOC_LEN];
+static size_t static_calloc_buf_offset;
+
+static void *static_calloc(size_t nmemb, size_t size)
+{
+	size_t prev_offset;
+
+	if (nmemb * size > sizeof(static_calloc_buf) - static_calloc_buf_offset) {
+		return NULL;
+	}
+	prev_offset = static_calloc_buf_offset;
+	static_calloc_buf_offset += nmemb * size;
+	return &static_calloc_buf[prev_offset];
+}
+
 void *malloc(size_t size)
 {
-	static void *(*plibc_malloc)(size_t size) = NULL;
+	static void *(*plibc_malloc)(size_t size);
 	void *retval;
 
 	if (plibc_malloc == NULL) {
@@ -45,7 +61,16 @@ void *malloc(size_t size)
 
 void free(void *ptr)
 {
-	static void *(*plibc_free)(void *ptr) = NULL;
+	static void (*plibc_free)(void *ptr);
+
+	/* Check whether the memory was allocated with
+	 * static_calloc, in which case there is nothing
+	 * to free.
+	 */
+	if ((char *)ptr >= static_calloc_buf &&
+	    (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN) {
+		return;
+	}
 
 	if (plibc_free == NULL) {
 		plibc_free = dlsym(RTLD_NEXT, "free");
@@ -57,3 +82,42 @@ void free(void *ptr)
 	tracepoint(ust_libc, free, ptr);
 	plibc_free(ptr);
 }
+
+void *calloc(size_t nmemb, size_t size)
+{
+	static void *(*volatile plibc_calloc)(size_t nmemb, size_t size);
+	void *retval;
+
+	if (plibc_calloc == NULL) {
+		/*
+		 * Temporarily redirect to static_calloc,
+		 * until the dlsym lookup has completed.
+		 */
+		plibc_calloc = static_calloc;
+		plibc_calloc = dlsym(RTLD_NEXT, "calloc");
+		if (plibc_calloc == NULL) {
+			fprintf(stderr, "callocwrap: unable to find calloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_calloc(nmemb, size);
+	tracepoint(ust_libc, calloc, nmemb, size, retval);
+	return retval;
+}
+
+void *realloc(void *ptr, size_t size)
+{
+	static void *(*plibc_realloc)(void *ptr, size_t size);
+	void *retval;
+
+	if (plibc_realloc == NULL) {
+		plibc_realloc = dlsym(RTLD_NEXT, "realloc");
+		if (plibc_realloc == NULL) {
+			fprintf(stderr, "reallocwrap: unable to find realloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_realloc(ptr, size);
+	tracepoint(ust_libc, realloc, ptr, size, retval);
+	return retval;
+}
diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
index af705aa..6b03a4d 100644
--- a/liblttng-ust-libc-wrapper/ust_libc.h
+++ b/liblttng-ust-libc-wrapper/ust_libc.h
@@ -47,6 +47,24 @@ TRACEPOINT_EVENT(ust_libc, free,
 	)
 )
 
+TRACEPOINT_EVENT(ust_libc, calloc,
+	TP_ARGS(size_t, nmemb, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer(size_t, nmemb, nmemb)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+	)
+)
+
+TRACEPOINT_EVENT(ust_libc, realloc,
+	TP_ARGS(void *, in_ptr, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer_hex(unsigned long, in_ptr, (unsigned long) in_ptr)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+	)
+)
+
 #endif /* _TRACEPOINT_UST_LIBC_H */
 
 #undef TRACEPOINT_INCLUDE
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Mathieu Desnoyers]

* Stefan Seefeld (stefan_seefeld@mentor.com) wrote:
> On 08/02/2013 09:18 PM, Mathieu Desnoyers wrote:
> > Please have a look at my implementation of quick and dirty malloc/free
> > tracker here:
> >
> > https://github.com/efficios/memleak-finder
> >
> > file: memleak-finder.c
> >
> > Implementing a work-around for calloc vs dlsym was indeed tricky. The
> > solution I got there works fine so far (static array used for this
> > special-case). I think we might get the free() case to be more solid by
> > checking if the address being freed is in the static buffer range.
> 
> I played a little with this code (e.g. by removing the "constructor"
> attribute from the do_init() function, to increase the chances that I
> could see static_calloc() being used in my own code).
> I found one bug (where the static buffer size adjustment was wrong, so
> the second time static_calloc was called a pointer pointing into the
> previous buffer would be returned).

Good catch. I'm merging this part.

> And I did observe a crash without the appropriate adjustment to free().
> 
> > If you could submit a patch that improves memleak-finder.c to get this
> > right, it would be very much appreciated.
> 
> Please find attached a small patch for this. As you can see, if the
> pointer falls into the range of the static buffer, free() does nothing.
> ("freeing" the buffer properly would require much more work,
> and arguably falls outside the scope of what is needed here, since
> static_calloc() is really only required during startup, and thus should
> never run out of space.

Yep, agreed.

I already did a commit a few days ago before seeing your patch here.
It's:

commit 07b2e0cd6297e5c7349793f5bfc9cc28d6c83a6b
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Mon Aug 5 13:42:29 2013 -0400

    Handle realloc/free of static_calloc
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

It's not perfect, but if people are reallocating memory allocated that
early, they will get a NULL pointer exception and we'll deal with it.

> 
> 
> > Then, I recommend porting the code from memleak-finder.c to UST
> > liblttng-ust-libc-wrapper. The approach I used in memleak-finder.c seems
> > to work quite well, and is clearly more complete that what we find in
> > liblttng-ust-libc-wrapper currently.
> 
> Yes, with the adjusted free() implementation as per this patch, I think
> this is a robust solution. Do you think we need any form of multi-thread
> protection ? Or can we assume that library initialization will have
> completed by the time multiple threads could possibly call calloc() ?

Well, considering that lttng-ust spawns thread in its constructor, it
might indeed be a good thing to be thread-safe. Fixed by:

commit d9e6d9a57fda6d3ad73016b2b3137997aac9c7ba
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Tue Aug 6 21:41:15 2013 -0400

    Fix: static_calloc should be thread-safe
    
    Reported-by: Stefan Seefeld <stefan_seefeld@mentor.com>
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thoughts ?

Thanks,

Mathieu

> 
>     Stefan
> 
> -- 
> Stefan Seefeld
> CodeSourcery / Mentor Graphics
> http://www.mentor.com/embedded-software/
> 

> From 858ae5805f3bea303edfcec8156494be2fa3580d Mon Sep 17 00:00:00 2001
> From: Stefan Seefeld <stefan_seefeld@mentor.com>
> Date: Mon, 5 Aug 2013 10:54:24 -0400
> Subject: [PATCH] Fix typo and robustify free() implementation.
> 
> Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
> ---
>  memleak-finder.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/memleak-finder.c b/memleak-finder.c
> index 2f8b996..d3755ff 100644
> --- a/memleak-finder.c
> +++ b/memleak-finder.c
> @@ -160,7 +160,7 @@ void *static_calloc(size_t nmemb, size_t size)
>  	if (nmemb * size > sizeof(static_calloc_buf) - static_calloc_len)
>  		return NULL;
>  	prev_len = static_calloc_len;
> -	static_calloc_len += nmemb + size;
> +	static_calloc_len += nmemb * size;
>  	return &static_calloc_buf[prev_len];
>  }
>  
> @@ -341,6 +341,15 @@ free(void *ptr)
>  {
>  	const void *caller = __builtin_return_address(0);
>  
> +	/* Check whether the memory was allocated with
> +	 * static_calloc, in which case there is nothing
> +	 * to free.
> +	 */
> +	if ((char*)ptr >= static_calloc_buf &&
> +	    (char*)ptr < static_calloc_buf + STATIC_CALLOC_LEN) {
> +		return;
> +	}
> +
>  	do_init();
>  
>  	if (thread_in_hook) {
> -- 
> 1.8.3.1
> 

> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

On 08/06/2013 09:42 PM, Mathieu Desnoyers wrote:
> * Stefan Seefeld (stefan_seefeld@mentor.com) wrote:
>> Yes, with the adjusted free() implementation as per this patch, I think
>> this is a robust solution. Do you think we need any form of multi-thread
>> protection ? Or can we assume that library initialization will have
>> completed by the time multiple threads could possibly call calloc() ?
> Well, considering that lttng-ust spawns thread in its constructor, it
> might indeed be a good thing to be thread-safe. Fixed by:
>
> commit d9e6d9a57fda6d3ad73016b2b3137997aac9c7ba
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date:   Tue Aug 6 21:41:15 2013 -0400
>
>     Fix: static_calloc should be thread-safe
>     
>     Reported-by: Stefan Seefeld <stefan_seefeld@mentor.com>
>     Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>
> Thoughts ?

Sorry, I'm confused. Where can I find this commit, i.e. what repo, what
branch ? I can't see it anywhere ?

Thanks,
        Stefan

-- 

      ...ich hab' noch einen Koffer in Berlin...

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Mathieu Desnoyers]

* Stefan Seefeld (stefan@seefeld.name) wrote:
> On 08/06/2013 09:42 PM, Mathieu Desnoyers wrote:
> > * Stefan Seefeld (stefan_seefeld@mentor.com) wrote:
> >> Yes, with the adjusted free() implementation as per this patch, I think
> >> this is a robust solution. Do you think we need any form of multi-thread
> >> protection ? Or can we assume that library initialization will have
> >> completed by the time multiple threads could possibly call calloc() ?
> > Well, considering that lttng-ust spawns thread in its constructor, it
> > might indeed be a good thing to be thread-safe. Fixed by:
> >
> > commit d9e6d9a57fda6d3ad73016b2b3137997aac9c7ba
> > Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Date:   Tue Aug 6 21:41:15 2013 -0400
> >
> >     Fix: static_calloc should be thread-safe
> >     
> >     Reported-by: Stefan Seefeld <stefan_seefeld@mentor.com>
> >     Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >
> > Thoughts ?
> 
> Sorry, I'm confused. Where can I find this commit, i.e. what repo, what
> branch ? I can't see it anywhere ?

See

https://github.com/efficios/memleak-finder/commits/master

Thanks,

Mathieu

> 
> Thanks,
>         Stefan
> 
> -- 
> 
>       ...ich hab' noch einen Koffer in Berlin...
> 
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

On 08/07/2013 09:57 AM, Mathieu Desnoyers wrote:
> * Stefan Seefeld (stefan@seefeld.name) wrote:
>>
>> Sorry, I'm confused. Where can I find this commit, i.e. what repo, what
>> branch ? I can't see it anywhere ?
> See
>
> https://github.com/efficios/memleak-finder/commits/master

Oh, sorry, I had been looking in the lttng repo. I see it now.
Yes, that looks good. I'll revise my malloc patch accordingly.

    Stefan

-- 

      ...ich hab' noch einen Koffer in Berlin...

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

...and another revision of the previous patch, now with mutex-protected
static_calloc.
For consistency I have also adjusted the ust_libc.h header to use 'void
*' instead of 'unsigned long' in its tracepoint declaration.

    Stefan


-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/


[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 4384 bytes --]

From abbfdd44590876427a3fac463e8743afa7ceb149 Mon Sep 17 00:00:00 2001
From: Stefan Seefeld <stefan_seefeld@mentor.com>
Date: Sat, 27 Jul 2013 01:10:23 -0400
Subject: [PATCH] Add trace support for calloc and realloc.

Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
---
 liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 73 +++++++++++++++++++++++++++-
 liblttng-ust-libc-wrapper/ust_libc.h         | 22 ++++++++-
 2 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
index 3212ff0..c7f747b 100644
--- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
+++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
@@ -21,14 +21,35 @@
 #include <dlfcn.h>
 #include <sys/types.h>
 #include <stdio.h>
+#include <pthread.h>
 
 #define TRACEPOINT_DEFINE
 #define TRACEPOINT_CREATE_PROBES
 #include "ust_libc.h"
 
+#define STATIC_CALLOC_LEN 4096
+static char static_calloc_buf[STATIC_CALLOC_LEN];
+static size_t static_calloc_buf_offset;
+static pthread_mutex_t static_calloc_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+static void *static_calloc(size_t nmemb, size_t size)
+{
+	size_t prev_offset;
+
+	pthread_mutex_lock(&static_calloc_mutex);
+	if (nmemb * size > sizeof(static_calloc_buf) - static_calloc_buf_offset) {
+		pthread_mutex_unlock(&static_calloc_mutex);
+		return NULL;
+	}
+	prev_offset = static_calloc_buf_offset;
+	static_calloc_buf_offset += nmemb * size;
+	pthread_mutex_unlock(&static_calloc_mutex);
+	return &static_calloc_buf[prev_offset];
+}
+
 void *malloc(size_t size)
 {
-	static void *(*plibc_malloc)(size_t size) = NULL;
+	static void *(*plibc_malloc)(size_t size);
 	void *retval;
 
 	if (plibc_malloc == NULL) {
@@ -45,7 +66,16 @@ void *malloc(size_t size)
 
 void free(void *ptr)
 {
-	static void *(*plibc_free)(void *ptr) = NULL;
+	static void (*plibc_free)(void *ptr);
+
+	/* Check whether the memory was allocated with
+	 * static_calloc, in which case there is nothing
+	 * to free.
+	 */
+	if ((char *)ptr >= static_calloc_buf &&
+	    (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN) {
+		return;
+	}
 
 	if (plibc_free == NULL) {
 		plibc_free = dlsym(RTLD_NEXT, "free");
@@ -57,3 +87,42 @@ void free(void *ptr)
 	tracepoint(ust_libc, free, ptr);
 	plibc_free(ptr);
 }
+
+void *calloc(size_t nmemb, size_t size)
+{
+	static void *(*volatile plibc_calloc)(size_t nmemb, size_t size);
+	void *retval;
+
+	if (plibc_calloc == NULL) {
+		/*
+		 * Temporarily redirect to static_calloc,
+		 * until the dlsym lookup has completed.
+		 */
+		plibc_calloc = static_calloc;
+		plibc_calloc = dlsym(RTLD_NEXT, "calloc");
+		if (plibc_calloc == NULL) {
+			fprintf(stderr, "callocwrap: unable to find calloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_calloc(nmemb, size);
+	tracepoint(ust_libc, calloc, nmemb, size, retval);
+	return retval;
+}
+
+void *realloc(void *ptr, size_t size)
+{
+	static void *(*plibc_realloc)(void *ptr, size_t size);
+	void *retval;
+
+	if (plibc_realloc == NULL) {
+		plibc_realloc = dlsym(RTLD_NEXT, "realloc");
+		if (plibc_realloc == NULL) {
+			fprintf(stderr, "reallocwrap: unable to find realloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_realloc(ptr, size);
+	tracepoint(ust_libc, realloc, ptr, size, retval);
+	return retval;
+}
diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
index af705aa..d2096a3 100644
--- a/liblttng-ust-libc-wrapper/ust_libc.h
+++ b/liblttng-ust-libc-wrapper/ust_libc.h
@@ -36,14 +36,32 @@ TRACEPOINT_EVENT(ust_libc, malloc,
 	TP_ARGS(size_t, size, void *, ptr),
 	TP_FIELDS(
 		ctf_integer(size_t, size, size)
-		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+		ctf_integer_hex(void *, ptr, ptr)
 	)
 )
 
 TRACEPOINT_EVENT(ust_libc, free,
 	TP_ARGS(void *, ptr),
 	TP_FIELDS(
-		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+		ctf_integer_hex(void *, ptr, ptr)
+	)
+)
+
+TRACEPOINT_EVENT(ust_libc, calloc,
+	TP_ARGS(size_t, nmemb, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer(size_t, nmemb, nmemb)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(void *, ptr, ptr)
+	)
+)
+
+TRACEPOINT_EVENT(ust_libc, realloc,
+	TP_ARGS(void *, in_ptr, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer_hex(void *, in_ptr, in_ptr)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(void *, ptr, ptr)
 	)
 )
 
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Mathieu Desnoyers]

* Stefan Seefeld (stefan_seefeld@mentor.com) wrote:
> ...and another revision of the previous patch, now with mutex-protected
> static_calloc.
> For consistency I have also adjusted the ust_libc.h header to use 'void
> *' instead of 'unsigned long' in its tracepoint declaration.

merged.

I also pushed the following on top:

commit 4c3536e01190b118e0889ebb991b0eea6f98260e
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Wed Aug 7 11:48:50 2013 -0400

    malloc instrumentation: remove dependency on pthread
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

I notice that memalign and posix_memalign are missing. Do you have plans
to add them ?

Thanks,

Mathieu

> 
>     Stefan
> 
> 
> -- 
> Stefan Seefeld
> CodeSourcery / Mentor Graphics
> http://www.mentor.com/embedded-software/
> 

> From abbfdd44590876427a3fac463e8743afa7ceb149 Mon Sep 17 00:00:00 2001
> From: Stefan Seefeld <stefan_seefeld@mentor.com>
> Date: Sat, 27 Jul 2013 01:10:23 -0400
> Subject: [PATCH] Add trace support for calloc and realloc.
> 
> Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
> ---
>  liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 73 +++++++++++++++++++++++++++-
>  liblttng-ust-libc-wrapper/ust_libc.h         | 22 ++++++++-
>  2 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> index 3212ff0..c7f747b 100644
> --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> @@ -21,14 +21,35 @@
>  #include <dlfcn.h>
>  #include <sys/types.h>
>  #include <stdio.h>
> +#include <pthread.h>
>  
>  #define TRACEPOINT_DEFINE
>  #define TRACEPOINT_CREATE_PROBES
>  #include "ust_libc.h"
>  
> +#define STATIC_CALLOC_LEN 4096
> +static char static_calloc_buf[STATIC_CALLOC_LEN];
> +static size_t static_calloc_buf_offset;
> +static pthread_mutex_t static_calloc_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
> +static void *static_calloc(size_t nmemb, size_t size)
> +{
> +	size_t prev_offset;
> +
> +	pthread_mutex_lock(&static_calloc_mutex);
> +	if (nmemb * size > sizeof(static_calloc_buf) - static_calloc_buf_offset) {
> +		pthread_mutex_unlock(&static_calloc_mutex);
> +		return NULL;
> +	}
> +	prev_offset = static_calloc_buf_offset;
> +	static_calloc_buf_offset += nmemb * size;
> +	pthread_mutex_unlock(&static_calloc_mutex);
> +	return &static_calloc_buf[prev_offset];
> +}
> +
>  void *malloc(size_t size)
>  {
> -	static void *(*plibc_malloc)(size_t size) = NULL;
> +	static void *(*plibc_malloc)(size_t size);
>  	void *retval;
>  
>  	if (plibc_malloc == NULL) {
> @@ -45,7 +66,16 @@ void *malloc(size_t size)
>  
>  void free(void *ptr)
>  {
> -	static void *(*plibc_free)(void *ptr) = NULL;
> +	static void (*plibc_free)(void *ptr);
> +
> +	/* Check whether the memory was allocated with
> +	 * static_calloc, in which case there is nothing
> +	 * to free.
> +	 */
> +	if ((char *)ptr >= static_calloc_buf &&
> +	    (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN) {
> +		return;
> +	}
>  
>  	if (plibc_free == NULL) {
>  		plibc_free = dlsym(RTLD_NEXT, "free");
> @@ -57,3 +87,42 @@ void free(void *ptr)
>  	tracepoint(ust_libc, free, ptr);
>  	plibc_free(ptr);
>  }
> +
> +void *calloc(size_t nmemb, size_t size)
> +{
> +	static void *(*volatile plibc_calloc)(size_t nmemb, size_t size);
> +	void *retval;
> +
> +	if (plibc_calloc == NULL) {
> +		/*
> +		 * Temporarily redirect to static_calloc,
> +		 * until the dlsym lookup has completed.
> +		 */
> +		plibc_calloc = static_calloc;
> +		plibc_calloc = dlsym(RTLD_NEXT, "calloc");
> +		if (plibc_calloc == NULL) {
> +			fprintf(stderr, "callocwrap: unable to find calloc\n");
> +			return NULL;
> +		}
> +	}
> +	retval = plibc_calloc(nmemb, size);
> +	tracepoint(ust_libc, calloc, nmemb, size, retval);
> +	return retval;
> +}
> +
> +void *realloc(void *ptr, size_t size)
> +{
> +	static void *(*plibc_realloc)(void *ptr, size_t size);
> +	void *retval;
> +
> +	if (plibc_realloc == NULL) {
> +		plibc_realloc = dlsym(RTLD_NEXT, "realloc");
> +		if (plibc_realloc == NULL) {
> +			fprintf(stderr, "reallocwrap: unable to find realloc\n");
> +			return NULL;
> +		}
> +	}
> +	retval = plibc_realloc(ptr, size);
> +	tracepoint(ust_libc, realloc, ptr, size, retval);
> +	return retval;
> +}
> diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
> index af705aa..d2096a3 100644
> --- a/liblttng-ust-libc-wrapper/ust_libc.h
> +++ b/liblttng-ust-libc-wrapper/ust_libc.h
> @@ -36,14 +36,32 @@ TRACEPOINT_EVENT(ust_libc, malloc,
>  	TP_ARGS(size_t, size, void *, ptr),
>  	TP_FIELDS(
>  		ctf_integer(size_t, size, size)
> -		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
> +		ctf_integer_hex(void *, ptr, ptr)
>  	)
>  )
>  
>  TRACEPOINT_EVENT(ust_libc, free,
>  	TP_ARGS(void *, ptr),
>  	TP_FIELDS(
> -		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
> +		ctf_integer_hex(void *, ptr, ptr)
> +	)
> +)
> +
> +TRACEPOINT_EVENT(ust_libc, calloc,
> +	TP_ARGS(size_t, nmemb, size_t, size, void *, ptr),
> +	TP_FIELDS(
> +		ctf_integer(size_t, nmemb, nmemb)
> +		ctf_integer(size_t, size, size)
> +		ctf_integer_hex(void *, ptr, ptr)
> +	)
> +)
> +
> +TRACEPOINT_EVENT(ust_libc, realloc,
> +	TP_ARGS(void *, in_ptr, size_t, size, void *, ptr),
> +	TP_FIELDS(
> +		ctf_integer_hex(void *, in_ptr, in_ptr)
> +		ctf_integer(size_t, size, size)
> +		ctf_integer_hex(void *, ptr, ptr)
>  	)
>  )
>  
> -- 
> 1.8.3.1
> 

> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

On 08/07/2013 11:50 AM, Mathieu Desnoyers wrote:
> * Stefan Seefeld (stefan_seefeld@mentor.com) wrote:
>> ...and another revision of the previous patch, now with mutex-protected
>> static_calloc.
>> For consistency I have also adjusted the ust_libc.h header to use 'void
>> *' instead of 'unsigned long' in its tracepoint declaration.
> merged.

Thanks !

> I also pushed the following on top:
>
> commit 4c3536e01190b118e0889ebb991b0eea6f98260e
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date:   Wed Aug 7 11:48:50 2013 -0400
>
>     malloc instrumentation: remove dependency on pthread
>     
>     Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>
> I notice that memalign and posix_memalign are missing. Do you have plans
> to add them ?

Yes, I can certainly do that.

    Stefan



-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

On 08/07/2013 11:50 AM, Mathieu Desnoyers wrote:

> I notice that memalign and posix_memalign are missing. Do you have plans
> to add them ?

Here is a patch adding those two.

	Stefan


-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 2751 bytes --]

From 10b4f91ea710b484a7e31fca68f55a2b58d026cd Mon Sep 17 00:00:00 2001
From: Stefan Seefeld <stefan_seefeld@mentor.com>
Date: Wed, 7 Aug 2013 13:05:34 -0400
Subject: [PATCH] Add trace support for memalign and posix_memalign.

Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
---
 liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 34 ++++++++++++++++++++++++++++
 liblttng-ust-libc-wrapper/ust_libc.h         | 19 ++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
index 4fe8fa9..33ed18b 100644
--- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
+++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
@@ -133,3 +133,37 @@ void *realloc(void *ptr, size_t size)
 	tracepoint(ust_libc, realloc, ptr, size, retval);
 	return retval;
 }
+
+void *memalign(size_t alignment, size_t size)
+{
+	static void *(*plibc_memalign)(size_t alignment, size_t size);
+	void *retval;
+
+	if (plibc_memalign == NULL) {
+		plibc_memalign = dlsym(RTLD_NEXT, "memalign");
+		if (plibc_memalign == NULL) {
+			fprintf(stderr, "memalignwrap: unable to find memalign\n");
+			return NULL;
+		}
+	}
+	retval = plibc_memalign(alignment, size);
+	tracepoint(ust_libc, memalign, alignment, size, retval);
+	return retval;
+}
+
+int posix_memalign(void **memptr, size_t alignment, size_t size)
+{
+	static int(*plibc_posix_memalign)(void **memptr, size_t alignment, size_t size);
+	int retval;
+
+	if (plibc_posix_memalign == NULL) {
+		plibc_posix_memalign = dlsym(RTLD_NEXT, "posix_memalign");
+		if (plibc_posix_memalign == NULL) {
+			fprintf(stderr, "posix_memalignwrap: unable to find posix_memalign\n");
+			return ENOMEM;
+		}
+	}
+	retval = plibc_posix_memalign(memptr, alignment, size);
+	tracepoint(ust_libc, posix_memalign, *memptr, alignment, size, retval);
+	return retval;
+}
diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
index d2096a3..a8ff9c6 100644
--- a/liblttng-ust-libc-wrapper/ust_libc.h
+++ b/liblttng-ust-libc-wrapper/ust_libc.h
@@ -65,6 +65,25 @@ TRACEPOINT_EVENT(ust_libc, realloc,
 	)
 )
 
+TRACEPOINT_EVENT(ust_libc, memalign,
+	TP_ARGS(size_t, alignment, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer(size_t, alignment, alignment)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(void *, ptr, ptr)
+	)
+)
+
+TRACEPOINT_EVENT(ust_libc, posix_memalign,
+	TP_ARGS(void *, out_ptr, size_t, alignment, size_t, size, int, result),
+	TP_FIELDS(
+		ctf_integer_hex(void *, out_ptr, out_ptr)
+		ctf_integer(size_t, alignment, alignment)
+		ctf_integer(size_t, size, size)
+		ctf_integer(int, result, result)
+	)
+)
+
 #endif /* _TRACEPOINT_UST_LIBC_H */
 
 #undef TRACEPOINT_INCLUDE
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-ust] Add trace support for calloc and realloc.

[Mathieu Desnoyers]

* Stefan Seefeld (stefan_seefeld@mentor.com) wrote:
> On 08/07/2013 11:50 AM, Mathieu Desnoyers wrote:
> 
> > I notice that memalign and posix_memalign are missing. Do you have plans
> > to add them ?
> 
> Here is a patch adding those two.

merged, thanks!

By the way, we usually don't put "dots" at the end of the patch title.
No biggie, I removed it from the changelog.

Thanks,

Mathieu

> 
> 	Stefan
> 
> 
> -- 
> Stefan Seefeld
> CodeSourcery / Mentor Graphics
> http://www.mentor.com/embedded-software/

> From 10b4f91ea710b484a7e31fca68f55a2b58d026cd Mon Sep 17 00:00:00 2001
> From: Stefan Seefeld <stefan_seefeld@mentor.com>
> Date: Wed, 7 Aug 2013 13:05:34 -0400
> Subject: [PATCH] Add trace support for memalign and posix_memalign.
> 
> Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
> ---
>  liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 34 ++++++++++++++++++++++++++++
>  liblttng-ust-libc-wrapper/ust_libc.h         | 19 ++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> index 4fe8fa9..33ed18b 100644
> --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
> @@ -133,3 +133,37 @@ void *realloc(void *ptr, size_t size)
>  	tracepoint(ust_libc, realloc, ptr, size, retval);
>  	return retval;
>  }
> +
> +void *memalign(size_t alignment, size_t size)
> +{
> +	static void *(*plibc_memalign)(size_t alignment, size_t size);
> +	void *retval;
> +
> +	if (plibc_memalign == NULL) {
> +		plibc_memalign = dlsym(RTLD_NEXT, "memalign");
> +		if (plibc_memalign == NULL) {
> +			fprintf(stderr, "memalignwrap: unable to find memalign\n");
> +			return NULL;
> +		}
> +	}
> +	retval = plibc_memalign(alignment, size);
> +	tracepoint(ust_libc, memalign, alignment, size, retval);
> +	return retval;
> +}
> +
> +int posix_memalign(void **memptr, size_t alignment, size_t size)
> +{
> +	static int(*plibc_posix_memalign)(void **memptr, size_t alignment, size_t size);
> +	int retval;
> +
> +	if (plibc_posix_memalign == NULL) {
> +		plibc_posix_memalign = dlsym(RTLD_NEXT, "posix_memalign");
> +		if (plibc_posix_memalign == NULL) {
> +			fprintf(stderr, "posix_memalignwrap: unable to find posix_memalign\n");
> +			return ENOMEM;
> +		}
> +	}
> +	retval = plibc_posix_memalign(memptr, alignment, size);
> +	tracepoint(ust_libc, posix_memalign, *memptr, alignment, size, retval);
> +	return retval;
> +}
> diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
> index d2096a3..a8ff9c6 100644
> --- a/liblttng-ust-libc-wrapper/ust_libc.h
> +++ b/liblttng-ust-libc-wrapper/ust_libc.h
> @@ -65,6 +65,25 @@ TRACEPOINT_EVENT(ust_libc, realloc,
>  	)
>  )
>  
> +TRACEPOINT_EVENT(ust_libc, memalign,
> +	TP_ARGS(size_t, alignment, size_t, size, void *, ptr),
> +	TP_FIELDS(
> +		ctf_integer(size_t, alignment, alignment)
> +		ctf_integer(size_t, size, size)
> +		ctf_integer_hex(void *, ptr, ptr)
> +	)
> +)
> +
> +TRACEPOINT_EVENT(ust_libc, posix_memalign,
> +	TP_ARGS(void *, out_ptr, size_t, alignment, size_t, size, int, result),
> +	TP_FIELDS(
> +		ctf_integer_hex(void *, out_ptr, out_ptr)
> +		ctf_integer(size_t, alignment, alignment)
> +		ctf_integer(size_t, size, size)
> +		ctf_integer(int, result, result)
> +	)
> +)
> +
>  #endif /* _TRACEPOINT_UST_LIBC_H */
>  
>  #undef TRACEPOINT_INCLUDE
> -- 
> 1.8.3.1
> 


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[PATCH lttng-ust] Add trace support for calloc and realloc.

[Stefan Seefeld]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

The attached patch adds support for calloc and realloc to the
lttng-ust-libc-wrapper library.

The implementation for calloc is slightly tricky, as dlsym calls calloc
internally, so the library needs to provide a 'dummy' implementation for
calloc to avoid an endless recursion. Testing has shown however that the
object(s) allocated at that point are small, so a dummy static 'heap'
suffices as a workaround.

-- 
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3496 bytes --]

From 78696689a46c13cdd2625c59348cbb1c03a32b0e Mon Sep 17 00:00:00 2001
From: Stefan Seefeld <stefan_seefeld@mentor.com>
Date: Sat, 27 Jul 2013 01:10:23 -0400
Subject: [PATCH] Add trace support for calloc and realloc.

Signed-off-by: Stefan Seefeld <stefan_seefeld@mentor.com>
---
 liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 68 ++++++++++++++++++++++++++++
 liblttng-ust-libc-wrapper/ust_libc.h         | 18 ++++++++
 2 files changed, 86 insertions(+)

diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
index 3212ff0..772663a 100644
--- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
+++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
@@ -57,3 +57,71 @@ void free(void *ptr)
 	tracepoint(ust_libc, free, ptr);
 	plibc_free(ptr);
 }
+
+/*
+   The following 'dummy' variables and functions are needed
+   to break out of an endless recursion:
+   dlsym itself appears to call calloc, so we need to provide
+   an implementation for the very first call to calloc to use.
+*/
+
+#define DUMMY_HEAP_SIZE 1024
+static char dummy_heap[DUMMY_HEAP_SIZE] = {0};
+static int dummy_end_of_heap = 0;
+
+static void *dummy_malloc(size_t size)
+{
+    if(size == 0)
+	return NULL;
+
+    size_t dummy_end_of_heap_reserved = dummy_end_of_heap + size;
+    if(dummy_end_of_heap_reserved >= DUMMY_HEAP_SIZE)
+	return NULL;
+
+    void *loc = (void*) &dummy_heap[dummy_end_of_heap];
+    dummy_end_of_heap = dummy_end_of_heap_reserved;
+    return loc;
+};
+
+static void *dummy_calloc(size_t nmemb, size_t size)
+{
+  return dummy_malloc(nmemb * size);
+}
+
+void *calloc(size_t nmemb, size_t size)
+{
+        static void *(* volatile plibc_calloc)(size_t nmemb, size_t size) = NULL;
+	void *retval;
+
+	if (plibc_calloc == NULL) {
+	        /* Temporarily redirect to dummy_calloc,
+		   until the dlsym lookup has completed. */
+                plibc_calloc = dummy_calloc;
+		plibc_calloc = dlsym(RTLD_NEXT, "calloc");
+		if (plibc_calloc == NULL) {
+			fprintf(stderr, "callocwrap: unable to find calloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_calloc(nmemb, size);
+	tracepoint(ust_libc, calloc, nmemb, size, retval);
+	return retval;
+}
+
+void *realloc(void *ptr, size_t size)
+{
+        static void *(*plibc_realloc)(void *ptr, size_t size) = NULL;
+	void *retval;
+
+	if (plibc_realloc == NULL) {
+		plibc_realloc = dlsym(RTLD_NEXT, "realloc");
+		if (plibc_realloc == NULL) {
+			fprintf(stderr, "reallocwrap: unable to find realloc\n");
+			return NULL;
+		}
+	}
+	retval = plibc_realloc(ptr, size);
+	tracepoint(ust_libc, realloc, ptr, size, retval);
+	return retval;
+}
+
diff --git a/liblttng-ust-libc-wrapper/ust_libc.h b/liblttng-ust-libc-wrapper/ust_libc.h
index af705aa..6b03a4d 100644
--- a/liblttng-ust-libc-wrapper/ust_libc.h
+++ b/liblttng-ust-libc-wrapper/ust_libc.h
@@ -47,6 +47,24 @@ TRACEPOINT_EVENT(ust_libc, free,
 	)
 )
 
+TRACEPOINT_EVENT(ust_libc, calloc,
+	TP_ARGS(size_t, nmemb, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer(size_t, nmemb, nmemb)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+	)
+)
+
+TRACEPOINT_EVENT(ust_libc, realloc,
+	TP_ARGS(void *, in_ptr, size_t, size, void *, ptr),
+	TP_FIELDS(
+		ctf_integer_hex(unsigned long, in_ptr, (unsigned long) in_ptr)
+		ctf_integer(size_t, size, size)
+		ctf_integer_hex(unsigned long, ptr, (unsigned long) ptr)
+	)
+)
+
 #endif /* _TRACEPOINT_UST_LIBC_H */
 
 #undef TRACEPOINT_INCLUDE
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev