Re: [PATCH 3/8] glibc.malloc.check: Wean away from malloc hooks

[DJ Delorie <dj@redhat.com>]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> Set up a new internal debugging hooks flag variable and migrate
> glibc.malloc.check to it so that it no longer uses the malloc hooks.

> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 4960aafd08..77855801c8 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -21,6 +21,8 @@
>     corrupt pointer is detected: do nothing (0), print an error message
>     (1), or call abort() (2). */
>  
> +#  include <stdbool.h>
> +

Ok.

> @@ -29,6 +31,14 @@
>  # define weak_variable weak_function
>  #endif
>  
> +/* The internal malloc debugging hooks.  */
> +enum malloc_debug_hooks
> +{
> +  MALLOC_NONE_HOOK = 0,
> +  MALLOC_CHECK_HOOK = 1 << 0,	/* MALLOC_CHECK_ or glibc.malloc.check.  */
> +};
> +static unsigned __malloc_debugging_hooks;
> +

Ok.

> @@ -48,6 +58,24 @@ void *weak_variable (*__memalign_hook)
>  
>  static void ptmalloc_init (void);
>  
> +static __always_inline bool
> +__is_malloc_debug_enabled (enum malloc_debug_hooks flag)
> +{
> +  return __malloc_debugging_hooks & flag;
> +}
> +
> +static __always_inline void
> +__malloc_debug_enable (enum malloc_debug_hooks flag)
> +{
> +  __malloc_debugging_hooks |= flag;
> +}
> +
> +static __always_inline void
> +__malloc_debug_disable (enum malloc_debug_hooks flag)
> +{
> +  __malloc_debugging_hooks &= ~flag;
> +}
> +

Ok.

> @@ -63,6 +91,12 @@ _malloc_debug_before (size_t bytes, void **victimp, const void *address)
>        *victimp = (*hook)(bytes, address);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = malloc_check (bytes);
> +      return true;
> +    }
>    return false;
>  }

Ok.  Too bad there isn't a way to tag the *function* as likely/unlikely,
rather than the *calls*.

> @@ -76,6 +110,12 @@ _free_debug_before (void *mem, const void *address)
>        (*hook)(mem, address);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      free_check (mem);
> +      return true;
> +    }
>    return false;
>  }

Ok.

> @@ -91,6 +131,12 @@ _realloc_debug_before (void *oldmem, size_t bytes, void **victimp,
>        return true;
>      }
>  
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = realloc_check (oldmem, bytes);
> +      return true;
> +    }
> +
>    return false;
>  }

Ok.

> @@ -105,6 +151,13 @@ _memalign_debug_before (size_t alignment, size_t bytes, void **victimp,
>        *victimp = (*hook)(alignment, bytes, address);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = memalign_check (alignment, bytes);
> +      return true;
> +    }
> +
>    return false;
>  }

Ok.

> @@ -120,6 +173,14 @@ _calloc_debug_before (size_t bytes, void **victimp, const void *address)
>  	memset (*victimp, 0, bytes);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = malloc_check (bytes);
> +      if (*victimp != NULL)
> +	memset (*victimp, 0, bytes);
> +      return true;
> +    }
>    return false;
>  }

Ok.

> @@ -195,7 +256,7 @@ malloc_set_state (void *msptr)
>    __realloc_hook = NULL;
>    __free_hook = NULL;
>    __memalign_hook = NULL;
> -  using_malloc_checking = 0;
> +  __malloc_debug_disable (MALLOC_CHECK_HOOK);

Ok.

> diff --git a/malloc/malloc-check.c b/malloc/malloc-check.c
> index dcab880510..a85e519498 100644
> --- a/malloc/malloc-check.c
> +++ b/malloc/malloc-check.c
> @@ -18,20 +18,6 @@
>     not, see <https://www.gnu.org/licenses/>.  */
>  
>  
> -/* Whether we are using malloc checking.  */
> -static int using_malloc_checking;
> -
> -/* Activate a standard set of debugging hooks. */
> -void
> -__malloc_check_init (void)
> -{
> -  using_malloc_checking = 1;
> -  __malloc_hook = malloc_check;
> -  __free_hook = free_check;
> -  __realloc_hook = realloc_check;
> -  __memalign_hook = memalign_check;
> -}
> -

Ok.

> @@ -69,7 +55,7 @@ malloc_check_get_size (mchunkptr p)
>    unsigned char c;
>    unsigned char magic = magicbyte (p);
>  
> -  assert (using_malloc_checking == 1);
> +  assert (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK));

Ok.

> @@ -203,7 +189,7 @@ top_check (void)
>  }
>  
>  static void *
> -malloc_check (size_t sz, const void *caller)
> +malloc_check (size_t sz)

Ok.

> @@ -222,7 +208,7 @@ malloc_check (size_t sz, const void *caller)
>  }
>  
>  static void
> -free_check (void *mem, const void *caller)
> +free_check (void *mem)

Ok.

> @@ -256,7 +242,7 @@ free_check (void *mem, const void *caller)
>  }
>  
>  static void *
> -realloc_check (void *oldmem, size_t bytes, const void *caller)
> +realloc_check (void *oldmem, size_t bytes)

Ok.

> @@ -269,11 +255,11 @@ realloc_check (void *oldmem, size_t bytes, const void *caller)
>        return NULL;
>      }
>    if (oldmem == 0)
> -    return malloc_check (bytes, NULL);
> +    return malloc_check (bytes);

Ok.

>    if (bytes == 0)
>      {
> -      free_check (oldmem, NULL);
> +      free_check (oldmem);

Ok.

> @@ -348,12 +334,12 @@ invert:
>  }
>  
>  static void *
> -memalign_check (size_t alignment, size_t bytes, const void *caller)
> +memalign_check (size_t alignment, size_t bytes)
>  {
>    void *mem;
>  
>    if (alignment <= MALLOC_ALIGNMENT)
> -    return malloc_check (bytes, NULL);
> +    return malloc_check (bytes);

Ok

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 75ca6ec3f0..60753446a1 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1124,13 +1124,6 @@ static void munmap_chunk(mchunkptr p);
>  static mchunkptr mremap_chunk(mchunkptr p, size_t new_size);
>  #endif
>  
> -static void*   malloc_check(size_t sz, const void *caller);
> -static void      free_check(void* mem, const void *caller);
> -static void*   realloc_check(void* oldmem, size_t bytes,
> -			       const void *caller);
> -static void*   memalign_check(size_t alignment, size_t bytes,
> -				const void *caller);

Ok.

> @@ -5078,7 +5071,7 @@ musable (void *mem)
>  
>        p = mem2chunk (mem);
>  
> -      if (__builtin_expect (using_malloc_checking == 1, 0))
> +      if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
>  	return malloc_check_get_size (p);

Ok.

>        if (chunk_is_mmapped (p))
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 1861d20006..357a3b0b30 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -210,7 +210,7 @@ TUNABLE_CALLBACK (set_mallopt_check) (tunable_val_t *valp)
>  {
>    int32_t value = (int32_t) valp->numval;
>    if (value != 0)
> -    __malloc_check_init ();
> +    __malloc_debug_enable (MALLOC_CHECK_HOOK);
>  }

Ok.

>  # define TUNABLE_CALLBACK_FNDECL(__name, __type) \
> @@ -400,7 +400,7 @@ ptmalloc_init (void)
>          }
>      }
>    if (s && s[0] != '\0' && s[0] != '0')
> -    __malloc_check_init ();
> +    __malloc_debug_enable (MALLOC_CHECK_HOOK);
>  #endif

Ok.

LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>