microsoft / snmalloc

Message passing based allocator
MIT License
1.58k stars 109 forks source link

aligned_alloc doesn't permit alignments less than `sizeof(uintptr_t)` #668

Closed jrtc27 closed 3 months ago

jrtc27 commented 4 months ago

C11 says:

The value of alignment shall be a valid alignment supported by the implementation and the value of size shall be an integral multiple of alignment.

where:

Alignments are represented as values of the type size_t. Valid alignments include only those values returned by an _Alignof expression for fundamental types, plus an additional implementation-defined set of values, which may be empty. Every valid alignment value shall be a nonnegative integral power of two.

However, snmalloc checks for alignment < sizeof(uintptr_t) and returns an error if so: https://github.com/microsoft/snmalloc/blob/6af38acd94d401313b7237bb7c2df5e662e135cf/src/snmalloc/global/libc.h#L148-L152

mjp41 commented 4 months ago

snmalloc only requires it to be a non-zero and a power of 2. I think as the comment in related issue says:

"It sounds like snmalloc's aligned_alloc is inheriting the following constraint from posix_memalign:"

I always tie myself in knots over which things I have to forbid due to the standard, and which things I can allow. I think posix_memalign said I needed to forbid this, but I am happy to be more permissive.

I am deep in some non-snmalloc things at the moment, but I can review the PR, if someone files it.

Thanks

bsdjhb commented 3 months ago

This does look like a regression in commit 6cbc50fe2c255b5b47ea63f4cf759a6a20f6e7a6. Prior to that memalign() (which aligned_alloc() is a simpler wrapper of) only required the alignment not be 0 and not be -1, and posix_memalign() explicitly checked for sizeof(uintptr_t). Now both memalign() and posix_memalign() check against sizeof(uintptr_t). I think memalign() should just go back to checking against 0. I'll draft up a PR in a bit.