malloc: Remove malloc_get_state, malloc_set_state

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.

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.
	(__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.

fweimer@redhat.com (Florian Weimer) writes:

> +* The __malloc_get_state and __malloc_set_state functions have been removed
> +  from the API.  __malloc_get_state has been replaced with a stub

s/__//g

Andreas.

On 06/10/2016 12:21 PM, Andreas Schwab wrote:
> fweimer@redhat.com (Florian Weimer) writes:
>
>> +* The __malloc_get_state and __malloc_set_state functions have been removed
>> +  from the API.  __malloc_get_state has been replaced with a stub
>
> s/__//g

Thanks, fixed locally.

Do you have any comments on the approach (removal without prior 
deprecation)?

Thanks,
Florian

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"
>

On Fri, 10 Jun 2016, Florian Weimer wrote:

> 	* malloc/Makefile (tests): Remove tst-mallocstate.
> 	* malloc/tst-mallocstate.c: Remove file.

It was previously stated that we should keep test coverage for the 
compatibility symbols 
<https://sourceware.org/ml/libc-alpha/2014-03/msg00388.html>.  Is this no 
longer possible because __malloc_get_state is a stub?

(Patch discussion went up to at least 
<https://sourceware.org/ml/libc-alpha/2014-05/msg00446.html>.)

Thanks for doing all this. Some comments:

> +* 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.

The NEWS item should talk about the public API and so should refer to names 
without leading underscores, and it'd be helpful to have a clearer discussion 
about the backwards-compatibility constraints. Perhaps wording like the 
following instead?

-----
The malloc_get_state and malloc_set_state functions have been removed. 
Already-existing binaries that dynamically link to these functions will get a 
hidden implementation in which malloc_get_state is a stub.  As far as we know, 
these functions are used only by GNU Emacs and this change will not adversely 
affect already-built Emacs executables.  Any undumped Emacs executables, which 
normally exist only during an Emacs build, should be rebuilt by re-running 
'configure; make' in the Emacs build tree.
-----


> +attribute_compat_text_section
> +malloc_get_state (void)
>   {
> ...
> +  return NULL;
>   }

Perhaps __malloc_get_state should set errno to ENOSYS?  Emacs won't care about 
errno, so this would merely be insurance in case someone else does care.

Perhaps the test program should be retained and should check that the hidden 
__malloc_get_state function indeed returns NULL?  Dunno how you'd test 
__malloc_set_state....

On 06/10/2016 01:42 PM, Paul Eggert wrote:
> Thanks for doing all this. Some comments:
> 
>> +* 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.
> 
> The NEWS item should talk about the public API and so should refer to
> names without leading underscores, and it'd be helpful to have a
> clearer discussion about the backwards-compatibility constraints.
> Perhaps wording like the following instead?
> 
> -----
> The malloc_get_state and malloc_set_state functions have been
> removed. Already-existing binaries that dynamically link to these
> functions will get a hidden implementation in which malloc_get_state
> is a stub. As far as we know, these functions are used only by GNU
> Emacs and this change will not adversely affect already-built Emacs
> executables. Any undumped Emacs executables, which normally exist
> only during an Emacs build, should be rebuilt by re-running
> 'configure; make' in the Emacs build tree.
> -----

+1. Excellent.

On 06/10/2016 07:42 PM, Paul Eggert wrote:
> Perhaps the test program should be retained and should check that the
> hidden __malloc_get_state function indeed returns NULL?  Dunno how you'd
> test __malloc_set_state....

I'm working on a test which emulates what Emacs is doing.  It's not very 
hard (just create something resembling a heap dump in the .data 
section), it's just a little bit of work to get the self-checking right, 
without being able to use malloc to do any book-keeping.

Unfortunately, the existing tst-mallocstate test doesn't really say if 
the heap dump/undump logic is compatible with Emacs.

Florian