Re: [PATCH] malloc: Remove malloc_get_state, malloc_set_state

[Carlos O'Donell <carlos at redhat dot com>]

On 06/10/2016 05:31 AM, Florian Weimer wrote:
> After the removal of __malloc_initialize_hook, newly compiled
> Emacs binaries are no longer able to use these interfaces.
> malloc_get_state is only used during the Emacs build process,
> so we provide a stub implementation only.  Existing Emacs binaries
> will not call this stub function, but still reference the symbol.

TLDR; Update news, and wiki, and this looks good to me.

This is a single cycle deprecation for a function which we have been saying
is going to be removed for the last 5 years. Given that we have shown that
nothing in distribution but Emacs uses this, that's fine with me.

However, I would like the release notes updated to mention that there
are going to be packaging changes required or user application changes
required if they were using these APIs.

e.g.
https://sourceware.org/glibc/wiki/Release/2.24#Packaging_Changes


> 2016-06-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	* malloc/malloc.h (malloc_get_state, malloc_set_state): Remove.
> 	* malloc/malloc.c (malloc_get_state, malloc_set_state): Remove
> 	weak aliases.
> 	* malloc/hooks.c (malloc_get_state): New stub implementation as
> 	compatibility symbo.

s/symbo/symbol/g.

> 	(__malloc_get_state): Remove.
> 	(malloc_set_state): Rename from __malloc_set_state.  Turn into
> 	compat symbol.
> 	* malloc/Makefile (tests): Remove tst-mallocstate.
> 	* malloc/tst-mallocstate.c: Remove file.

Minor comment about the NEWS entry.

> diff --git a/NEWS b/NEWS
> index 9d6ac56..de2af85 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -36,6 +36,13 @@ Version 2.24
>  * The deprecated __malloc_initialize_hook variable has been removed from the
>    API.
>  
> +* The __malloc_get_state and __malloc_set_state functions have been removed
> +  from the API.  __malloc_get_state has been replaced with a stub
> +  implementation.  Existing undumped Emacs binaries will have to be
> +  recompiled so that they do not use glibc malloc (or malloc heap dumping).
> +  Existing installed Emacs binaries (after dumping) are not affected by this
> +  change.

This needs to explain that an "Emacs" you use in a distribution is an installed
dumped Emacs.

e.g. Add "Emacs users should not see any impact on their installed distribution
binaries of Emacs."

Or something like that.

> +
>  Security related changes:
>  
>  * An unnecessary stack copy in _nss_dns_getnetbyname_r was removed.  It
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 91eb17d..8473f74 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -25,7 +25,7 @@ include ../Makeconfig
>  dist-headers := malloc.h
>  headers := $(dist-headers) obstack.h mcheck.h
>  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
> -	 tst-mallocstate tst-mcheck tst-mallocfork tst-trim1 \
> +	 tst-mcheck tst-mallocfork tst-trim1 \
>  	 tst-malloc-usable tst-realloc tst-posix_memalign \
>  	 tst-pvalloc tst-memalign tst-mallopt tst-scratch_buffer \
>  	 tst-malloc-backtrace tst-malloc-thread-exit \
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index caa1e70..8ef2eba 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -447,6 +447,7 @@ memalign_check (size_t alignment, size_t bytes, const void *caller)
>    return mem2mem_check (mem, bytes);
>  }
>  
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_24)
>  
>  /* Get/set state: malloc_get_state() records the current state of all
>     malloc variables (_except_ for the actual heap contents and `hook'
> @@ -492,60 +493,20 @@ struct malloc_save_state
>    unsigned long narenas;
>  };
>  
> +/* Dummy implementation which always fails.  We need to provide this
> +   symbol so that existing Emacs binaries continue to work with
> +   BIND_NOW.  */
>  void *
> -__malloc_get_state (void)
> +attribute_compat_text_section
> +malloc_get_state (void)
>  {
> -  struct malloc_save_state *ms;
> -  int i;
> -  mbinptr b;
> -
> -  ms = (struct malloc_save_state *) __libc_malloc (sizeof (*ms));
> -  if (!ms)
> -    return 0;
> -
> -  (void) mutex_lock (&main_arena.mutex);
> -  malloc_consolidate (&main_arena);
> -  ms->magic = MALLOC_STATE_MAGIC;
> -  ms->version = MALLOC_STATE_VERSION;
> -  ms->av[0] = 0;
> -  ms->av[1] = 0; /* used to be binblocks, now no longer used */
> -  ms->av[2] = top (&main_arena);
> -  ms->av[3] = 0; /* used to be undefined */
> -  for (i = 1; i < NBINS; i++)
> -    {
> -      b = bin_at (&main_arena, i);
> -      if (first (b) == b)
> -        ms->av[2 * i + 2] = ms->av[2 * i + 3] = 0; /* empty bin */
> -      else
> -        {
> -          ms->av[2 * i + 2] = first (b);
> -          ms->av[2 * i + 3] = last (b);
> -        }
> -    }
> -  ms->sbrk_base = mp_.sbrk_base;
> -  ms->sbrked_mem_bytes = main_arena.system_mem;
> -  ms->trim_threshold = mp_.trim_threshold;
> -  ms->top_pad = mp_.top_pad;
> -  ms->n_mmaps_max = mp_.n_mmaps_max;
> -  ms->mmap_threshold = mp_.mmap_threshold;
> -  ms->check_action = check_action;
> -  ms->max_sbrked_mem = main_arena.max_system_mem;
> -  ms->max_total_mem = 0;
> -  ms->n_mmaps = mp_.n_mmaps;
> -  ms->max_n_mmaps = mp_.max_n_mmaps;
> -  ms->mmapped_mem = mp_.mmapped_mem;
> -  ms->max_mmapped_mem = mp_.max_mmapped_mem;
> -  ms->using_malloc_checking = using_malloc_checking;
> -  ms->max_fast = get_max_fast ();
> -  ms->arena_test = mp_.arena_test;
> -  ms->arena_max = mp_.arena_max;
> -  ms->narenas = narenas;
> -  (void) mutex_unlock (&main_arena.mutex);
> -  return (void *) ms;
> +  return NULL;
>  }
> +compat_symbol (libc, malloc_get_state, malloc_get_state, GLIBC_2_0);
>  
>  int
> -__malloc_set_state (void *msptr)
> +attribute_compat_text_section
> +malloc_set_state (void *msptr)
>  {
>    struct malloc_save_state *ms = (struct malloc_save_state *) msptr;
>  
> @@ -612,6 +573,9 @@ __malloc_set_state (void *msptr)
>  
>    return 0;
>  }
> +compat_symbol (libc, malloc_set_state, malloc_set_state, GLIBC_2_0);
> +
> +#endif	/* SHLIB_COMPAT */
>  
>  /*
>   * Local variables:
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index ac0f751..b054fe6 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -5266,8 +5266,6 @@ strong_alias (__libc_mallopt, __mallopt) weak_alias (__libc_mallopt, mallopt)
>  weak_alias (__malloc_stats, malloc_stats)
>  weak_alias (__malloc_usable_size, malloc_usable_size)
>  weak_alias (__malloc_trim, malloc_trim)
> -weak_alias (__malloc_get_state, malloc_get_state)
> -weak_alias (__malloc_set_state, malloc_set_state)
>  
>  
>  /* ------------------------------------------------------------
> diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 54b1862..e0c2788 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -134,13 +134,6 @@ extern void malloc_stats (void) __THROW;
>  /* Output information about state of allocator to stream FP.  */
>  extern int malloc_info (int __options, FILE *__fp) __THROW;
>  
> -/* Record the state of all malloc variables in an opaque data structure. */
> -extern void *malloc_get_state (void) __THROW;
> -
> -/* Restore the state of all malloc variables from data obtained with
> -   malloc_get_state(). */
> -extern int malloc_set_state (void *__ptr) __THROW;
> -
>  /* Hooks for debugging and user-defined versions. */
>  extern void (*__MALLOC_HOOK_VOLATILE __free_hook) (void *__ptr,
>                                                     const void *)
> diff --git a/malloc/tst-mallocstate.c b/malloc/tst-mallocstate.c
> deleted file mode 100644
> index a00d045..0000000
> --- a/malloc/tst-mallocstate.c
> +++ /dev/null
> @@ -1,84 +0,0 @@
> -/* Copyright (C) 2001-2016 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Contributed by Wolfram Gloger <wg@malloc.de>, 2001.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library; if not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#include <errno.h>
> -#include <stdio.h>
> -#include "malloc.h"
> -
> -static int errors = 0;
> -
> -static void
> -merror (const char *msg)
> -{
> -  ++errors;
> -  printf ("Error: %s\n", msg);
> -}
> -
> -static int
> -do_test (void)
> -{
> -  void *p1, *p2;
> -  void *save_state;
> -  long i;
> -
> -  errno = 0;
> -
> -  p1 = malloc (10);
> -  if (p1 == NULL)
> -    merror ("malloc (10) failed.");
> -
> -  p2 = malloc (20);
> -  if (p2 == NULL)
> -    merror ("malloc (20) failed.");
> -
> -  free (malloc (10));
> -
> -  for (i = 0; i < 100; ++i)
> -    {
> -      save_state = malloc_get_state ();
> -      if (save_state == NULL)
> -        {
> -          merror ("malloc_get_state () failed.");
> -          break;
> -        }
> -      /*free (malloc (10)); This could change the top chunk! */
> -      malloc_set_state (save_state);
> -      p1 = realloc (p1, i * 4 + 4);
> -      if (p1 == NULL)
> -        merror ("realloc (i*4) failed.");
> -      free (save_state);
> -    }
> -
> -  p1 = realloc (p1, 40);
> -  free (p2);
> -  p2 = malloc (10);
> -  if (p2 == NULL)
> -    merror ("malloc (10) failed.");
> -  free (p1);
> -
> -  return errors != 0;
> -}
> -
> -/*
> - * Local variables:
> - * c-basic-offset: 2
> - * End:
> - */
> -
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> 


-- 
Cheers,
Carlos.