[v4,02/10] mtrace: Deprecate mallwatch and tr_break

  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.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 NEWS            |  4 ++++
 malloc/mtrace.c | 57 +++++++++++++++++--------------------------------
 2 files changed, 24 insertions(+), 37 deletions(-)
  

On 7/2/21 7:38 AM, Siddhesh Poyarekar wrote:
> 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.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> Reviewed-by: DJ Delorie <dj@redhat.com>
> ---
>  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.
> +
>  Changes to build and runtime requirements:
>  
>  * On Linux, the shm_open, sem_open, and related functions now expect the
> 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
> +
>  
>  /* Old hook values.  */
>  static void (*tr_old_free_hook) (void *ptr, const void *);
> @@ -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)
> -
>  static void
>  tr_where (const void *caller, Dl_info *info)
>  {
> @@ -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);
> -    }
>    set_default_hooks ();
>    if (tr_old_free_hook != NULL)
>      (*tr_old_free_hook)(ptr, caller);
> @@ -203,9 +201,6 @@ tr_mallochook (size_t size, const void *caller)
>  
>    __libc_lock_unlock (lock);
>  
> -  if (hdr == mallwatch)
> -    tr_break ();
> -
>    return hdr;
>  }
>  
> @@ -214,9 +209,6 @@ tr_reallochook (void *ptr, size_t size, const void *caller)
>  {
>    void *hdr;
>  
> -  if (ptr == mallwatch)
> -    tr_break ();
> -
>    Dl_info mem;
>    Dl_info *info = lock_and_info (caller, &mem);
>  
> @@ -247,9 +239,6 @@ tr_reallochook (void *ptr, size_t size, const void *caller)
>  
>    __libc_lock_unlock (lock);
>  
> -  if (hdr == mallwatch)
> -    tr_break ();
> -
>    return hdr;
>  }
>  
> @@ -274,9 +263,6 @@ tr_memalignhook (size_t alignment, size_t size, const void *caller)
>  
>    __libc_lock_unlock (lock);
>  
> -  if (hdr == mallwatch)
> -    tr_break ();
> -
>    return hdr;
>  }
>  
> @@ -296,10 +282,7 @@ release_libc_mem (void)
>  #endif
>  
>  
> -/* 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)
>
  

Siddhesh Poyarekar via Libc-alpha <libc-alpha@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.

I noticed new build failures on ppc64le after this patch when using GCC 8 and 9.

/tmp/ccg1QEe7.s: Assembler messages:
/tmp/ccg1QEe7.s: Error: `__SImallwatch_0' can't be equated to common symbol `mallwatch'

I can't reproduce the issue when using GCC 10 or newer.
  

On 02/07/2021 17:37, Tulio Magno Quites Machado Filho via Libc-alpha wrote:
> Siddhesh Poyarekar via Libc-alpha <libc-alpha@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.
> 
> I noticed new build failures on ppc64le after this patch when using GCC 8 and 9.
> 
> /tmp/ccg1QEe7.s: Assembler messages:
> /tmp/ccg1QEe7.s: Error: `__SImallwatch_0' can't be equated to common symbol `mallwatch'
> 
> I can't reproduce the issue when using GCC 10 or newer.
> 

I think we will need to use __attribute__ ((nocommon)) on mallwatch.
  

* Tulio Magno Quites Machado Filho:

> Siddhesh Poyarekar via Libc-alpha <libc-alpha@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.
>
> I noticed new build failures on ppc64le after this patch when using GCC 8 and 9.
>
> /tmp/ccg1QEe7.s: Assembler messages:
> /tmp/ccg1QEe7.s: Error: `__SImallwatch_0' can't be equated to common symbol `mallwatch'
>
> I can't reproduce the issue when using GCC 10 or newer.

Missing __attribute__ ((nocommon)).  GCC 10 defaults to -fno-common.

I plan to write a patch that builds glibc unconditionally with
-fno-common, then the attribute won't be needed even with older GCC.

Thanks,
Florian
  

On 7/3/21 2:12 AM, Adhemerval Zanella wrote:
> 
> 
> On 02/07/2021 17:37, Tulio Magno Quites Machado Filho via Libc-alpha wrote:
>> Siddhesh Poyarekar via Libc-alpha <libc-alpha@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.
>>
>> I noticed new build failures on ppc64le after this patch when using GCC 8 and 9.
>>
>> /tmp/ccg1QEe7.s: Assembler messages:
>> /tmp/ccg1QEe7.s: Error: `__SImallwatch_0' can't be equated to common symbol `mallwatch'
>>
>> I can't reproduce the issue when using GCC 10 or newer.
>>
> 
> I think we will need to use __attribute__ ((nocommon)) on mallwatch.
> 

Thanks for looking into it Adhemerval.  Tulio, can you please check if 
that fixes the build and if it does, push a patch with that?  x86_64 
continues to build fine with the attribute.

Thanks,
Siddhesh
  

On 7/3/21 2:13 AM, Florian Weimer via Libc-alpha wrote:
> * Tulio Magno Quites Machado Filho:
> 
>> Siddhesh Poyarekar via Libc-alpha <libc-alpha@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.
>>
>> I noticed new build failures on ppc64le after this patch when using GCC 8 and 9.
>>
>> /tmp/ccg1QEe7.s: Assembler messages:
>> /tmp/ccg1QEe7.s: Error: `__SImallwatch_0' can't be equated to common symbol `mallwatch'
>>
>> I can't reproduce the issue when using GCC 10 or newer.
> 
> Missing __attribute__ ((nocommon)).  GCC 10 defaults to -fno-common.
> 
> I plan to write a patch that builds glibc unconditionally with
> -fno-common, then the attribute won't be needed even with older GCC.

Ahh OK, that would do too.

Thanks,
Siddhesh
  

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:

> Thanks for looking into it Adhemerval.  Tulio, can you please check if 
> that fixes the build and if it does, push a patch with that?  x86_64 
> continues to build fine with the attribute.

It does work!  I can push it indeed, but first...

> I plan to write a patch that builds glibc unconditionally with
> -fno-common, then the attribute won't be needed even with older GCC.

Florian, how is your plan on this?  Would you like me to push this patch first?
  

* Tulio Magno Quites Machado Filho:

> Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
>
>> Thanks for looking into it Adhemerval.  Tulio, can you please check if 
>> that fixes the build and if it does, push a patch with that?  x86_64 
>> continues to build fine with the attribute.
>
> It does work!  I can push it indeed, but first...
>
>> I plan to write a patch that builds glibc unconditionally with
>> -fno-common, then the attribute won't be needed even with older GCC.
>
> Florian, how is your plan on this?  Would you like me to push this
> patch first?

That patch is still weeks away and is probably 2.35 anyway.  Please go
ahead with the localized fix.

Thanks,
Florian