Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for mcf thread model #659

Merged
merged 4 commits into from Oct 1, 2023
Merged

Add support for mcf thread model #659

merged 4 commits into from Oct 1, 2023

Conversation

starg2
Copy link
Contributor

@starg2 starg2 commented Sep 28, 2023

WIP. I think it's easier for us to discuss the issues when we have actual code.

  • I couldn't build 9004-crt-Copy-clock-and-nanosleep-from-winpthreads.patch so I commented it out. This patch seems unnecessary.
  • The CRT patches only apply to trunk and not v11. Fixed.
  • How do we bootstrap the first toolchain? Apparently we don't have to.
  • Update README.

Closes #658.

@lhmouse
Copy link
Contributor

lhmouse commented Sep 29, 2023

WIP. I think it's easier for us to discuss the issues when we have actual code.

  • I couldn't build 9004-crt-Copy-clock-and-nanosleep-from-winpthreads.patch so I commented it out.

What's the error?

While building libstdc++, its configure script checks for existence of nanosleep() and #define _GLIBCXX_USE_NANOSLEEP 1 in 'c++config.h', which is almost always the case.

However nanosleep() is implemented in winpthreads. This means that if a program calls std::this_thread::sleep_for() it will have to link against winpthreads. This patch provides nanosleep() in the main CRT library to get rid of such dependency.

  • The CRT patches only apply to trunk and not v11.

There may be conflicts. I think we can apply them on trunk, then cherry-pick them to the v11 branch, then re-format-patch them.

  • How do we bootstrap the first toolchain?

If we link GCC against the unpatched CRT, then no special action is necessary. GCC is single-threaded, so it doesn't matter what CRT itself is linked against; only GCC libraries (libgcc, libstdc++, libobjc, libgfortran, etc.) matter.

If we want to link GCC against the patched CRT which references __MCF_cxa_atexit() and __MCF_cxa_finalize(), then a conventional procedure for cross compilation shall apply:

  1. Build a GCC that links against the 'host' (unpatched) CRT and produces code linking against the 'target' (patched) CRT.
  2. Use this cross GCC to build the patched CRT.
  3. Install the patched CRT, so the 'host' is changed to patched CRT.
  4. Rebuild GCC, linking against the 'host' (patched ) CRT and produces code linking against the 'target' (patched) CRT. This is now a normal three-stage bootstrap.
  • Update README.

We may take the comments from https://www.mingw-w64.org/downloads/#gcc-with-the-mcf-thread-model but it's a bit out of date. I shall update it a little a few moments later.

@starg2
Copy link
Contributor Author

starg2 commented Sep 30, 2023

WIP. I think it's easier for us to discuss the issues when we have actual code.

  • I couldn't build 9004-crt-Copy-clock-and-nanosleep-from-winpthreads.patch so I commented it out.

What's the error?

I got a bunch of undeclared identifier errors like this:

../../src/mingw-w64/mingw-w64-crt/time/clock.c: In function 'clock_getres':
../../src/mingw-w64/mingw-w64-crt/time/clock.c:81:15: error: 'CLOCK_REALTIME' undeclared (first use in this function); did you mean 'NIF_REALTIME'?
   81 |     if (id == CLOCK_REALTIME && load_GetSystemTimeBestAsFileTime() == GetSystemTimeAsFileTime)
      |               ^~~~~~~~~~~~~~
      |               NIF_REALTIME

CLOCK_REALTIME is usually defined in pthread_time.h, but the one I found under $ROOT_DIR/runtime was dummy:

/* Dummy header, which gets overridden, if winpthread library gets installed.  */

I have no idea why a dummy header is installed.

While building libstdc++, its configure script checks for existence of nanosleep() and #define _GLIBCXX_USE_NANOSLEEP 1 in 'c++config.h', which is almost always the case.

However nanosleep() is implemented in winpthreads. This means that if a program calls std::this_thread::sleep_for() it will have to link against winpthreads. This patch provides nanosleep() in the main CRT library to get rid of such dependency.

