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