Implement fibers for win32

[straight-shoota]

This PR continues the efforts to bring concurrency features to windows (see #5430).

There are some refactorings to the the existing Fiber, Thread and EventLoop classes to extract the platform-specific implementation focused on POSIX and libevent.

Then it adds implementations of these classes for windows. Thread and EventLoop are only stubbed so far.
The core functionality is the context swap on win32.

concurrent_spec passes and channels work in general, but the channel spec seems to hang.

Supersedes #6955

[cfsamson]

I have been thinking a bit about the shadow space and added a comment with a suggestion of a small change that I think we need to make.

[straight-shoota]

@ysbaddaden in #6955 (comment) (answered here in the new PR):

About the rework: I meant that POSIX and Win32 don't need to share a base Thread class. The amount of duplicated code isn't large enough to justify the split. That would allow them to have their own internal details, so we treat them as "opaque classes".

Sure, we can do that. I don't know yet what the proper Thread implementation for win32 will look like, so I just did the minimal required thing to get fibers working.

We need to consider: Once we get proper multithreading, I suppose Thread might become a public API. I'm not sure if that's planned yet, but at least I think it would make sense to allow access to threads directly. In that case, we'll need to define a single public interface, like we do with other system-specific types.

I'd also like this rework to settle on where we should put specific classes, namely Thread, Thread::Mutex, Thread::ConditionVariable (pthread, win32), EventLoop (libevent, win32, then probably epoll, kqueue, ...) to avoid further conflicts.

Being system specific, I suppose they should be moved under src/crystal/system?

For threads, I guess they should be unix/pthread.cr, unix/pthread_mutex.cr, win32/thread.cr, and so on, then have src/thread.cr, src/thread/mutex.cr require the correct one?

Sounds good.

For the event loop, maybe unix/libevent.cr and win32/iocp.cr then have src/crystal/event_loop.cr require the correct one?

I'd like to include event_loop in the system-specific file names. So unix/event_loop_libevent.cr and win32/event_loop_iocp.cr.
For now, I've moved them all to src/crystal/event_loop because it's not exactly system-specific. Basically, the libevent bindings for unix should in theory work with win32 as well. But yeah, it probably still makes more sense to put that in src/crystal/system/{unix,win32}.

Just a question: would it be hard to bring real win32 thread support, instead of bringing a stub? I know that's not needed to implement fibers, but threads aren't needed on POSIX either, and it would be awesome to have a basic support for them.

I really don't know. Haven't looked into that yet. And I'd rather like to get fibers working right away because that's the limiting factor for porting other features as well.

[straight-shoota]

I pushed some updates but didn't move any files yet. I'm waiting for feedback on this before pushing things around.

[ysbaddaden]

@straight-shoota Thread is meant to stay has a private internal API, there is no plan to eventually make it public.

[straight-shoota]

I've moved Thread and EventLoop to crystal/system, split the x86_64 fiber context implementation and also squashed all those fixup commits.

[ysbaddaden]

It looks perfect now!

Renaming Thread as Crystal::Thread makes sense, but it may introduce annoying conflicts with other branches, and add some noise when using threads and mutexes, so I'll let @bcardiff decide whether we keep Thread for the time being, or just move to Crystal::Thread.

[straight-shoota]

Yes, exactly. It kind of makes sense to have Thread in the Crystal namespace, but that breaks some things. That's the reason why the tests keep failing because there are ever more places with broken references. It's probably worth it, though. When it's not intended to be public API, then it should go into the Crystal namespace like everything else.

The PR changes everything to Crystal::Thread but that's easy to swing back should we decide to keep it in the top level namespace.

[RX14]

A few minor things, but in general this looks awesome!

[RX14]

It would be more consistent to rename these Crystal::System::EventLoop, although it does make the diff even more noisy.

[straight-shoota]

I figured the Crystal::System namespace is only really useful when there are both a system-specific type and a general type in Crystal. Otherwise it only adds clutter.
There is however the possibility of name clashes with the compiler which also uses the Crystal namespace. But that's a general issue unless we put everything in Crystal::System.

[RX14]

I put everything which has a platform-specific implementation - and is using the new Crystal::System structure for it in Crystal::System.

I'm fine with not renaming, but I personally would have done it, since the files are in src/crystal/system.

[straight-shoota]

I see it that way that the things in Crystal::System are not used anywhere directly except as an implementation detail of their specific counterpart in the public API. EventLoop, Thread etc. however are general APIs for Crystal's runtime. They only have system-specific implementations, but they're directly used in the runtime. Crystal::EventLoop for example is referenced 10 times in the stdlib (without specs), Crystal::Thread 17 times.

[straight-shoota]

Rebased and squashed.

[RX14]

and CI failed

[ysbaddaden]

Unhandled exception: BUG: Thread.current returned NULL (Exception)

It fails to build crystal itself (ECR.embed for the init command).

[straight-shoota]

@ysbaddaden check_format fails while building the compiler, but that's the only thing being built in that spec. All test checks fail while executing ecr/process in a run macro for ecr_spec. ecr/process is probably just the first crystal program executed with the changed (local) standard library-

To reproduce the error directly:

$ bin/crystal run src/ecr/process.cr -- spec/std/data/test_template2.ecr
Unhandled exception: BUG: Crystal::Thread.current returned NULL (Exception)
  from src/raise.cr:191:1 in 'raise'
  from src/raise.cr:229:3 in 'raise'
  from src/crystal/system/unix/pthread.cr:64:3 in 'current'
  from src/crystal/scheduler.cr:17:5 in 'enqueue'
  from src/concurrent.cr:63:3 in 'spawn'
  from src/kernel.cr:540:1 in '__crystal_main'
  from src/crystal/main.cr:97:5 in 'main_user_code'
  from src/crystal/main.cr:86:7 in 'main'
  from src/crystal/main.cr:106:3 in 'main'
  from __libc_start_main
  from _start
  from ???

Out of a hunch, I re-inserted require "thread" into scheduler.cr (which seemed unnecessary at first), but it makes the error vanish. I guess there is some effect from require order. CI should pass now.

[RX14]

Another conflict?

[RX14]

would love another review

[asterite]

Looks great! I don't think this needs a thorough review (though I kind of did that, except for the asm part) because we still don't fully support Windows. Let's fill the missing pieces first. Once we can bootstrap the compiler on Windows we can tweak the little details.

[firejox]

The order it popped is different from the order it stored in makecontext. Is it correct?

[ysbaddaden]

I believe it's a documentation error of setting rbx in makecontext (or we push/pop rbx when we don't need to?)

