[PATCH] malloc: Improve memalign alignment handling

[Wilco Dijkstra]

Hi Paul,

> Surely you meant stdc_leading_zeros not stdc_count_zeros, as the latter
> is a population count. Similarly elsewhere in the patch.

Good find - I think this was a bad search&replace...

> Given that the patch passed regression tests, that argues the code isn't
> being tested enough....

No kidding! I've added a testcase to ensure it gets tested, and that found
bugs in the mcheck checker. So who checks the checkers, and which
checker checks the checker???

> Less important: why is that (size_t) cast needed? MAX_TCACHE_SMALL_SIZE
> is already of type size_t, no?

It is indeed, but how are we sure it doesn't ever change? So the size_t is
explicit to show it must be a size_t (unfortunately there is no _sz variant
of the stdbit macros).

> +      alignment = (size_t) 2 << (stdc_count_zeros ((size_t) 1)
> +                                - stdc_count_zeros (alignment));
>
> Simpler and clearer (and more correct) is the following, which works
> because ALIGNMENT is nonzero and is not a power of 2:
>
>    alignment = (size_t) 2 << (stdc_bit_width (alignment) - 1);

Nice, I've used that in v2.

> -  if (alignment % sizeof (void *) != 0
> -      || !powerof2 (alignment / sizeof (void *))
> -      || alignment == 0)
> +  if (alignment < sizeof (void *) || !powerof2 (alignment))
>
> This assumes sizeof (void *) is a power of 2, which is a reasonable
> assumption but that should be stated by adding this:
>
>    static_assert (powerof2 (sizeof (void *)));

We could add tests for this as static asserts, but it's a fundamental assumption
in malloc (mentioned at the start) and true for all existing targets. The existing
implementation could never align odd sized pointers.

Note POSIX requires alignment to be both a power of 2 and a multiple of
sizeof (void *), so it also requires that the pointer size must be a power of 2.

Cheers,
Wilco