[mikael-s-persson]

This PR (along with dallison/cpp_toolbelt#27) is about decoupling the coroutine interface that functions/lambdas that run in a coroutine need from the actual scheduler implementation. This is mostly separating concerns and to some extent helps hide implementation details and flags that don't affect the code that runs in a coroutine context, only how the coroutines are scheduled and switched.

This could be controversial change because it breaks users since the concrete coroutine class that you construct and tie to a scheduler is now called ScheduledCoroutine, while Coroutine is just an abstract class now.

[dallison]

Would it be better to rename Coroutine to CoroutineBase keeping that abstract and then Coroutine is a concrete instance of it. Any functions that take a Coroutine* or Coroutine& would still work, and any code that creates Coroutine instances can still compile.

Unless that breaks you separation of concerns issue you are addressing. I am concerned about breaking existing code.

[mikael-s-persson]

Yeah, that might make more sense now. Originally, I tried to make Coroutine abstract so that all the code in toolbelt that takes Coroutine* parameters wouldn't have to depend on the full coroutine scheduler implementation. But, after that, I introduced the "Poller" abstraction in toolbelt, which effectively cuts that dependency out.

A potential "redux" version of this PR could simply be to make the original Coroutine class inherit from Poller and split the toolbelt/coroutine dependency that way.

Splitting the Coroutine class into two parts (in this PR: Coroutine and ScheduledCoroutine) is mostly nice in principle (and cleaner) but it's a hard breaking change with little practical effect.

[dallison]

That sounds better. I will be slow to respond for a week unfortunately. Personal situation to deal with.

[dallison]

Any progress on this Mik?

[mikael-s-persson]

Sorry for the delay. I've been busy and made changes on my side that made this change less critical.

I updated the PR to the "redux" version. There should be no breaking changes on users now. The main changes are:

• Coroutine derives from toolbelt::Poller
• All the various convenience overloads of poll/wait functions have moved Poller (see co::Coroutine -> toolbelt::AsyncWaiter cpp_toolbelt#27)
• A few trivial fly-by fixes.
• Added support for arm32 for that little longjmp trampoline. AFAIK, that should work.

To merge, we probably first need to merge the PR on toolbelt, then update this PR with the correct git url and commit, and then after that is merged, update the same on toolbelt (which only depends on co for its test suite now).

[dallison]

I'll do proper ARM support. The issue with longjmp is that asan doesn't like the stack changing. It thinks there is a bug when longjmp is used for switching between coroutines. I guess it's kind of an abuse of setjmp/longjmp. I'll write an assembly language context switcher for it.

[mikael-s-persson]

I'll do proper ARM support. The issue with longjmp is that asan doesn't like the stack changing. It thinks there is a bug when longjmp is used for switching between coroutines. I guess it's kind of an abuse of setjmp/longjmp. I'll write an assembly language context switcher for it.

Good to know. I guess the trampoline still makes sense in the meantime.

[dallison]

I have added the ARM context switcher, along with some others. This conflicted with your change. Do you mind resolving it?

[mikael-s-persson]

Resolved the conflicts.

I'm a bit skeptical of the arm assembly, are you sure this works in mixed mode between arm and thumb?

[dallison]

Why are you skeptical about it? Do you see any failures? It was tested on colima on my mac. I asked an agent and it says:

Yes, on ARM targets that support ARM/Thumb interworking.

The switcher itself forces ARM mode with .arm, but:

CoroutineSetContext enters saved code using bx r2, honoring the function pointer’s Thumb bit.
Thumb functions return via lr to the ARM-mode trampoline.
Calls from Thumb code to switcher functions use linker-generated interworking veneers.
Thumb-configured builds were cross-assembled successfully, though mixed-mode execution wasn’t runtime-tested. Cortex-M/Thumb-only targets are not supported because the switcher contains ARM instructions.

[mikael-s-persson]

Just making sure. What caught my eye was the longjmp bit (that I had originally added):

asm(
  SYM(__real_longjmp) ":\n"
  "b " SYM(longjmp) "\n");

My version went through r12 and bx specifically in case of mixed mode, and my AI told me that was needed. I guess it's only need if longjmp is not supplied in the same mode as the code is being compiled for.

[dallison]

I asked my agent (GPT 5.6 Sol) and this makes me sound smart:

Good question. I checked the generated relocations. In Thumb mode, b longjmp produces R_ARM_THM_JUMP24; in ARM mode it produces R_ARM_JUMP24. These relocations allow the linker to insert an ARM/Thumb interworking veneer or route through the appropriate PLT stub when the target is in the opposite mode.

Your r12/bx sequence explicitly performs the mode switch using bit 0 of the resolved address, so it is also valid. It would be necessary for an indirect address unknown to the linker, or with a linker that does not support normal EABI interworking. It is not generally necessary merely because longjmp was compiled in a different mode.

Also, the current ARM configuration uses the custom context switcher, so this relay is not exercised in normal ARM builds.

SInce we now support ARM context switches we won't use the longjmp.

[mikael-s-persson]

Sounds good. Who am I to question super intelligence.