[PATCH v3] malloc: Improve memalign alignment

[DJ Delorie]

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
>  __debug_memalign (size_t alignment, size_t bytes)
>  {
> +  alignment = stdc_bit_ceil (alignment);
> +  if (alignment == 0)
> +    {
> +      errno = EINVAL;
> +      return NULL;
> +    }
>    return _debug_mid_memalign (alignment, bytes, RETURN_ADDRESS (0));
>  }

This really needs a comment.  Only glibc defines the zero return value
(gcc et al say "undefined"), and I'm guessing from the docs that it only
returns zero if the alignment is higher than 0x8000...0000.  If so it
would be more obvious to do *that* comparison.

>  large_csize2tidx(size_t nb)
>  {
>    size_t idx = TCACHE_SMALL_BINS
> -	       + __builtin_clz (MAX_TCACHE_SMALL_SIZE)
> -	       - __builtin_clz (nb);
> +	       - stdc_bit_width (MAX_TCACHE_SMALL_SIZE)
> +	       + stdc_bit_width (nb);
>    return idx;
>  }

This is technically correct, but I wonder if its purpose would be more
obvious like this?

    size_t idx = TCACHE_SMALL_BINS
 	       + (stdc_bit_width (nb)
 	          - stdc_bit_width (MAX_TCACHE_SMALL_SIZE));

> @@ -3518,6 +3518,19 @@ libc_hidden_def (__libc_realloc)
>  void *
>  __libc_memalign (size_t alignment, size_t bytes)
>  {
> +  /* Round the alignment up to a power of 2, and report an error on overflow.
> +     Zero alignment is handled by _mid_memalign.  */
> +  if (__glibc_unlikely (!powerof2 (alignment)))
> +    {
> +      alignment = (size_t) 2 << (stdc_bit_width (alignment) - 1);

We're using std_bit_ceil elsewhere, why not here?

> @@ -3530,8 +3543,8 @@ aligned_alloc (size_t alignment, size_t bytes)
>  /* Similar to memalign, but starting with ISO C17 the standard
>     requires an error for alignments that are not supported by the
>     implementation.  Valid alignments for the current implementation
> -   are non-negative powers of two.  */
> +   are powers of two.  */

We do not support negative alignments.  Either this comment didn't mean
to you what it meant to me, or you added support for negative
alignments?

>    /* Test whether the SIZE argument is valid.  It must be a power of
>       two multiple of sizeof (void *).  */
> -  if (alignment % sizeof (void *) != 0
> -      || !powerof2 (alignment / sizeof (void *))
> -      || alignment == 0)
> +  if (alignment < sizeof (void *) || !powerof2 (alignment))

These two expressions do not do the same thing.  As noted, the original
supports sizeof(void*) not itself being a power of two.

I know we don't support sizeof(void*) not being a power of 2, but if
you're going to do this replacement, the comment needs to say that we're
making that assumption in order to simplify the logic.

> diff --git a/malloc/tst-memalign.c b/malloc/tst-memalign.c
> index 771dd9c62eb708a8f3bd5c508794abbf9aa2a54d..ade37091b8223bad5c223f8b73271d7aed90c8f1 100644
> --- a/malloc/tst-memalign.c
> +++ b/malloc/tst-memalign.c
> @@ -107,10 +107,10 @@ do_test (void)
>    free (p);
>  
>    /* Check the alignment of the returned pointer is correct.  */
> -  p = memalign (0x100, 10);
> +  p = memalign (129, 10);
>  
>    if (p == NULL)
> -    merror ("memalign (0x100, 10) failed.");
> +    merror ("memalign (129, 10) failed.");

You need to do this as a new test in that file, not modifying an old
test.  The existing test tests a normal case (alignment of 0x100, no
rounding needed); your change is testing the "rounding" of the
alignment, which is a separate (new) test.