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

[Jessica Clarke <jrtc27@jrtc27.com>]

On 12 Jan 2022, at 15:47, René Scharfe <l.s.r@web.de> wrote:
> 
> 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.

Sure, that all makes sense.

Jess

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