Re: [PATCH v2 01/10] mtrace: Deprecate mallwatch and tr_break

[DJ Delorie <dj@redhat.com>]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> The variable and function pair appear to provide a way for users to
> set conditional breakpoints in mtrace when a specific address is
> returned by the allocator.  This can be achieved by using conditional
> breakpoints in gdb so it is redundant.  There is no documentation of
> this interface in the manual either, so it appears to have been a hack
> that got added to debug malloc.  Deprecate these symbols and do not
> call tr_break anymore.
> ---
>  NEWS            |  4 ++++
>  malloc/mtrace.c | 57 +++++++++++++++++--------------------------------
>  2 files changed, 24 insertions(+), 37 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 60933bd975..8e72946c3f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -93,6 +93,10 @@ Deprecated and removed features, and other changes affecting compatibility:
>    package managers that delete removed files late during the package
>    upgrade or downgrade process.
>  
> +* The symbols mallwatch and tr_break are now deprecated and no longer used in
> +  mtrace.  Similar functionality can be achieved by using conditional
> +  breakpoints within mtrace functions from within gdb.

Ok.

> diff --git a/malloc/mtrace.c b/malloc/mtrace.c
> index b65b21a933..6c2c58b706 100644
> --- a/malloc/mtrace.c
> +++ b/malloc/mtrace.c
> @@ -50,8 +50,25 @@ static char *malloc_trace_buffer;
>  
>  __libc_lock_define_initialized (static, lock);
>  
> -/* Address to breakpoint on accesses to... */
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_34)
> +/* Compatibility symbols that were introduced to help break at allocation sites
> +   for specific memory allocations.  This is unusable with ASLR, although gdb
> +   may allow predictable allocation addresses.  Even then, gdb has watchpoint
> +   and conditional breakpoint support which should provide the same
> +   functionality without having this kludge.  These symbols are preserved in
> +   case some applications ended up linking against them but they don't actually
> +   do anything anymore; not that they did much before anyway.  */
> +
>  void *mallwatch;
> +compat_symbol (libc, mallwatch, mallwatch, GLIBC_2_0);
> +
> +void
> +tr_break (void)
> +{
> +}
> +compat_symbol (libc, tr_break, tr_break, GLIBC_2_0);
> +#endif
> +

Ok.

> @@ -61,19 +78,6 @@ static void *(*tr_old_realloc_hook) (void *ptr, size_t size,
>  static void *(*tr_old_memalign_hook) (size_t __alignment, size_t __size,
>  				      const void *);
>  
> -/* This function is called when the block being alloc'd, realloc'd, or
> -   freed has an address matching the variable "mallwatch".  In a debugger,
> -   set "mallwatch" to the address of interest, then put a breakpoint on
> -   tr_break.  */
> -
> -extern void tr_break (void) __THROW;
> -libc_hidden_proto (tr_break)
> -void
> -tr_break (void)
> -{
> -}
> -libc_hidden_def (tr_break)
> -

Ok.

> @@ -167,12 +171,6 @@ tr_freehook (void *ptr, const void *caller)
>    tr_where (caller, info);
>    /* Be sure to print it first.  */
>    fprintf (mallstream, "- %p\n", ptr);
> -  if (ptr == mallwatch)
> -    {
> -      __libc_lock_unlock (lock);
> -      tr_break ();
> -      __libc_lock_lock (lock);
> -    }

Ok.

> @@ -203,9 +201,6 @@ tr_mallochook (size_t size, const void *caller)
>  
>    __libc_lock_unlock (lock);
>  
> -  if (hdr == mallwatch)
> -    tr_break ();
> -

Ok.

> @@ -214,9 +209,6 @@ tr_reallochook (void *ptr, size_t size, const void *caller)
>  {
>    void *hdr;
>  
> -  if (ptr == mallwatch)
> -    tr_break ();
> -

Ok.

> @@ -247,9 +239,6 @@ tr_reallochook (void *ptr, size_t size, const void *caller)
>  
>    __libc_lock_unlock (lock);
>  
> -  if (hdr == mallwatch)
> -    tr_break ();
> -

Ok.

> -  if (hdr == mallwatch)
> -    tr_break ();
> -

Ok.

> -/* We enable tracing if either the environment variable MALLOC_TRACE
> -   is set, or if the variable mallwatch has been patched to an address
> -   that the debugging user wants us to stop on.  When patching mallwatch,
> -   don't forget to set a breakpoint on tr_break!  */
> +/* We enable tracing if the environment variable MALLOC_TRACE is set.  */
>  
>  void
>  mtrace (void)
> @@ -321,7 +304,7 @@ mtrace (void)
>  #else
>    mallfile = getenv (mallenv);
>  #endif
> -  if (mallfile != NULL || mallwatch != NULL)
> +  if (mallfile != NULL)
>      {
>        char *mtb = malloc (TRACE_BUFFER_SIZE);
>        if (mtb == NULL)

Ok.

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