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

["Jérémie Galarneau" <jeremie.galarneau@efficios.com>]

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