Sounds like this patch is useful for the win32 thread model as well.

  • The CRT patches only apply to trunk and not v11.

There may be conflicts. I think we can apply them on trunk, then cherry-pick them to the v11 branch, then re-format-patch them.

I think we at least want to support the latest release branch (v11), so let's do that before merging this PR.

  • How do we bootstrap the first toolchain?

If we link GCC against the unpatched CRT, then no special action is necessary. GCC is single-threaded, so it doesn't matter what CRT itself is linked against; only GCC libraries (libgcc, libstdc++, libobjc, libgfortran, etc.) matter.

What about the prerequisites like zlib, libiconv, gmp, mpfr, etc? They are built before CRT.

@lhmouse
Copy link
Contributor

lhmouse commented Sep 30, 2023

I have no idea why a dummy header is installed.

Ah I see. The patch only move the implementation of nanosleep() into the main CRT, but the code still depends winpthreads headers. I have remembered why 'pthread_time.h' wasn't included in that patch: There are still libraries that require winpthreads (such as libgomp) so this header is still installed with winpthreads and not the main CRT.

Therefore the patch should be considered incomplete, as 'pthread_time.h' from winpthreads should be installed by hand before building the CRT.

Sounds like this patch is useful for the win32 thread model as well.

I suggest you check 'c++config.h' in the win32 thread model for sure. I have checked my Debian installation and it is not defined in '/usr/lib/gcc/x86_64-w64-mingw32/8.3-win32/include/c++/x86_64-w64-mingw32/bits/c++config.h'. It's either that they pass --disable-libstdcxx-time when configuring libstdc++, or they do not install winpthreads.

What about the prerequisites like zlib, libiconv, gmp, mpfr, etc? They are built before CRT.

Those can be rebuilt with the new GCC. however I don't rebuild them.

@lhmouse
Copy link
Contributor

lhmouse commented Sep 30, 2023

I'm considering to try --disable-libstdcxx-time and drop that nanosleep() patch. Not only because the patch is incomplete, it also looks like a hack and non-solution.

@starg2
Copy link
Contributor Author

starg2 commented Sep 30, 2023

CI finally ran successfully, so I checked c++config.h:

Thread model _GLIBCXX_USE_NANOSLEEP defined?
posix yes
win32 no
mcf no

We always pass --enable-libstdcxx-time=yes to configure though.

@lhmouse
Copy link
Contributor

lhmouse commented Sep 30, 2023

The result looks correct. Was it the case that winpthreads was not installed before building GCC? Failure to reference nanosleep() is likely to cause _GLIBCXX_USE_NANOSLEEP to be not defined.

Anyway I have filed a PR to MSYS2: msys2/MINGW-packages#18667

@starg2
Copy link
Contributor Author

starg2 commented Sep 30, 2023

We always install winpthreads before building gcc. Also, regardless of the target thread model, we use gcc with the posix thread model as host toolchains by default.

Build and install mcfgthread library when the mcf thread model is
selected.
@lhmouse
Copy link
Contributor

lhmouse commented Sep 30, 2023

I see. Here is a test program:
test.cc.txt

The C++ standard requires that all thread_local objects be destroyed before all static objects. With the posix or win32 thread model, this program outputs:

static_obj_type constructor: `this` = 00007ff633e6e030
thread_local constructor: `this` = 000002ab619d1d68, `00007ff633e6e030` is alive
main
static_obj_type destructor: `this` = 00007ff633e6e030
thread_local destructor: `this` = 000002ab619d1d68, `00007ff633e6e030` is dead

which violates the standard (the destructor of tls references the destroyed st).

With the mcf thread model and the patched CRT this outputs:

static_obj_type constructor: `this` = 00007ff70c0dd030
thread_local constructor: `this` = 0000023149ff1518, `00007ff70c0dd030` is alive
main
thread_local destructor: `this` = 0000023149ff1518, `00007ff70c0dd030` is alive
static_obj_type destructor: `this` = 00007ff70c0dd030

@lhmouse
Copy link
Contributor

lhmouse commented Sep 30, 2023

