* [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment @ 2016-06-21 11:17 Florian Weimer 2016-06-21 12:54 ` H.J. Lu 2016-07-11 2:35 ` Carlos O'Donell 0 siblings, 2 replies; 15+ messages in thread From: Florian Weimer @ 2016-06-21 11:17 UTC (permalink / raw) To: libc-alpha If malloc is used instead of memalign for small alignments, malloc needs to provide the ABI-required alignment. 2016-06-21 Florian Weimer <fweimer@redhat.com> * elf/dl-minimal.c (malloc): Allocate with fundamental alignment. diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c index c8a8f8d..2d7d139 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 <stdalign.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 (_Alignof (max_align_t), n); } /* We use this function occasionally since the real implementation may ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-21 11:17 [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment Florian Weimer @ 2016-06-21 12:54 ` H.J. Lu 2016-06-21 12:57 ` Florian Weimer 2016-07-11 2:35 ` Carlos O'Donell 1 sibling, 1 reply; 15+ messages in thread From: H.J. Lu @ 2016-06-21 12:54 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library On Tue, Jun 21, 2016 at 4:17 AM, Florian Weimer <fweimer@redhat.com> wrote: > If malloc is used instead of memalign for small alignments, > malloc needs to provide the ABI-required alignment. > > 2016-06-21 Florian Weimer <fweimer@redhat.com> > > * elf/dl-minimal.c (malloc): Allocate with fundamental alignment. > > diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c > index c8a8f8d..2d7d139 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 <stdalign.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 (_Alignof (max_align_t), n); > } > > /* We use this function occasionally since the real implementation may We also have MALLOC_ALIGNMENT in malloc. Should they be consistent? -- H.J. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-21 12:54 ` H.J. Lu @ 2016-06-21 12:57 ` Florian Weimer 2016-06-21 13:00 ` H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2016-06-21 12:57 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library On 06/21/2016 02:54 PM, H.J. Lu wrote: > On Tue, Jun 21, 2016 at 4:17 AM, Florian Weimer <fweimer@redhat.com> wrote: >> If malloc is used instead of memalign for small alignments, >> malloc needs to provide the ABI-required alignment. >> >> 2016-06-21 Florian Weimer <fweimer@redhat.com> >> >> * elf/dl-minimal.c (malloc): Allocate with fundamental alignment. >> >> diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c >> index c8a8f8d..2d7d139 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 <stdalign.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 (_Alignof (max_align_t), n); >> } >> >> /* We use this function occasionally since the real implementation may > > We also have MALLOC_ALIGNMENT in malloc. Should they be consistent? MALLOC_ALIGNMENT is potentially larger. malloc/tst-malloc-thread-fail tests for alignment. To my knowledge, it passes on all regularly tested architectures after commit dea39b13e2958a7f0e75b5594a06d97d61cc439f. Thanks, Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-21 12:57 ` Florian Weimer @ 2016-06-21 13:00 ` H.J. Lu 2016-06-21 13:06 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2016-06-21 13:00 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library On Tue, Jun 21, 2016 at 5:57 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 06/21/2016 02:54 PM, H.J. Lu wrote: >> >> On Tue, Jun 21, 2016 at 4:17 AM, Florian Weimer <fweimer@redhat.com> >> wrote: >>> >>> If malloc is used instead of memalign for small alignments, >>> malloc needs to provide the ABI-required alignment. >>> >>> 2016-06-21 Florian Weimer <fweimer@redhat.com> >>> >>> * elf/dl-minimal.c (malloc): Allocate with fundamental alignment. >>> >>> diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c >>> index c8a8f8d..2d7d139 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 <stdalign.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 (_Alignof (max_align_t), n); >>> } >>> >>> /* We use this function occasionally since the real implementation may >> >> >> We also have MALLOC_ALIGNMENT in malloc. Should they be consistent? > > > MALLOC_ALIGNMENT is potentially larger. malloc/tst-malloc-thread-fail tests > for alignment. To my knowledge, it passes on all regularly tested > architectures after commit dea39b13e2958a7f0e75b5594a06d97d61cc439f. > MALLOC_ALIGNMENT is kind of mapped to the malloc alignment of a psABI. Shouldn't ld.so malloc have the same alignment of libc malloc? -- H.J. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-21 13:00 ` H.J. Lu @ 2016-06-21 13:06 ` Florian Weimer 2016-06-21 13:20 ` H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2016-06-21 13:06 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library On 06/21/2016 03:00 PM, H.J. Lu wrote: >> MALLOC_ALIGNMENT is potentially larger. malloc/tst-malloc-thread-fail tests >> for alignment. To my knowledge, it passes on all regularly tested >> architectures after commit dea39b13e2958a7f0e75b5594a06d97d61cc439f. > > MALLOC_ALIGNMENT is kind of mapped to the malloc alignment of > a psABI. Shouldn't ld.so malloc have the same alignment of libc malloc? I don't see why. MALLOC_ALIGNMENT has to match both the ABI constraint and the malloc/malloc.c implementation constraint (which requires a minimum alignment of 2 * sizeof (size_t)). Other mallocs do not have matching implementation constraints, and it is standard practice (in non-glibc mallocs) to lower the alignment for allocations which are smaller in size than _Alignof (max_align_t), although this is not compliant with C11. Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-21 13:06 ` Florian Weimer @ 2016-06-21 13:20 ` H.J. Lu 2016-06-23 15:01 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2016-06-21 13:20 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library On Tue, Jun 21, 2016 at 6:06 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 06/21/2016 03:00 PM, H.J. Lu wrote: > >>> MALLOC_ALIGNMENT is potentially larger. malloc/tst-malloc-thread-fail >>> tests >>> for alignment. To my knowledge, it passes on all regularly tested >>> architectures after commit dea39b13e2958a7f0e75b5594a06d97d61cc439f. >> >> >> MALLOC_ALIGNMENT is kind of mapped to the malloc alignment of >> a psABI. Shouldn't ld.so malloc have the same alignment of libc malloc? > > > I don't see why. MALLOC_ALIGNMENT has to match both the ABI constraint and > the malloc/malloc.c implementation constraint (which requires a minimum > alignment of 2 * sizeof (size_t)). My understanding is since the minimum constraint of malloc alignment <= ABI alignment, MALLOC_ALIGNMENT == ABI alignment. Do you have a glibc platform where it isn't true? > Other mallocs do not have matching implementation constraints, and it is > standard practice (in non-glibc mallocs) to lower the alignment for > allocations which are smaller in size than _Alignof (max_align_t), although > this is not compliant with C11. > > Florian -- H.J. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-21 13:20 ` H.J. Lu @ 2016-06-23 15:01 ` Florian Weimer 2016-06-23 16:15 ` H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2016-06-23 15:01 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library On 06/21/2016 03:20 PM, H.J. Lu wrote: > On Tue, Jun 21, 2016 at 6:06 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 06/21/2016 03:00 PM, H.J. Lu wrote: >> >>>> MALLOC_ALIGNMENT is potentially larger. malloc/tst-malloc-thread-fail >>>> tests >>>> for alignment. To my knowledge, it passes on all regularly tested >>>> architectures after commit dea39b13e2958a7f0e75b5594a06d97d61cc439f. >>> >>> >>> MALLOC_ALIGNMENT is kind of mapped to the malloc alignment of >>> a psABI. Shouldn't ld.so malloc have the same alignment of libc malloc? >> >> >> I don't see why. MALLOC_ALIGNMENT has to match both the ABI constraint and >> the malloc/malloc.c implementation constraint (which requires a minimum >> alignment of 2 * sizeof (size_t)). > > My understanding is since the minimum constraint of malloc alignment > <= ABI alignment, MALLOC_ALIGNMENT == ABI alignment. Do you > have a glibc platform where it isn't true? If I'm not mistaken, m68k has fundamental alignment 2 (the alignment of long, double, and long double), but our malloc still aligns to 8 (2 * sizeof (size_t)). Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-23 15:01 ` Florian Weimer @ 2016-06-23 16:15 ` H.J. Lu 2016-06-23 16:25 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2016-06-23 16:15 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library On Thu, Jun 23, 2016 at 8:01 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 06/21/2016 03:20 PM, H.J. Lu wrote: >> >> On Tue, Jun 21, 2016 at 6:06 AM, Florian Weimer <fweimer@redhat.com> >> wrote: >>> >>> On 06/21/2016 03:00 PM, H.J. Lu wrote: >>> >>>>> MALLOC_ALIGNMENT is potentially larger. malloc/tst-malloc-thread-fail >>>>> tests >>>>> for alignment. To my knowledge, it passes on all regularly tested >>>>> architectures after commit dea39b13e2958a7f0e75b5594a06d97d61cc439f. >>>> >>>> >>>> >>>> MALLOC_ALIGNMENT is kind of mapped to the malloc alignment of >>>> a psABI. Shouldn't ld.so malloc have the same alignment of libc malloc? >>> >>> >>> >>> I don't see why. MALLOC_ALIGNMENT has to match both the ABI constraint >>> and >>> the malloc/malloc.c implementation constraint (which requires a minimum >>> alignment of 2 * sizeof (size_t)). >> >> >> My understanding is since the minimum constraint of malloc alignment >> <= ABI alignment, MALLOC_ALIGNMENT == ABI alignment. Do you >> have a glibc platform where it isn't true? > > > If I'm not mistaken, m68k has fundamental alignment 2 (the alignment of > long, double, and long double), but our malloc still aligns to 8 (2 * sizeof > (size_t)). > Malloc alignment has been an isssie. GCC defines MALLOC_ABI_ALIGNMENT as #ifndef MALLOC_ABI_ALIGNMENT #define MALLOC_ABI_ALIGNMENT BITS_PER_WORD #endif which is incorrect for Linux and unusable: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36159 I'd like to see a reasonable solution to address malloc alignment on Linux, not just in ld.so. -- H.J. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-23 16:15 ` H.J. Lu @ 2016-06-23 16:25 ` Florian Weimer 2016-06-23 17:02 ` Joseph Myers 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2016-06-23 16:25 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library On 06/23/2016 06:15 PM, H.J. Lu wrote: > Malloc alignment has been an isssie. GCC defines > MALLOC_ABI_ALIGNMENT as > > #ifndef MALLOC_ABI_ALIGNMENT > #define MALLOC_ABI_ALIGNMENT BITS_PER_WORD > #endif > > which is incorrect for Linux and unusable: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36159 > > I'd like to see a reasonable solution to address malloc alignment > on Linux, not just in ld.so. But this glibc bug is fixed: https://sourceware.org/bugzilla/show_bug.cgi?id=6527 As far as I know, we know match or exceed the C11 max_align_t requirements. What we cannot do is to bump max_align_t alignment because it affects ABI. Overall, this entire concept of fundamental alignment is somewhat ill-defined. Over-aligned allocations with default operator new in C++ are a different matter and require C++ ABI changes, so that the C++ runtime can call the appropriate aligned allocation function. There are popular interposed mallocs which do not provide sufficient alignment, matching max_align_t, as well. Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-23 16:25 ` Florian Weimer @ 2016-06-23 17:02 ` Joseph Myers 2016-06-23 17:54 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Joseph Myers @ 2016-06-23 17:02 UTC (permalink / raw) To: Florian Weimer; +Cc: H.J. Lu, GNU C Library On Thu, 23 Jun 2016, Florian Weimer wrote: > As far as I know, we know match or exceed the C11 max_align_t requirements. > What we cannot do is to bump max_align_t alignment because it affects ABI. That's not obvious to me. It shouldn't affect the ABI of any function in glibc, for example; it's not the sort of type you embed in other structures. Increasing max_align_t to cover _Float128 (for 32-bit x86 as the sole affected case) is a question I left open in my GCC _FloatN / _FloatNx patch posting (not included in that patch, potentially relevant for a followup); that issue properly applies to _Decimal128 as well. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-23 17:02 ` Joseph Myers @ 2016-06-23 17:54 ` Florian Weimer 0 siblings, 0 replies; 15+ messages in thread From: Florian Weimer @ 2016-06-23 17:54 UTC (permalink / raw) To: Joseph Myers; +Cc: H.J. Lu, GNU C Library On 06/23/2016 07:02 PM, Joseph Myers wrote: > On Thu, 23 Jun 2016, Florian Weimer wrote: > >> As far as I know, we know match or exceed the C11 max_align_t requirements. >> What we cannot do is to bump max_align_t alignment because it affects ABI. > > That's not obvious to me. It shouldn't affect the ABI of any function in > glibc, for example; it's not the sort of type you embed in other > structures. I'm more worried about something using _Alignas (max_align_t), affecting struct layout. > Increasing max_align_t to cover _Float128 (for 32-bit x86 as > the sole affected case) is a question I left open in my GCC _FloatN / > _FloatNx patch posting (not included in that patch, potentially relevant > for a followup); that issue properly applies to _Decimal128 as well. If we hurry up, we could bump max_align_t on i386, true. The type and its implied promise is still fairly new, after all. But a path towards increasing the fundamental alignment from time to time is less clear to me. We also interpret the max_align_t requirement strictly in our malloc in the sense that we also apply it to small allocations. Other mallocs will happily return blocks with just 8-byte alignment on x86_64, where _Alignof (max_align_t) is 16. Maybe this something we could address in the definition of max_align_t in the C standard, but its definition is already very confusing. Surprisingly the impact on i386 wouldn't even be *that* severe. The minimum allocation size would still be 12 (for a total chunk size of 16). The size range from 13 to 20 is affected most prominently: The total chunk size would go from 24 bytes to 32 bytes, an increase of one third. But this is also the largest such increase, the subsequent ones are progressively smaller: def roundup(value, align): return (value + align - 1) / align * align def chunk_size(pointer_size, align, object_size): minimum_chunk_size = roundup(4 * pointer_size, align) size = roundup(object_size + pointer_size, align) return max(minimum_chunk_size, size) print "size old new increase" for size in range(0, 65): old_size = chunk_size(4, 8, size) new_size = chunk_size(4, 16, size) increase = (float(new_size) / old_size - 1) * 100 print " %2d %2d %2d %8.2f" % ( size, old_size, new_size, increase) size old new increase 0 16 16 0.00 1 16 16 0.00 2 16 16 0.00 3 16 16 0.00 4 16 16 0.00 5 16 16 0.00 6 16 16 0.00 7 16 16 0.00 8 16 16 0.00 9 16 16 0.00 10 16 16 0.00 11 16 16 0.00 12 16 16 0.00 13 24 32 33.33 14 24 32 33.33 15 24 32 33.33 16 24 32 33.33 17 24 32 33.33 18 24 32 33.33 19 24 32 33.33 20 24 32 33.33 21 32 32 0.00 22 32 32 0.00 23 32 32 0.00 24 32 32 0.00 25 32 32 0.00 26 32 32 0.00 27 32 32 0.00 28 32 32 0.00 29 40 48 20.00 30 40 48 20.00 31 40 48 20.00 32 40 48 20.00 Â… glibc malloc is not based on size groups for allocations, but fewer, more granular sizes will still help to reduce fragmentation somewhat. Maybe we should just make the change, document the increased alignment, and have GCC change their definition of max_align-t? The overhead would be more pronounced if we go from 16 bytes to 32 bytes alignment on 64-bit architectures. Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-06-21 11:17 [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment Florian Weimer 2016-06-21 12:54 ` H.J. Lu @ 2016-07-11 2:35 ` Carlos O'Donell 2016-07-11 8:55 ` Florian Weimer 1 sibling, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2016-07-11 2:35 UTC (permalink / raw) To: Florian Weimer, libc-alpha On 06/21/2016 07:17 AM, Florian Weimer wrote: > If malloc is used instead of memalign for small alignments, > malloc needs to provide the ABI-required alignment. > > 2016-06-21 Florian Weimer <fweimer@redhat.com> > > * elf/dl-minimal.c (malloc): Allocate with fundamental alignment. > > diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c > index c8a8f8d..2d7d139 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 <stdalign.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 (_Alignof (max_align_t), n); > } > > /* We use this function occasionally since the real implementation may > I agree with H.J. here, this should be MALLOC_ALIGNMENT, and if it's larger then so be it. It should logically match the behaviour, as best it can, of glibc's malloc since we're handing off most commonly to that malloc after relocation. Thus I'd like to see the behaviours harmonized. Other mallocs may not have the same behaviour but that's not a reason to avoid MALLOC_ALIGNMENT. The discussions about fixing libc malloc's alignment are out of scope for this change IMO. We should focus on fixing ld.so's behaviour. Out of curiosity have you tried to assemble a unit test for these functions based on linking directly with dl-minimal.os? It would be nice to run them through similar testing as is done by malloc. OK to checkin if you use MALLOC_ALIGMENT. Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-07-11 2:35 ` Carlos O'Donell @ 2016-07-11 8:55 ` Florian Weimer 2016-08-03 10:04 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2016-07-11 8:55 UTC (permalink / raw) To: Carlos O'Donell, libc-alpha On 07/11/2016 04:35 AM, Carlos O'Donell wrote: > I agree with H.J. here, this should be MALLOC_ALIGNMENT, and if it's larger > then so be it. It should logically match the behaviour, as best it can, of > glibc's malloc since we're handing off most commonly to that malloc after > relocation. Thus I'd like to see the behaviours harmonized. > > Other mallocs may not have the same behaviour but that's not a reason to > avoid MALLOC_ALIGNMENT. I'm still not convinced. Obviously, we cannot rely on MALLOC_ALIGNMENT anywhere because interposed mallocs may not provide it. So I still don't see the value of compatibility with the main malloc here. > The discussions about fixing libc malloc's alignment are out of scope for > this change IMO. We should focus on fixing ld.so's behaviour. > > Out of curiosity have you tried to assemble a unit test for these functions > based on linking directly with dl-minimal.os? It would be nice to run them > through similar testing as is done by malloc. > > 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. Thanks, Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-07-11 8:55 ` Florian Weimer @ 2016-08-03 10:04 ` Florian Weimer 2016-08-03 13:51 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2016-08-03 10:04 UTC (permalink / raw) To: Carlos O'Donell, libc-alpha, H.J. Lu, Joseph S. Myers [-- Attachment #1: Type: text/plain, Size: 469 bytes --] 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 [-- Attachment #2: minimal-malloc.patch --] [-- Type: text/x-patch, Size: 7349 bytes --] 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment 2016-08-03 10:04 ` Florian Weimer @ 2016-08-03 13:51 ` Carlos O'Donell 0 siblings, 0 replies; 15+ messages in thread From: Carlos O'Donell @ 2016-08-03 13:51 UTC (permalink / raw) To: Florian Weimer, libc-alpha, H.J. Lu, Joseph S. Myers 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 -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-08-03 13:51 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-21 11:17 [PATCH] elf: dl-minimal malloc needs to respect fundamental alignment Florian Weimer 2016-06-21 12:54 ` H.J. Lu 2016-06-21 12:57 ` Florian Weimer 2016-06-21 13:00 ` H.J. Lu 2016-06-21 13:06 ` Florian Weimer 2016-06-21 13:20 ` H.J. Lu 2016-06-23 15:01 ` Florian Weimer 2016-06-23 16:15 ` H.J. Lu 2016-06-23 16:25 ` Florian Weimer 2016-06-23 17:02 ` Joseph Myers 2016-06-23 17:54 ` Florian Weimer 2016-07-11 2:35 ` Carlos O'Donell 2016-07-11 8:55 ` Florian Weimer 2016-08-03 10:04 ` Florian Weimer 2016-08-03 13:51 ` Carlos O'Donell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).