elf: dl-minimal malloc needs to respect fundamental alignment

  On 07/11/2016 10:55 AM, Florian Weimer wrote:

>> OK to checkin if you use MALLOC_ALIGMENT.
>
> This change is not exactly trivial because it's currently defined in
> malloc/malloc.c only.  I will need to post another version for review.

This is what I've got now.  I still thinks it's not technically correct. 
  Like I said, exporting MALLOC_ALIGNMENT is fairly invasive, and the 
number is just not the right one in this place.

Okay to check in?

Thanks,
Florian
  

On 08/03/2016 06:04 AM, Florian Weimer wrote:
> On 07/11/2016 10:55 AM, Florian Weimer wrote:
> 
>>> OK to checkin if you use MALLOC_ALIGMENT.
>>
>> This change is not exactly trivial because it's currently defined in
>> malloc/malloc.c only.  I will need to post another version for review.
> 
> This is what I've got now. I still thinks it's not technically
> correct. Like I said, exporting MALLOC_ALIGNMENT is fairly invasive,
> and the number is just not the right one in this place.

I acknowledge your objection. I disagree that it's invasive. I appreciate
the work you've done here to harmonize the internal ld.so dl-minimal with
the matching glibc malloc. While not technically required, and not guaranteed
by an interposed malloc, for the default GNU system I would like these allocators
to behave more like each other. It makes it easier to review and debug their
behaviours.

I also think that MALLOC_ALIGNMENT needs to be cleaned up and possibly
minimized down to smallest value we need given the ABI requirements, but
that's an orthogonal issue that is much harder to solve than your current
set related fixes.

Also on the "to do" list would be enhancing dl-minimal.c to the point where
it _could_ hand-off to the main arena, having kept some minimal chunk book
keeping that would allow glibc's free to work with those chunks. That would
simplify a lot of weird logic in the dynamic loader to detect which allocator
was used.

> Okay to check in?

Yes. Thanks.
 
