> On 3 Dec 2024, at 18:28, Jason Merrill <ja...@redhat.com> wrote:
>
> On 9/18/24 4:36 PM, Arsen Arsenović wrote:
>> This patch implements support for frames and promises with new-extended
>> alignment.
>> There are two kinds of alignment to worry about here:
>> - Promise alignment, which causes "internal" padding inside the frame
>> struct. The reason this is a problem is because the (yet to be
>> formalized, but agreed upon) coroutine ABI requires us to place the
>
> Where is this "agreed upon" ABI described? AFAICT there's nothing publicly
> visible. Could someone add something to
> https://github.com/itanium-cxx-abi/cxx-abi/issues even if it isn't well
> drafted yet?
At the moment we just have notes in a shared google doc. it is on my TODO to
convert those into a PR on itanium-cxx-abi.
That was the agreement at the last meeting we had (probably in Cologne) but
neither GCC nor clang have implemented it and now we face an ABI break to do
so. I have some migivings about whether we should go ahead “retrospectively”
and think this needs to be re-discussed with that group.
I was hoping to raise this with Lewis et. al. in Wroclaw, but Lewis did not
attend.
If we can fix the underaligment of frames with > 2xsizeof(pointer) alignment
but initially preserve the current ABI that might be the best short-term
approach - and then I will take an action to poll the attendees of that group
for opinons on how now to proceed.
>> resume and destroy pointers right before the promise of a coroutine,
>> so that they are at offset -sizeof(void*)B and -2*sizeof(void*)B. To
>> this end, we might need to insert padding to the start of the frame
>> type in order to ensure that these pointers are aligned correctly.
>> - Frame alignment (either as a result of the above or some other field in
>> the frame), which is the alignment of the entire finished frame type.
>> This alignment currently (up to the standardization of P2014) requires
>> us to over-allocate and then round the result.
>
> Can't we go ahead with implementing P2014R2 even though the wording isn't
> finalized yet? The direction was unanimously approved by EWG.
>
>> In addition, this patch also alters __builtin_coro_promise, the builtin
>> that turns a promise pointer into a _M_fr_ptr of a coroutine handle.
>> This builtin took an additional alignment parameter it used to
>> compensate for the padding that'd happen between the resume/destroy
>> pointer and the promise. This is no longer necessary, as now these
>> pointers directly precede the frame.
>
> Could we add a bit of documentation of the frame struct to the comment at the
> top of the file? Is this the same thing that the standard refers to as "the
> coroutine state"?
>
> Even just Iain's sketch from
> https://github.com/llvm/llvm-project/issues/58397#issuecomment-1281983337would
> be an improvement over the nothing we have now.
the google doc contains pictures - but those are no more helpful to source code.
Iain
>
>> As a result of the resume pointer no longer being at the start of the
>> coroutine frame, our actors and destroys are changed. They now take
>> &frame->_Coro_resume_fn as the argument ("resume pointer"), and need to
>> adjust it to be the start of the frame before use.
>>
>> The way each of the kinds of padding above are handled is as follows:
>> - For promise alignment, we emit extra members before the resume
>> pointer: an "allocation pointer", if there's room for it, and a char[]
>> of appropriate size to ensure there's no room between the resume
>> pointer and the padding, and
>> - For frame alignment, we allocate extra memory in order to perform
>> alignment on our own and store the result in the allocation pointer,
>> which is after the frame if it could not be placed as part of the
>> process above.
>> The location and necessity of the allocation pointer is dictated by
>> coroutine_info::allocptr_expr and coroutine_info::alloc_store_pointer,
>> the former provides an lvalue for the allocation pointer based on the
>> frame pointer, iff rounding is to be performed (nullptr otherwise), and
>> the latter decides whether to allocate extra space for the allocation
>> pointer (if stored after the frame). This allows us flexibility to
>> later add support for P2014.
>
> I don't understand the need for the allocation pointer in the first place.
> If we know the offset from &resume_ptr when we create the object, why don't
> we also know it when we destroy the object? Isn't that already what
> build_resume_to_frame_expr does?
>
> Jason
