On 12/3/24 2:31 PM, Iain Sandoe wrote:
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.
Maybe for now add an issue with a link to the doc?
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.
Is overaligned frames still broken with clang as well?
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
