Bug 466104 - aligned_alloc problems, part 1
Summary: aligned_alloc problems, part 1
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.21 GIT
Platform: Other All
: NOR normal
Target Milestone: ---
Assignee: Paul Floyd
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-02-19 21:18 UTC by Paul Floyd
Modified: 2023-03-06 21:00 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Floyd 2023-02-19 21:18:51 UTC
There are several issues with the handling of memalign, posix_memalign and aligned_alloc, mostly on non-Linux-glibc systems.

Some arguments are missing TRIGGER_MEMCHECK_ERROR_IF_UNDEFINED
memalign does not set errno correctly
The posix_memalign wrapper just calls the memalign one. That adds noise to the callstacks and makes it difficult to cutomize behaviour for different platforms.

memcheck/tests/memalign2.c fails with an assert on at least FreeBSD

I've mostly been looking at glibc musl and FreeBSD / memalign and posix_memalign
I'll look more at Solaris (Darwin if I can) / aligned alloc.
Comment 1 Paul Floyd 2023-02-20 06:52:52 UTC
One other potential inconsistency is the minimum size of an allocation. VG_MIN_MALLOC_SZB is 16 on most 64bit platforms and 8 on most 32bit platforms. I don't think that causes problems but there ought to be a comment in the code.

memcheck/tests/memalign2.c does a lot of tests like

p = memalign(1, 100);      assert(0 == (long)p % 8);

These tests pass even when the allocation fails (since NULL % 8 is 0. I'll update them to also check that p is not NULL
Comment 2 Paul Floyd 2023-02-20 09:28:21 UTC
More details. glibc memalign is here
https://elixir.bootlin.com/glibc/glibc-2.37/source/malloc/malloc.c#L3504

  if (alignment <= MALLOC_ALIGNMENT)
    return __libc_malloc (bytes);

  /* Otherwise, ensure that it is at least a minimum chunk size */
  if (alignment < MINSIZE)
    alignment = MINSIZE;

  /* If the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a
     power of 2 and will cause overflow in the check below.  */
  if (alignment > SIZE_MAX / 2 + 1)
    {
      __set_errno (EINVAL);
      return 0;
    }

What are those macros?
#define MALLOC_ALIGNMENT (2 * SIZE_SZ < __alignof__ (long double) \
			  ? __alignof__ (long double) : 2 * SIZE_SZ)

SIZE_SZ is the size of size_t, 8 on 64bit and 4 on 32bit.

alignof long double is 4 on x86 16 on amd64 16 on aarch64 8/16 for MIPS32/64 and 16 for PPC. I don't think that the condition is true on any platform we support so this is 8 on 32bit and 16 on 64bit.



MINSIZE, another macro rabbit hole

#define MINSIZE  \
  (unsigned long)(((MIN_CHUNK_SIZE+MALLOC_ALIGN_MASK) & ~MALLOC_ALIGN_MASK))

MIN_CHUNK_SIZE is 16/32 on 32/64 bit systems.
MALLOC_ALIGN_MASK is MALLOC_ALIGNMENT - 1, so 7 or 15
I make that 16 for 32bit systems and 32 for 64bit systems.

SIZE_MAX is in stdint.h. I can add a VKI_SIZE_MAX.
Comment 3 Paul Floyd 2023-02-20 22:54:59 UTC
The limit for alignment in jemalloc is much higher than Valgrind supports.

The Valgrind limit is 16M (all word sizes).

jemalloc i386 caps the alignment at 7 times this (112M). The amd64 cap is huge, 0x7'000'000'000'000'000 or 1,000,000T.

I'll what happens between these values and reasonable values.
Comment 4 Paul Floyd 2023-02-22 08:08:58 UTC
musl memalign seems to behave like jemalloc / FreeBSD

Illumos is different

https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/gen/memalign.c

It doesn't accept a size of 0. Min alignment is 8. What are MAX_ALIGN MAX_MALLOC MINSIZE ?

#define	MINSIZE		(sizeof (TREE) - sizeof (WORD))
#define	MAX_MALLOC (size_t)(SIZE_MAX - CORESIZE - 3 * ALIGN) /* overflow chk */
#define	MAX_ALIGN	(1 + (size_t)SSIZE_MAX)

nbytes gets clamped to MINSIZE
align gets bumped up to be the next power of 2 above MINSIZE + WORDSIZE
Comment 5 Paul Floyd 2023-02-22 12:36:57 UTC
Ilumos cont'd

TREE is a struct of size 6 words, so MINSIZE is 5 words

usr/src/uts/common/sys/int_limits.h:#define     SIZE_MAX        18446744073709551615UL
usr/src/uts/common/sys/int_limits.h:#define     SIZE_MAX        4294967295UL

usr/src/lib/libc/port/gen/mallint.h:#define     CORESIZE        (1024*ALIGN)

ALIGN is 16 / 8 on 64/32 bit.

usr/src/head/limits.h:#define   SSIZE_MAX       LONG_MAX        /* max value of an "ssize_t" */

And the 64bit long  max as expected
usr/src/boot/sys/x86/include/_limits.h:#define  __LONG_MAX      0x7fffffffffffffff      /* max for a long */
32 bit

I think that's enough to work everything out on Illumos
And hope that Solaris is the same.
usr/src/boot/sys/x86/include/_limits.h:#define  __LONG_MAX      0x7fffffffL
Comment 6 Paul Floyd 2023-02-28 14:51:24 UTC
I've pushed a change that largely clears up memalign.
A few @todos, see the commit message for more info:
commit e862c6f3d2374beebf78b52dfc8303ff33707001
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Tue Feb 28 13:17:24 2023 +0100

In short
Large alignment values, runtime configuration.

Next up, posix_memalign.

This has a spec! And the spec doesn't look like nonsense!

The only thing that is implementation defined is what to do for a size of 0.That can either return NULL or some allocation of an undefined size.
Comment 7 Paul Floyd 2023-02-28 22:04:40 UTC
commit 33ce1bf1cb01b49a441e43a11f5c2de21712e60b
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Tue Feb 28 21:21:05 2023 +0100

That's posix_memalign. Just leaves 'aligned_alloc'
Comment 8 Paul Floyd 2023-03-01 07:40:27 UTC
Before that, there are a few memalign_args tests failures in the nightlies
Fedora 36 s390
gcc110 ppc64
gcc112 ppc64le

-> Fixed
Comment 9 Paul Floyd 2023-03-05 16:27:35 UTC
I think that aligned_allloc is now just about done.
Comment 10 Paul Floyd 2023-03-06 21:00:16 UTC
I haven't much changed Linux glibc, but just about all the other platforms have been overhauled.

The behaviour is still static. However it shouldn't be too difficult to refactor the present code to make it runtime configurable. If ever we do that then we could have a set of bitmasks that would correspond to things like VG_MEMALIGN_ALIGN_POWER_TWO (there 1 to 4 per function). Then we could have either specific options or options like --malloc-lib=tcmalloc (or both).

That would add a bit of overhead - the current code with static macros should optimize out all the ones that are turned off.

commit e8d4d64e46cde7507c3716ad8094d886e8824fa4 (HEAD -> master, origin/master, origin/HEAD)
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Mon Mar 6 21:50:01 2023 +0100

    Bug 466104 - aligned_alloc problems, part 1
    
    I think that these are all now done.
    This commit refactors memalign and updates manual-core.xml
    to say some behaviour of Valgrind depends on the build time
    OS and libraries.