We only pop up till rcx and the return address is left on the stack, I believe.

[firejox]

@ysbaddaden Well, I meant these popq %gs:0x08 and popq %gs:0x10 two instructions , not about rbx and rcx. Sorry for including too large region.

In makecontext, they store in the following order.

    # The following two values are stored in the Thread Information Block (NT_TIB)
    # and are used by Windows to track the current stack limits
    stack_ptr[-2] = @stack_bottom # %gs:0x08: Stack Bottom
    stack_ptr[-3] = @stack        # %gs:0x10: Stack Limit

Shouldn't the pop order be this?

  popq %gs:0x10
  popq %gs:0x08

[cfsamson]

I think you're right. GS:[0x08] is the "high address", and GS:[0x10] is the "low address" and if I'm not mistaken @stack_bottom is the high address, and @stack is the low address so they're put on the initial stack in the wrong order (or we need to change both push and pop orders in asm).

I would just change the initial setup to:

stack_ptr[-2] = @stack        # %gs:0x10: Stack Limit
stack_ptr[-3] = @stack_bottom # %gs:0x08: Stack Bottom

@ysbaddaden might be right that we don't need to push/pop %rcx on every switch. I don't have the setup here to confirm that, but it might be a few cycles wasted there.

[ysbaddaden]

Ah yes. They're inverted in makecontext. I totally missed that.