[PATCH 5/8] mtrace: Wean away from malloc hooks

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> Split mtrace hooks into before and after and adapt to the new internal
> debugging hooks infrastructure.  With this, the malloc hooks are
> unused internally and can be removed from the main library.
>
> This also eliminates the only use of memalign through a PLT in the
> library, so checklocalplt data needs to be updated to reflect that.

> diff --git a/include/malloc.h b/include/malloc.h
> index bb1123d9d3..6169d486f5 100644
> --- a/include/malloc.h
> +++ b/include/malloc.h
> @@ -15,6 +15,7 @@ typedef struct malloc_state *mstate;
>  #define __malloc_initialized __libc_malloc_initialized
>  /* Nonzero if the malloc is already initialized.  */
>  extern int __malloc_initialized attribute_hidden;
> +extern FILE *__mtrace_mallstream attribute_hidden;

Ok.

> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index b517a98ea2..492e9aac63 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -37,6 +37,7 @@ enum malloc_debug_hooks
>    MALLOC_NONE_HOOK = 0,
>    MALLOC_CHECK_HOOK = 1 << 0,	/* MALLOC_CHECK_ or glibc.malloc.check.  */
>    MALLOC_MCHECK_HOOK = 1 << 1,	/* mcheck()  */
> +  MALLOC_MTRACE_HOOK = 1 << 2,	/* mtrace()  */
>  };
>  static unsigned __malloc_debugging_hooks;

Ok.

> @@ -111,8 +113,13 @@ _malloc_debug_before (size_t *bytesp, void **victimp, const void *address)
>  static __always_inline void *
>  _malloc_debug_after (void *mem, size_t bytes, const void *address)
>  {
> -  if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> -    mem = malloc_mcheck_after (mem, bytes);
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
> +    {
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> +	mem = malloc_mcheck_after (mem, bytes);
> +      if (__is_malloc_debug_enabled (MALLOC_MTRACE_HOOK))
> +	mem = malloc_mtrace_after (mem, bytes, address);
> +    }
>    return mem;
>  }

I suspect this should have been in part 4, but ok.

> @@ -129,6 +136,8 @@ _free_debug_before (void **mem, const void *address)
>  
>    if (__glibc_unlikely (__malloc_debugging_hooks))
>      {
> +      if (__is_malloc_debug_enabled (MALLOC_MTRACE_HOOK))
> +	free_mtrace (mem, address);
>        if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
>  	*mem = free_mcheck (*mem);
>        if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))

Ok.

> @@ -171,8 +180,13 @@ static __always_inline void *
>  _realloc_debug_after (void *mem, void *oldmem, size_t bytes, size_t oldsize,
>  		      const void *address)
>  {
> -  if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> -    mem = realloc_mcheck_after (mem, oldmem, bytes, oldsize);
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
> +    {
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> +	mem = realloc_mcheck_after (mem, oldmem, bytes, oldsize);
> +      if (__is_malloc_debug_enabled (MALLOC_MTRACE_HOOK))
> +	mem = realloc_mtrace_after (mem, oldmem, bytes, address);
> +    }
>    return mem;
>  }

Part 4, but ok.

> @@ -206,8 +220,13 @@ static __always_inline void *
>  _memalign_debug_after (void *mem, size_t alignment, size_t bytes,
>  		       const void *address)
>  {
> -  if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> -    mem = memalign_mcheck_after (mem, alignment, bytes);
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
> +    {
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> +	mem = memalign_mcheck_after (mem, alignment, bytes);
> +      if (__is_malloc_debug_enabled (MALLOC_MTRACE_HOOK))
> +	mem = memalign_mtrace_after (mem, bytes, address);
> +    }
>    return mem;
>  }

Likewise.  Ok.

> @@ -243,11 +262,14 @@ _calloc_debug_before (size_t *bytesp, void **victimp, const void *address)
>  static __always_inline void *
>  _calloc_debug_after (void *mem, size_t bytes, const void *address)
>  {
> -  if (__glibc_unlikely (__malloc_debugging_hooks) && mem != NULL)
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
>      {
>        if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
>  	mem = malloc_mcheck_after (mem, bytes);
> -      memset (mem, 0, bytes);
> +      if (__is_malloc_debug_enabled (MALLOC_MTRACE_HOOK))
> +	mem = malloc_mtrace_after (mem, bytes, address);
> +      else if (mem != NULL)
> +	memset (mem, 0, bytes);
>      }

Why "else memset" ? We don't zero memory when tracing?  Or it's zero'd
elsewhere?  What if we have both hooks set?

Regardless, deserves a comment here.

> diff --git a/malloc/mtrace-hooks.c b/malloc/mtrace-hooks.c
> new file mode 100644
> index 0000000000..c1c6d6a6e9
> --- /dev/null
> +++ b/malloc/mtrace-hooks.c

Still missing makefile dependency.

Mostly just a copy-paste from mtrace.c, so OK.

> diff --git a/malloc/mtrace.c b/malloc/mtrace.c
> index 6c2c58b706..2cc4507e25 100644
> --- a/malloc/mtrace.c
> +++ b/malloc/mtrace.c
> @@ -22,7 +22,6 @@
>  # define _MALLOC_INTERNAL
>  # include <malloc.h>
>  # include <mcheck.h>
> -# include <libc-lock.h>
>  #endif

Lots of deletions moved to mtrace-hooks.c, ok.

> @@ -276,7 +71,7 @@ static void __libc_freeres_fn_section
>  release_libc_mem (void)
>  {
>    /* Only call the free function if we still are running in mtrace mode.  */
> -  if (mallstream != NULL)
> +  if (__mtrace_mallstream != NULL)
>      __libc_freeres ();
>  }

Ok.

> @@ -293,7 +88,7 @@ mtrace (void)
>    char *mallfile;
>  
>    /* Don't panic if we're called more than once.  */
> -  if (mallstream != NULL)
> +  if (__mtrace_mallstream != NULL)
>      return;

Ok.

>  #ifdef _LIBC
> @@ -310,15 +105,15 @@ mtrace (void)
>        if (mtb == NULL)
>          return;
>  
> -      mallstream = fopen (mallfile != NULL ? mallfile : "/dev/null", "wce");
> -      if (mallstream != NULL)
> +      __mtrace_mallstream = fopen (mallfile != NULL ? mallfile : "/dev/null",
> +				   "wce");
> +      if (__mtrace_mallstream != NULL)

Ok.

>          {
>            /* Be sure it doesn't malloc its buffer!  */
>            malloc_trace_buffer = mtb;
> -          setvbuf (mallstream, malloc_trace_buffer, _IOFBF, TRACE_BUFFER_SIZE);
> -          fprintf (mallstream, "= Start\n");
> -	  save_default_hooks ();
> -	  set_trace_hooks ();
> +          setvbuf (__mtrace_mallstream, malloc_trace_buffer, _IOFBF,
> +		   TRACE_BUFFER_SIZE);
> +          fprintf (__mtrace_mallstream, "= Start\n");

Ok.

> @@ -336,15 +131,11 @@ mtrace (void)
>  void
>  muntrace (void)
>  {
> -  if (mallstream == NULL)
> +  if (__mtrace_mallstream == NULL)
>      return;

Ok.

> -  /* Do the reverse of what done in mtrace: first reset the hooks and
> -     MALLSTREAM, and only after that write the trailer and close the
> -     file.  */
> -  FILE *f = mallstream;
> -  mallstream = NULL;
> -  set_default_hooks ();
> +  FILE *f = __mtrace_mallstream;
> +  __mtrace_mallstream = NULL;
>  
>    fprintf (f, "= End\n");
>    fclose (f);

Ok.