Re: [PATCH v8 03/10] Remove __morecore and __default_morecore

[Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org>]

On 7/13/21 3:38 AM, Siddhesh Poyarekar wrote:
> Make the __morecore and __default_morecore symbols compat-only and
> remove their declarations from the API.  Also, include morecore.c
> directly into malloc.c; this should ideally get merged into malloc in
> a future cleanup.

OK for 2.34.

Tested without regression on x86_64 and i686.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  NEWS              |  5 +++++
>  include/stdlib.h  |  3 ---
>  malloc/Makefile   |  2 +-
>  malloc/arena.c    | 12 ++----------
>  malloc/hooks.c    |  2 ++
>  malloc/malloc.c   |  7 +++----
>  malloc/malloc.h   |  8 --------
>  malloc/morecore.c | 34 +++++++++++-----------------------
>  8 files changed, 24 insertions(+), 49 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 13ffe627da..18d9e65eb2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -113,6 +113,11 @@ Deprecated and removed features, and other changes affecting compatibility:
>    mtrace.  Similar functionality can be achieved by using conditional
>    breakpoints within mtrace functions from within gdb.
>  
> +* The __morecore and __after_morecore_hook malloc hooks and the default
> +  implementation __default_morecore have been removed from the API.  Existing
> +  applications will continue to link against these symbols but the interfaces
> +  no longer have any effect on malloc.

OK.

> +
>  Changes to build and runtime requirements:
>  
>  * On Linux, the shm_open, sem_open, and related functions now expect the
> diff --git a/include/stdlib.h b/include/stdlib.h
> index 1f6e1508e4..1c6f70b082 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -306,9 +306,6 @@ libc_hidden_proto (__qfcvt_r)
>  #  define MB_CUR_MAX (_NL_CURRENT_WORD (LC_CTYPE, _NL_CTYPE_MB_CUR_MAX))
>  # endif
>  
> -extern void *__default_morecore (ptrdiff_t) __THROW;
> -libc_hidden_proto (__default_morecore)

OK.

> -
>  struct abort_msg_s
>  {
>    unsigned int size;
> diff --git a/malloc/Makefile b/malloc/Makefile
> index d15729569b..020f781b59 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -104,7 +104,7 @@ tests-exclude-mcheck = tst-mallocstate \
>  tests-mcheck = $(filter-out $(tests-exclude-mcheck), $(tests))
>  endif
>  
> -routines = malloc morecore mcheck mtrace obstack reallocarray \
> +routines = malloc mcheck mtrace obstack reallocarray \

OK.

>    scratch_buffer_dupfree \
>    scratch_buffer_grow scratch_buffer_grow_preserve \
>    scratch_buffer_set_array_size \
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 991fc21a7e..f1693ed48f 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -274,14 +274,6 @@ next_env_entry (char ***position)
>  #endif
>  
>  
> -#if defined(SHARED) || defined(USE_MTAG)
> -static void *
> -__failing_morecore (ptrdiff_t d)
> -{
> -  return (void *) MORECORE_FAILURE;
> -}
> -#endif
> -

OK.

>  #ifdef SHARED
>  extern struct dl_open_hook *_dl_open_hook;
>  libc_hidden_proto (_dl_open_hook);
> @@ -310,7 +302,7 @@ ptmalloc_init (void)
>  	 and that morecore does not support tagged regions, then
>  	 disable it.  */
>        if (__MTAG_SBRK_UNTAGGED)
> -	__morecore = __failing_morecore;
> +	__always_fail_morecore = true;

OK.

>  
>        mtag_enabled = true;
>        mtag_mmap_flags = __MTAG_MMAP_FLAGS;
> @@ -323,7 +315,7 @@ ptmalloc_init (void)
>       generic sbrk implementation also enforces this, but it is not
>       used on Hurd.  */
>    if (!__libc_initial)
> -    __morecore = __failing_morecore;
> +    __always_fail_morecore = true;

OK.

>  #endif
>  
>    thread_arena = &main_arena;
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 45c91d6502..4aa6dadcff 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -20,6 +20,8 @@
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_34)
>  void weak_variable (*__after_morecore_hook) (void) = NULL;
>  compat_symbol (libc, __after_morecore_hook, __after_morecore_hook, GLIBC_2_0);
> +void *(*__morecore)(ptrdiff_t);
> +compat_symbol (libc, __morecore, __morecore, GLIBC_2_0);

