ooc-lang / rock

:ocean: self-hosted ooc compiler that generates c99
http://ooc-lang.org/
MIT License
403 stars 40 forks source link

Threads broken on msys2/mingw32? #924

Open exelotl opened 9 years ago

exelotl commented 9 years ago

Sorry if this is just a mistake on my end! After following the setup instructions over at oh-god-windows.md everything works fine, except any program using the Thread class crashes with

Fatal error in GC: Collecting from unknown thread

even if no allocation occurred inside the threads.

I tested using the example from the docs:

import structs/ArrayList
import threading/Thread

counter := 0

mutex := Mutex new()

threads := ArrayList<Thread> new()
main: func {
  for (i in 0..10) {
    threads add(Thread new(||
      for (i in 0..1000) {
        mutex lock()
        counter += 1   
        mutex unlock()
        Thread yield()
      }
    ))
  }

  for (t in threads) t start()
  for (t in threads) t wait()

  // prints counter = ???
  "counter = %d" printfln(counter)
}
alexnask commented 9 years ago

I can confirm, I get the same error on my compilation.

fasterthanlime commented 9 years ago

I don't think the GC is compiled with threading support anymore on Windows.

You can try doing

# in rock's folder
make boehmgc-clean
make boehmgc GC_FLAGS="--enable-threads=win32"

# back to ~/ooc/tests
rock -x
rock threads_test
exelotl commented 9 years ago

Nope, no luck for me, but wasn't that flag already specified when we ran GC_FLAGS="--build=i686-pc-mingw32 --enable-threads=win32" make rescue?

fasterthanlime commented 9 years ago

already specified when we ran

Ahh right, forgot about that. Perhaps it's the SDK's fault then, not registering threads with the GC properly? Because of some version block weirdness maybe?

alexnask commented 8 years ago

Apparently calling GC_endthreadex before the thread exits makes this work!
Here is my sample code that runs:

import structs/ArrayList
import threading/Thread

endthread: extern(GC_endthreadex) func(Int)

counter := 0

mutex := Mutex new()

threads := ArrayList<Thread> new()
main: func {
  for (i in 0..10) {
    threads add(Thread new(||
      for (i in 0..1000) {
        mutex lock()
        counter += 1   
        mutex unlock()
        Thread yield()
      }
      endthread(0)
    ))
  }

  for (t in threads) t start()
  for (t in threads) t wait()

  // prints counter = ???
  "counter = %d" printfln(counter)
}

It seems that for some reason the GC doesn't unregister threads properly, although I think this shouldn't be the case.

Quoting a part of GC_CreateThread documentation in gc.h:

Currently the collector expects all threads to fall through and
terminate normally, or call GC_endthreadex() or GC_ExitThread,
so that the thread is properly unregistered

It does seem that the threads in the code above should "fall through and terminate normally" though, so I don't see why they cause the GC to complain.

I will see if I can find out some other solution.

Otherwise wrapping the user's function in a closure that calls it then endthreadex is the only way I can see fixing this but it just feels and probably is wrong.

alexnask commented 8 years ago

Actually, the GC unregistering the thread may not be the issue, GC_endthreadex is defined as:

GC_API void GC_CALL GC_endthreadex(unsigned retval)
{
  GC_unregister_my_thread();
  _endthreadex(retval);
}

Calling GC_unregister_my_thread at the end of the thread still makes the program crash, while calling windows' _endthreadex fixes it.

alexnask commented 8 years ago

And for some reason I rebuilt boehmgc with -DDEBUGTHREADS and now it doesn't crash anymore >>

(And it still crashes when I recompile without DDEBUG_THREADS, that's really weird...)

alexnask commented 8 years ago

This is kind of crazy, I rebuilt the GC with:

make boehmgc-clean
ARCH=32 GC_FLAGS="CFLAGS='-DROCK_BUILD' --build=i686-pc-mingw32 --enable-threads=win32" make boehmgc

The program above no longer crashes.
Apparently for some reason if you build with any define, it works, while it doesn't without one (I tried adding additional flags like --enable-thread-local-alloc and --enable-parallel-mark as well and the program only stopped crashing if I added a define in the CFLAGS), so I'm guessing it's an issue with the Makefile not passing the GC_FLAGS correctly (?).

alexnask commented 8 years ago

I finally got it!

Boehm GC is compiled with -O2 by default but that seems to optimize something important out, leading to this bug.
Passing CFLAGS must override the default CFLAGS (which I think are -g -O2).
I tried compiling with -O1, it still crashes, passing CFLAGS='-g -O0' seems to do the trick.

Obviously, this is a Boehm GC bug, however it may have been fixed in more recent versions.

As a temporary workaround, I suggest fixing our makefile to pass the necessary flags on windows (which would also fix make rescue).

@geckojsc I would appreciate if you could test this too (just make boehmgc-clean && ARCH=32 GC_FLAGS="CFLAGS='-g -O0' --build=i686-pc-mingw32 --enable-threads=win32" make boehmgc) @fasterthanlime What do you think?

fasterthanlime commented 8 years ago

Oof, that's going to seriously impact performance of windows apps imho :(

alexnask commented 8 years ago

I know, should I try with a more recent version of Boehm GC?

It may have been fixed (although I didn't find anything similar in their issue tracker after a quick look)

Edit: Nope, tried latest version on their site (7.4.2), same issue there

refi64 commented 8 years ago

@shamanas Try it from Git: https://github.com/ivmai/bdwgc/.

exelotl commented 8 years ago

I thought that we switched to an old gc because we wanted to drop the windows pthreads dependency but the newer version didn't support win32-threads? How did you test with a newer version? Or am I talking nonsense? xD

alexnask commented 8 years ago

I haven't managed to compile from git yet (autotools on windows suck, I had some trouble so I moved on to another issue for now) but at least up to 7.4.2 they are available (and they seem to be available on the latest version on git too, look at win32_threads.c)

Also tried to recompile with gcc 5.2 (was using 4.9 before), still having the same issues.