Found the original issue for the nanosleep() patch: lhmouse/MINGW-packages#11 (comment)
Just checked, this test build of winlibs also didn't define _GLIBCXX_USE_NANOSLEEP.

This report says 'several projects fail to build because clock_gettime seems to be missing.' GCC with the posix thread model and a mingw target seems to add -lpthread as a default library, which isn't the case on Linux (this is the outout of the MSYS2 gcc):

$ gcc test.c -v 2>&1 |  grep --color pthread
(... ...) -lmingw32 -lgcc -lgcc_eh -lmoldname -lmingwex -lmsvcrt -lkernel32 -lpthread -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 -lgcc -lgcc_eh -lmoldname -lmingwex -lmsvcrt -lkernel32 (... ...)
                                                                            ^^^^^^^^^

With winpthreads, clock_gettime() and nanosleep() require -lpthread when linking, but on Linux they don't (they do not require -lrt any more). The patch was created to match the Linux behavior.

@starg2
Copy link
Contributor Author

starg2 commented Oct 1, 2023

I just tried building with sjlj exception handling and gcc fails to build during stage 2:

echo timestamp > stmp-int-hdrs
/home/user/ucrt-sjlj-13/x86_64-1320-mcf-sjlj-ucrt-rt_v12/build/gcc-13.2.0/./gcc/xgcc -B/home/user/ucrt-sjlj-13/x86_64-1320-mcf-sjlj-ucrt-rt_v12/build/gcc-13.2.0/./gcc/ -fno-checking -xc -nostdinc nul -S -o nul -fself-test=../../../../src/gcc-13.2.0/gcc/testsuite/selftests
/home/user/ucrt-sjlj-13/x86_64-1320-mcf-sjlj-ucrt-rt_v12/build/gcc-13.2.0/./gcc/xgcc -B/home/user/ucrt-sjlj-13/x86_64-1320-mcf-sjlj-ucrt-rt_v12/build/gcc-13.2.0/./gcc/ -fno-checking -xc++ -nostdinc nul -S -o nul -fself-test=../../../../src/gcc-13.2.0/gcc/testsuite/selftests
make[3]: *** [../../../../src/gcc-13.2.0/gcc/c/Make-lang.in:128: s-selftest-c] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [../../../../src/gcc-13.2.0/gcc/cp/Make-lang.in:230: s-selftest-c++] Error 1
rm gcc.pod
make[3]: Leaving directory '/home/user/ucrt-sjlj-13/x86_64-1320-mcf-sjlj-ucrt-rt_v12/build/gcc-13.2.0/gcc'
make[2]: *** [Makefile:5022: all-stage2-gcc] Error 2
make[2]: Leaving directory '/home/user/ucrt-sjlj-13/x86_64-1320-mcf-sjlj-ucrt-rt_v12/build/gcc-13.2.0'
make[1]: *** [Makefile:23922: stage2-bubble] Error 2
make[1]: Leaving directory '/home/user/ucrt-sjlj-13/x86_64-1320-mcf-sjlj-ucrt-rt_v12/build/gcc-13.2.0'
make: *** [Makefile:1091: all] Error 2

@lhmouse Are mcf and sjlj incompatible? If so, I'll add a test in the build script to reject that configuration.

@lhmouse
Copy link
Contributor

lhmouse commented Oct 1, 2023

@lhmouse Are mcf and sjlj incompatible? If so, I'll add a test in the build script to reject that configuration.

In theory, no. I used to build GCC with SJLJ exception model many years ago and there was no build issue.

@starg2
Copy link
Contributor Author

starg2 commented Oct 1, 2023

I see. Since sjlj is rarely used these days, I'll leave it as is until someone complains.

@niXman niXman marked this pull request as ready for review October 1, 2023 11:42
@niXman niXman merged commit b876df9 into niXman:develop Oct 1, 2023
10 of 12 checks passed
@niXman
Copy link
Owner

niXman commented Oct 1, 2023

merged.

thank you guys!

@starg2 starg2 deleted the mcf branch October 7, 2023 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do you have any interest in the mcf thread model?
3 participants