Re: [PATCH] Properly align memory allocations and temporary buffers

From: René Scharfe <l.s.r@web.de>
Date: Jan 12, 2022, 15:47 UTC
Message-ID: <c4de043e-45e1-6066-37d5-6b06b7a820f7@web.de>
In-Reply-To: <641A33F3-E98A-40B1-BB11-ADC8C1FECA75@jrtc27.com>

Am 12.01.22 um 14:58 schrieb Jessica Clarke:

Show 7 quoted lines

>
> I can see the alternative fixes for qsort are now in next, as is the
> cleanup of register_symlink_changes to use string sets, which just
> leaves the mem-pool.c change to not assume that uintmax_t is
> sufficiently aligned for every type that git will use. If I move
> GIT_MAX_ALIGNMENT and its helper aggregates to mem-pool.c would that be
> acceptable?

Defining it at its single site of use sounds like a good idea to me. We can export it to git-compat-util.h later, iff necessary.

Show 18 quoted lines

>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index 5fa54a7afe..28581a45c5 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -274,6 +274,17 @@ typedef unsigned long uintptr_t;
>> #define _ALL_SOURCE 1
>> #endif
>>
>> +typedef union {
>> +	uintmax_t max_align_uintmax;
>> +	void *max_align_pointer;
>> +} git_max_align;
>> +
>> +typedef struct {
>> +	char unalign;
>> +	git_max_align aligned;
>> +} git_max_alignment;
>> +#define GIT_MAX_ALIGNMENT offsetof(git_max_alignment, aligned)

Style nit: We tend to use typedef sparingly. And the union type doesn't even need a name. E.g. this would work as well, while reducing the name space footprint:

   struct git_max_alignment {
      char unalign;
      union {
         uintmax_t max_align_uintmax;
         void *max_align_pointer;
      } aligned;
   };
   #define GIT_MAX_ALIGNMENT offsetof(struct git_max_alignment, aligned)

When someone uses a mempool for objects that requires 128-bit alignment (e.g. because it includes a long double or uint128_t member) then we'd need add long double to the union as well, like e.g. compat/obstack.c does. That would be the safe route, but currently only add 8 bytes of unnecessary overhead to each allocation.

Perhaps mempool alignment should be configurable. For now a comment might suffice which indicates that GIT_MAX_ALIGNMENT is only the maximum alignment requirement of current mempool users, not the highest possible requirement for the machine. Renaming it to something with MEMPOOL in the name would help in this regard as well.

Show 25 quoted lines

>> +
>> /* used on Mac OS X */
>> #ifdef PRECOMPOSE_UNICODE
>> #include "compat/precompose_utf8.h"
>> diff --git a/mem-pool.c b/mem-pool.c
>> index ccdcad2e3d..748eff925a 100644
>> --- a/mem-pool.c
>> +++ b/mem-pool.c
>> @@ -69,9 +69,9 @@ void *mem_pool_alloc(struct mem_pool *pool, size_t len)
>> 	struct mp_block *p = NULL;
>> 	void *r;
>>
>> -	/* round up to a 'uintmax_t' alignment */
>> -	if (len & (sizeof(uintmax_t) - 1))
>> -		len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
>> +	/* round up to a 'GIT_MAX_ALIGNMENT' alignment */
>> +	if (len & (GIT_MAX_ALIGNMENT - 1))
>> +		len += GIT_MAX_ALIGNMENT - (len & (GIT_MAX_ALIGNMENT - 1));
>>
>> 	if (pool->mp_block &&
>> 	    pool->mp_block->end - pool->mp_block->next_free >= len)
>> --
>> 2.33.1
>>
>