Re: [PATCH] coroutine: add libucontext as external library

[Joelle van Dyne]

Good point, I only ran the PC BIOS but it did time out of 56 tests.

-j

On Wed, Mar 10, 2021 at 1:30 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Mar 09, 2021 at 01:21:29PM -0800, Joelle van Dyne wrote:
> > On Tue, Mar 9, 2021 at 10:24 AM Joelle van Dyne <j@getutm.app> wrote:
> > >
> > > On Tue, Mar 9, 2021 at 7:38 AM Stefan Hajnoczi <stefanha@redhat.com> 
> > > wrote:
> > > >
> > > > On Mon, Mar 08, 2021 at 07:26:36PM -0800, Joelle van Dyne wrote:
> > > > > iOS does not support ucontext natively for aarch64 and the 
> > > > > sigaltstack is
> > > > > also unsupported (even worse, it fails silently, see:
> > > > > https://openradar.appspot.com/13002712 )
> > > > >
> > > > > As a workaround we include a library implementation of ucontext and 
> > > > > add it
> > > > > as a build option.
> > > > >
> > > > > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > > > > ---
> > > > >  configure                 | 21 ++++++++++++++++++---
> > > > >  meson.build               | 12 +++++++++++-
> > > > >  util/coroutine-ucontext.c |  9 +++++++++
> > > > >  .gitmodules               |  3 +++
> > > > >  MAINTAINERS               |  6 ++++++
> > > > >  meson_options.txt         |  2 ++
> > > > >  subprojects/libucontext   |  1 +
> > > > >  7 files changed, 50 insertions(+), 4 deletions(-)
> > > > >  create mode 160000 subprojects/libucontext
> > > > >
> > > > > diff --git a/configure b/configure
> > > > > index 34fccaa2ba..5f225894a9 100755
> > > > > --- a/configure
> > > > > +++ b/configure
> > > > > @@ -1773,7 +1773,7 @@ Advanced options (experts only):
> > > > >    --oss-lib                path to OSS library
> > > > >    --cpu=CPU                Build for host CPU [$cpu]
> > > > >    --with-coroutine=BACKEND coroutine backend. Supported options:
> > > > > -                           ucontext, sigaltstack, windows
> > > > > +                           ucontext, libucontext, sigaltstack, 
> > > > > windows
> > > >
> > > > This approach mixes the concept of the coroutine backend (ucontext,
> > > > sigaltstack, etc) with the optional libucontext library dependency.
> > > >
> > > > libucontext is not a coroutine backend. The patch had to introduce
> > > > $coroutine_impl in addition to $coroutine in order to work around this.
> > > > Let's avoid combining these two independent concepts into
> > > > --with-coroutine=.
> > > >
> > > > I suggest treating libucontext as an optional library dependency in
> > > > ./configure with explicit --enable-libucontext/--disable-libucontext
> > > > options. Most of the time neither option will be provided by the user
> > > > and ./configure should automatically decide whether libucontext is
> > > > needed or not.
> > > >
> > > > > +case $coroutine in
> > > > > +libucontext)
> > > > > +  git_submodules="${git_submodules} subprojects/libucontext"
> > > > > +  mkdir -p libucontext
> > > >
> > > > Why is this mkdir necessary?
> > >
> > > That is a typo, will fix.
> > >
> > > Thanks to all the feedback in this thread. I will shelve this patchset
> > > for now and see if it's possible to fix ucontext on Darwin. Or if we
> > > go with gcoroutine that would work as well. Either way it seems like
> > > this isn't ready yet.
> > >
> > > -j
> >
> > The following is enough to get ucontext working on macOS 11 (Apple
> > seems to have fixed it when they added ARM64 support for M1 Macs).
> > However, ucontext still does not work (no symbols) on iOS so there's
> > not much point in switching from sigaltstack.
> >
> > -j
> >
> > diff --git a/configure b/configure
> > index a2736ecf16..042f4e87a5 100755
> > --- a/configure
> > +++ b/configure
> > @@ -774,7 +774,8 @@ Darwin)
> >    audio_possible_drivers="coreaudio sdl"
> >    # Disable attempts to use ObjectiveC features in os/object.h since they
> >    # won't work when we're compiling with gcc as a C compiler.
> > -  QEMU_CFLAGS="-DOS_OBJECT_USE_OBJC=0 $QEMU_CFLAGS"
> > +  # _XOPEN_SOURCE and _DARWIN_C_SOURCE needed for ucontext
> > +  QEMU_CFLAGS="-D_XOPEN_SOURCE=500 -D_DARWIN_C_SOURCE
> > -DOS_OBJECT_USE_OBJC=0 $QEMU_CFLAGS"
> >  ;;
> >  SunOS)
> >    solaris="yes"
> > @@ -4486,17 +4487,15 @@ fi
> >  # specific one.
> >
> >  ucontext_works=no
> > -if test "$darwin" != "yes"; then
> > -  cat > $TMPC << EOF
> > +cat > $TMPC << EOF
> >  #include <ucontext.h>
> >  #ifdef __stub_makecontext
> >  #error Ignoring glibc stub makecontext which will always fail
> >  #endif
> >  int main(void) { makecontext(0, 0, 0); return 0; }
> >  EOF
> > -  if compile_prog "" "" ; then
> > -    ucontext_works=yes
> > -  fi
> > +if compile_prog "" "" ; then
> > +  ucontext_works=yes
> >  fi
> >
> >  if test "$coroutine" = ""; then
>
> I have tried doing this before, and while it was enough for the compile
> to succeed, I found that tests failed / hung when running the macOS CI
> jobs.  Did you actually try running tests with this change directly,
> and/or under Cirrus CI ?
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>