OK.

>  #endif
>  
>  /* Hooks for debugging versions.  The initial hooks just call the
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 24e7854a0e..6e8fa9e424 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -384,12 +384,11 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
>  #define TRIM_FASTBINS  0
>  #endif
>  
> -
>  /* Definition for getting more memory from the OS.  */
> -#define MORECORE         (*__morecore)
> +#include "morecore.c"
> +
> +#define MORECORE         (*__glibc_morecore)

OK.

>  #define MORECORE_FAILURE 0
> -void * __default_morecore (ptrdiff_t);
> -void *(*__morecore)(ptrdiff_t) = __default_morecore;

OK.

>  
>  /* Memory tagging.  */
>  
> diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 634b7db868..17ab9ee345 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -76,14 +76,6 @@ extern void *valloc (size_t __size) __THROW __attribute_malloc__
>  extern void *pvalloc (size_t __size) __THROW __attribute_malloc__
>    __wur __attr_dealloc_free;
>  
> -/* Underlying allocation function; successive calls should return
> -   contiguous pieces of memory.  */
> -extern void *(*__morecore) (ptrdiff_t __size) __MALLOC_DEPRECATED;
> -
> -/* Default value of `__morecore'.  */
> -extern void *__default_morecore (ptrdiff_t __size)
> -__THROW __attribute_malloc__  __MALLOC_DEPRECATED;

OK.

> -
>  /* SVID2/XPG mallinfo structure */
>  
>  struct mallinfo
> diff --git a/malloc/morecore.c b/malloc/morecore.c
> index 047228779b..8168ef158c 100644
> --- a/malloc/morecore.c
> +++ b/malloc/morecore.c
> @@ -15,39 +15,27 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#ifndef _MALLOC_INTERNAL
> -# define _MALLOC_INTERNAL
> -# include <malloc.h>
> -#endif
> -
> -#ifndef __GNU_LIBRARY__
> -# define __sbrk  sbrk
> -#endif
> -
> -#ifdef __GNU_LIBRARY__
> -/* It is best not to declare this and cast its result on foreign operating
> -   systems with potentially hostile include files.  */
> -
> -# include <stddef.h>
> -# include <stdlib.h>
> -extern void *__sbrk (ptrdiff_t increment) __THROW;
> -libc_hidden_proto (__sbrk)
> -#endif
> -
> -#ifndef NULL
> -# define NULL 0

OK.

> +#if defined(SHARED) || defined(USE_MTAG)
> +static bool __always_fail_morecore = false;
>  #endif
>  
>  /* Allocate INCREMENT more bytes of data space,
>     and return the start of data space, or NULL on errors.
>     If INCREMENT is negative, shrink data space.  */
>  void *
> -__default_morecore (ptrdiff_t increment)
> +__glibc_morecore (ptrdiff_t increment)
>  {
> +#if defined(SHARED) || defined(USE_MTAG)
> +  if (__always_fail_morecore)
> +    return NULL;
> +#endif

OK.

> +
>    void *result = (void *) __sbrk (increment);
>    if (result == (void *) -1)
>      return NULL;
>  
>    return result;
>  }
> -libc_hidden_def (__default_morecore)
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_34)
> +compat_symbol (libc, __glibc_morecore, __default_morecore, GLIBC_2_0);

OK. Use a new name and create a versioned compat symbol for __default_morecore
in case someone calls it.

> +#endif
> 


-- 
Cheers,
Carlos.