> Thanks,
> Florian
> 
> 
> minimal-malloc.patch
> 
> 
> elf: dl-minimal malloc needs to respect fundamental alignment
> 
> The dynamic linker currently uses __libc_memalign for TLS-related
> allocations.  The goal is to switch to malloc instead.  If the minimal
> malloc follows the ABI fundamental alignment, we can assume that malloc
> provides this alignment, and thus skip explicit alignment in a few
> cases as an optimization.
> 
> It was requested on libc-alpha that MALLOC_ALIGNMENT should be used,
> although this results in wasted space if MALLOC_ALIGNMENT is larger
> than the fundamental alignment.  (The dynamic linker cannot assume
> that the non-minimal malloc will provide an alignment of
> MALLOC_ALIGNMENT; the ABI provides _Alignof (max_align_t) only.)
> 
> 2016-08-03  Florian Weimer  <fweimer@redhat.com>
> 
> 	* malloc/malloc.c (INTERNAL_SIZE_T, SIZE_SZ, MALLOC_ALIGNMENT)
> 	(MALLOC_ALIGN_MASK): Move ...
> 	* malloc/malloc-internal.h: ... to here.
> 	* elf/dl-minimal.c (malloc): Allocate with MALLOC_ALIGNMENT.
> 
> diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c
> index c8a8f8d..6034b5a 100644
> --- a/elf/dl-minimal.c
> +++ b/elf/dl-minimal.c
> @@ -27,6 +27,7 @@
>  #include <sys/types.h>
>  #include <ldsodefs.h>
>  #include <_itoa.h>
> +#include <malloc/malloc-internal.h>
>  
>  #include <assert.h>
>  
> @@ -90,7 +91,7 @@ __libc_memalign (size_t align, size_t n)
>  void * weak_function
>  malloc (size_t n)
>  {
> -  return __libc_memalign (sizeof (double), n);
> +  return __libc_memalign (MALLOC_ALIGNMENT, n);
>  }
>  
>  /* We use this function occasionally since the real implementation may
> diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
> index 98afd14..a3df8c3 100644
> --- a/malloc/malloc-internal.h
> +++ b/malloc/malloc-internal.h
> @@ -19,6 +19,59 @@
>  #ifndef _MALLOC_INTERNAL_H
>  #define _MALLOC_INTERNAL_H
>  
> +#include <malloc-machine.h>
> +#include <malloc-sysdep.h>
> +
> +/* INTERNAL_SIZE_T is the word-size used for internal bookkeeping of
> +   chunk sizes.
> +
> +   The default version is the same as size_t.
> +
> +   While not strictly necessary, it is best to define this as an
> +   unsigned type, even if size_t is a signed type. This may avoid some
> +   artificial size limitations on some systems.
> +
> +   On a 64-bit machine, you may be able to reduce malloc overhead by
> +   defining INTERNAL_SIZE_T to be a 32 bit `unsigned int' at the
> +   expense of not being able to handle more than 2^32 of malloced
> +   space. If this limitation is acceptable, you are encouraged to set
> +   this unless you are on a platform requiring 16byte alignments. In
> +   this case the alignment requirements turn out to negate any
> +   potential advantages of decreasing size_t word size.
> +
> +   Implementors: Beware of the possible combinations of:
> +     - INTERNAL_SIZE_T might be signed or unsigned, might be 32 or 64 bits,
> +       and might be the same width as int or as long
> +     - size_t might have different width and signedness as INTERNAL_SIZE_T
> +     - int and long might be 32 or 64 bits, and might be the same width
> +
> +   To deal with this, most comparisons and difference computations
> +   among INTERNAL_SIZE_Ts should cast them to unsigned long, being
> +   aware of the fact that casting an unsigned int to a wider long does
> +   not sign-extend. (This also makes checking for negative numbers
> +   awkward.) Some of these casts result in harmless compiler warnings
> +   on some systems.  */
> +#ifndef INTERNAL_SIZE_T
> +# define INTERNAL_SIZE_T size_t
> +#endif
> +
> +/* The corresponding word size.  */
> +#define SIZE_SZ (sizeof (INTERNAL_SIZE_T))
> +
> +/* MALLOC_ALIGNMENT is the minimum alignment for malloc'ed chunks.  It
> +   must be a power of two at least 2 * SIZE_SZ, even on machines for
> +   which smaller alignments would suffice. It may be defined as larger
> +   than this though. Note however that code and data structures are
> +   optimized for the case of 8-byte alignment.  */
> +#ifndef MALLOC_ALIGNMENT
> +# define MALLOC_ALIGNMENT (2 * SIZE_SZ < __alignof__ (long double) \
> +                           ? __alignof__ (long double) : 2 * SIZE_SZ)
> +#endif
> +
> +/* The corresponding bit mask value.  */
> +#define MALLOC_ALIGN_MASK (MALLOC_ALIGNMENT - 1)
> +
> +
>  /* Called in the parent process before a fork.  */
>  void __malloc_fork_lock_parent (void) internal_function attribute_hidden;
>  
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 1f5f166..bb52b3e 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -173,8 +173,6 @@
>      Changing default word sizes:
>  
>      INTERNAL_SIZE_T            size_t
> -    MALLOC_ALIGNMENT           MAX (2 * sizeof(INTERNAL_SIZE_T),
> -				    __alignof__ (long double))
>  
>      Configuration and functionality options:
>  
> @@ -216,9 +214,6 @@
>  #include <stdlib.h>   /* for getenv(), abort() */
>  #include <unistd.h>   /* for __libc_enable_secure */
>  
> -#include <malloc-machine.h>
> -#include <malloc-sysdep.h>
> -
>  #include <atomic.h>
>  #include <_itoa.h>
>  #include <bits/wordsize.h>
> @@ -304,64 +299,6 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
>  
>  
>  /*
> -  INTERNAL_SIZE_T is the word-size used for internal bookkeeping
> -  of chunk sizes.
> -
> -  The default version is the same as size_t.
> -
> -  While not strictly necessary, it is best to define this as an
> -  unsigned type, even if size_t is a signed type. This may avoid some
> -  artificial size limitations on some systems.
> -
> -  On a 64-bit machine, you may be able to reduce malloc overhead by
> -  defining INTERNAL_SIZE_T to be a 32 bit `unsigned int' at the
> -  expense of not being able to handle more than 2^32 of malloced
> -  space. If this limitation is acceptable, you are encouraged to set
> -  this unless you are on a platform requiring 16byte alignments. In
> -  this case the alignment requirements turn out to negate any
> -  potential advantages of decreasing size_t word size.
> -
> -  Implementors: Beware of the possible combinations of:
> -     - INTERNAL_SIZE_T might be signed or unsigned, might be 32 or 64 bits,
> -       and might be the same width as int or as long
> -     - size_t might have different width and signedness as INTERNAL_SIZE_T
> -     - int and long might be 32 or 64 bits, and might be the same width
> -  To deal with this, most comparisons and difference computations
> -  among INTERNAL_SIZE_Ts should cast them to unsigned long, being
> -  aware of the fact that casting an unsigned int to a wider long does
> -  not sign-extend. (This also makes checking for negative numbers
> -  awkward.) Some of these casts result in harmless compiler warnings
> -  on some systems.
> -*/
> -
> -#ifndef INTERNAL_SIZE_T
> -#define INTERNAL_SIZE_T size_t
> -#endif
> -
> -/* The corresponding word size */
> -#define SIZE_SZ                (sizeof(INTERNAL_SIZE_T))
> -
> -
> -/*
> -  MALLOC_ALIGNMENT is the minimum alignment for malloc'ed chunks.
> -  It must be a power of two at least 2 * SIZE_SZ, even on machines
> -  for which smaller alignments would suffice. It may be defined as
> -  larger than this though. Note however that code and data structures
> -  are optimized for the case of 8-byte alignment.
> -*/
> -
> -
> -#ifndef MALLOC_ALIGNMENT
> -# define MALLOC_ALIGNMENT       (2 * SIZE_SZ < __alignof__ (long double) \
> -				 ? __alignof__ (long double) : 2 * SIZE_SZ)
> -#endif
> -
> -/* The corresponding bit mask value */
> -#define MALLOC_ALIGN_MASK      (MALLOC_ALIGNMENT - 1)
> -
> -
> -
> -/*
>    REALLOC_ZERO_BYTES_FREES should be set if a call to
>    realloc with zero bytes should be the same as a call to free.
>    This is required by the C standard. Otherwise, since this malloc