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

From: Jessica Clarke <jrtc27@jrtc27.com>
Date: Jan 6, 2022, 22:56 UTC
Message-ID: <97D2AB55-0F8F-4C38-A5C6-1AAA43EA064A@jrtc27.com>
In-Reply-To: <xmqqh7agbiuq.fsf@gitster.g>

On 6 Jan 2022, at 22:27, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> 
> Taylor Blau <me@ttaylorr.com> writes:
> 
>> (+cc René as another possible reviewer)
>> 
>> On Wed, Jan 05, 2022 at 01:23:24PM +0000, Jessica Clarke wrote:
>>> Currently git_qsort_s allocates a buffer on the stack that has no
>>> alignment, and mem_pool_alloc assumes uintmax_t's size is adequate
>>> alignment for any type.
>>> 
>>> On CHERI, and thus Arm's Morello prototype, pointers are implemented as
>>> hardware capabilities which, as well as having a normal integer address,
>>> have additional bounds, permissions and other metadata in a second word,
>>> so on a 64-bit architecture they are 128-bit quantities, including their
>>> alignment requirements. Despite being 128-bit, their integer component
>>> is still only a 64-bit field, so uintmax_t remains 64-bit, and therefore
>>> uintmax_t does not sufficiently align an allocation.
> 
> Alignment aside, if uintmax_t is 64-bit but your pointer needs
> 128-bit to store, saving a pointer value in uintmax_t variable would
> not work correctly, I presume, as casting the 64-bit integral type
> back into pointer would not be sufficient to recover the lost
> information that used to be in the second word.
> 
> So, does the architecture have 128-bit uintptr_t and that is a safe
> type both from the point of view of alignment and from the point of
> view of not losing information?

Yes. It is basically just a char * that lets you perform arbitrary arithmetic; not an unsigned long any more for our architectures.

Show 6 quoted lines

> If that type is larger than uintmax_t, something smells wrong,
> though.  max is not max anymore at that point.
> 
> IIRC, uintptr_t is optional in C99, so a simpler solution to use the
> larger type between uintptr_t and uintmax_t as a replacement for how
> we use uintmax_t would not quite work out of the box X-<.

This is also true of uint128_t, it doesn’t fit in a uintmax_t either. uintmax_t was a mistake as it becomes part of the ABI and can never be revised even when new integer types come along. uintmax_t can hold any valid address, but will strip the metadata. It turns out almost no software tries to put a pointer in a uintmax_t and cast it back to a pointer.

Note that, even if we wanted to, we coldn't just map uintmax_t to a uintptr_t, as on 32-bit architectures uintmax_t is a 64-bit integer but uintptr_t only has a 32-bit range (plus 32 bits of metadata), and we do have a 32-bit CHERI variant for embedded use cases.

This is one of a few warts that people will just have to deal with for CHERI; there’s no way round it if you want anything that implements the ideas of CHERI.

Jess