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

["René Scharfe" <l.s.r@web.de>]

Am 12.01.22 um 14:58 schrieb Jessica Clarke:
>
> 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.

>> 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.

>> +
>> /* 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
>>
>