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

cpp: thread: support for std::thread, std::mutex, std::condition_variable, etc #43729

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Mar 13, 2022

This commit adds support for std::thread and std::this_thread when the configured C++ standard is >= C++11.

Fixes #25569

Accompanying sdk-ng PR: zephyrproject-rtos/sdk-ng#735

SeaBIOS (version rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org)
Booting from ROM..*** Booting Zephyr OS build zephyr-v3.0.0-1074-g90c018160af3  ***
version 201703
Running test suite std_thread_tests
===================================================================
START - test_this_thread_get_id
 PASS - test_this_thread_get_id in 0.2 seconds
===================================================================
START - test_this_thread_yield
 PASS - test_this_thread_yield in 0.1 seconds
===================================================================
START - test_this_thread_sleep_until
 PASS - test_this_thread_sleep_until in 0.1 seconds
===================================================================
START - test_this_thread_sleep_for
 PASS - test_this_thread_sleep_for in 0.1 seconds
===================================================================
START - test_thread_get_id
 PASS - test_thread_get_id in 0.1 seconds
===================================================================
START - test_thread_native_handle
 SKIP - test_thread_native_handle in 0.1 seconds
===================================================================
START - test_thread_hardware_concurrency
 PASS - test_thread_hardware_concurrency in 0.1 seconds
===================================================================
START - test_thread_join
 SKIP - test_thread_join in 0.1 seconds
===================================================================
START - test_thread_detach
 SKIP - test_thread_detach in 0.1 seconds
===================================================================
START - test_thread_swap
 SKIP - test_thread_swap in 0.1 seconds
===================================================================
Test suite std_thread_tests succeeded
===================================================================
PROJECT EXECUTION SUCCESSFUL

Notes:

  • Tests pass for CONFIG_ARCH_POSIX=y and CONFIG_ARCH_POSIX=n
  • Currently skipping a few tests when CONFIG_ARCH_POSIX=n as we cannot yet auto-allocate thread stacks
  • Requires a couple of toolchain header modifications (described in comments / PR)
  • Eventually would be ideal to bypass POSIX threads and use Zephyr native threads, but they are a convenient stepping stone

@github-actions github-actions bot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Mar 13, 2022
@cfriedt cfriedt force-pushed the issue/25569/support-for-std-thread-and-std-this-thread branch 4 times, most recently from 7e0911f to 597f1c9 Compare March 13, 2022 14:31
@cfriedt
Copy link
Member Author

cfriedt commented Mar 13, 2022

The toolchain header include/bits/gthr.h was modified as follows:

#if _GLIBCXX___ANDROID__
#include <bits/gthr-posix.h>
#elif defined(__ZEPHYR__)
#include <bits/gthr-zephyr.h>
#else
#include <bits/gthr-default.h>
#endif

The file include/bits/gthr-zephyr.h is a copy of the upstream gthr-posix.h with the following additions. Normally, I would suggest keeping this in the toolchain, as this file is GPLv3. It is included here for reference purposes only.

#define __GTHREADS 1
#define __GTHREADS_CXX0X 1
#define _GTHREAD_USE_MUTEX_TIMEDLOCK 1
#define _GLIBCXX_USE_NANOSLEEP 1

struct __sFILE_fake {
	unsigned char *_p; /* current position in (some) buffer */
	int _r; /* read space left for getc() */
	int _w; /* write space left for putc() */
	short _flags; /* flags, below; this FILE is free if 0 */
	short _file; /* fileno, if Unix descriptor, else -1 */
	struct __sbuf _bf; /* the buffer (at least 1 byte, if !NULL) */
	int _lbfsize; /* 0 or -_bf._size, for inline putc */

	struct _reent *_data;
};

typedef long _off_t;
typedef unsigned long size_t;
typedef long ssize_t;
#define _fpos_t long
#define __FILE struct __sFILE_fake
typedef __builtin_va_list __gnuc_va_list;
#define PTHREAD_MUTEX_INITIALIZER {0}
#define PTHREAD_COND_INITIALIZER {0}

#include <posix/pthread.h>

#if ((defined(_LIBOBJC) || defined(_LIBOBJC_WEAK)) \
     || !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK))
# include <posix/unistd.h>

@cfriedt cfriedt force-pushed the issue/25569/support-for-std-thread-and-std-this-thread branch 3 times, most recently from 48ebd02 to 9b19892 Compare March 13, 2022 14:51
@cfriedt cfriedt force-pushed the issue/25569/support-for-std-thread-and-std-this-thread branch 2 times, most recently from c7f071a to b5f0b91 Compare April 10, 2022 11:55
@stephanosio stephanosio mentioned this pull request May 19, 2022
8 tasks
@ortogonal
Copy link

@cfriedt is there any reason why this pull-req is not progressing forward?

I'm also very interested in getting better C++ support into Zephyr and would like to help out. Is there any specific I can help out with here?

I've tested this PR with the SDK changes mentioned above and my 5-min test says it works fine.

@cfriedt
Copy link
Member Author

cfriedt commented Jun 27, 2022

@cfriedt is there any reason why this pull-req is not progressing forward?

@ortogonal
The biggest reasons are

  1. Zephyr has no way currently to dynamically allocate a thread stack (there is a draft pr for that kernel: support dynamic thread stack allocation #44379)
  2. The related toolchain integrations need to happen (some work for @stephanosio)
  3. Additional work may be necessary around k_mutex (iirc, there may be some static requirement for Zephyr mutex objects)

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 28, 2022
@cfriedt cfriedt removed the Stale label Aug 28, 2022
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Oct 28, 2022
@cfriedt cfriedt removed the Stale label Nov 1, 2022
@github-actions
Copy link

github-actions bot commented Jan 1, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 1, 2023
@cfriedt cfriedt removed the Stale label Jan 3, 2023
@github-actions
Copy link

github-actions bot commented Mar 5, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Mar 5, 2023
@cfriedt cfriedt removed the Stale label Mar 5, 2023
@cfriedt cfriedt force-pushed the issue/25569/support-for-std-thread-and-std-this-thread branch from e1486d4 to 914ad2a Compare March 9, 2024 23:14
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Mar 9, 2024
@zephyrbot zephyrbot requested a review from ycsin March 9, 2024 23:15
@cfriedt cfriedt force-pushed the issue/25569/support-for-std-thread-and-std-this-thread branch from 914ad2a to d97b95b Compare March 9, 2024 23:29
@cfriedt
Copy link
Member Author

cfriedt commented Mar 10, 2024

cc @XenuIsWatching

@cfriedt cfriedt changed the title cpp: thread: support for std::thread and std::this_thread cpp: thread: support for std::thread, std::mutex, std::condition_variable, etc Mar 10, 2024
@cfriedt
Copy link
Member Author

cfriedt commented Mar 10, 2024

Currently, eval SDK Toolchain components are ready to download for evaluation purposes

  • aarch64-zephyr-elf
  • arm-zephyr-eabi
  • riscv64-zephyr-elf
  • x86_64-zephyr-elf

The x86_64 and riscv64 toolchain components also support the 32-bit variants for those ISAs

zephyrproject-rtos/sdk-ng#735

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 10, 2024
@cfriedt cfriedt removed the Stale label May 10, 2024
@cfriedt
Copy link
Member Author

cfriedt commented May 10, 2024

This is just waiting on a candidate toolchain or CI image to become available.

v0.16.6 does have the necessary toolchain changes (aside from having to enable c11 threads) so as long as we can point Zephyr CI at a different image with that toolchain it can run through automated testing.

@stephanosio - is there a way that this can be pulled into the CI run you had going

@AfflatusX
Copy link

AfflatusX commented May 30, 2024

Hi @cfriedt - I patched the 16.6 build to give the threading support a try. I was able to build/link my firmware successfully with the patch, thanks to your work!

I noticed that whenever __cxxabiv1::__cxa_guard_acquire() is invoked, it would throw exception though. I'm building against QEMU MPS3_AN547 target (cortex M55). When running LLDB against the elf I saw following callstack:

(lldb) bt
* thread #1, stop reason = breakpoint 1.1
  * frame #0: 0x10008160 zephyr.elf`__cxa_throw
    frame #1: 0x1000446c zephyr.elf`(anonymous namespace)::mutex_wrapper::mutex_wrapper() (.isra.0) + 44
    frame #2: 0x100044a0 zephyr.elf`__cxa_guard_acquire + 24
    frame #3: 0x100015ce zephyr.elf`main at main.cpp:27:33
    frame #4: 0x100031f0 zephyr.elf`bg_thread_main(unused1=0x00000000, unused2=0x00000000, unused3=0x00000000) at init.c:459:8
    frame #5: 0x10001672 zephyr.elf`z_thread_entry(entry=(zephyr.elf`bg_thread_main + 1 at init.c:424:20), p1=0x00000000, p2=0x00000000, p3=0x00000000) at thread_entry.c:48:2
    frame #6: 0x10002612 zephyr.elf`arch_switch_to_main_thread(main_thread=<unavailable>, stack_ptr=0x00000000, _main=<unavailable>) at thread.c:575:2

my main.c is very simple:

#include <stdio.h>
#include <cxxabi.h>

int main(void)
{
	printf("%s\n", "started");
	__cxxabiv1::__cxa_guard_acquire(NULL);
	printf("%s\n", "ended");
}

I want to debug this further, but as you can see the symbols for anything in stdlibc++ is missing. Two questions for you:

  1. have you run into this or any guess on why this might be happening?
  2. any suggestion on how can I load the libstdc++'s symbol into lldb so I can debug further?

Again, thanks for the great work here!

@cfriedt
Copy link
Member Author

cfriedt commented May 30, 2024

Hi @AfflatusX -

There is a known issue and fix, documented here

zephyrproject-rtos/sdk-ng#751
zephyrproject-rtos/gcc#31

I'm not sure what release this will be integrated into in the Zephyr SDK.

@stephanosio is the C++ maintainer and SDK maintainer, and might have a better idea.

@AfflatusX
Copy link

@cfriedt - Thanks for the quick reply! I can fork/patch your PR in the meantime. However, when I pull the v0.16.6 branch and try to build the toolchain using contrib/linux_build_toochain.sh it failed at the GCC step with following output:

[ALL  ]    Using host-linux.o for host machine hooks.
[ALL  ]    c11 is an unknown thread package
[ERROR]    make[2]: *** [Makefile:4565: configure-gcc] Error 1
[ALL  ]    make[2]: Leaving directory '/home/txie/zephyr-sdk-build/build-arm-zephyr-eabi/.build/arm-zephyr-eabi/build/build-cc-gcc-final'
[ERROR]    make[1]: *** [Makefile:1038: all] Error 2
[ALL  ]    make[1]: Leaving directory '/home/txie/zephyr-sdk-build/build-arm-zephyr-eabi/.build/arm-zephyr-eabi/build/build-cc-gcc-final'

I tried to search for the message c11 is an unknown thread package but couldn't find anything useful. Any thoughts?

@cfriedt
Copy link
Member Author

cfriedt commented May 31, 2024

@cfriedt - Thanks for the quick reply! I can fork/patch your PR in the meantime. However, when I pull the v0.16.6 branch and try to build the toolchain using contrib/linux_build_toochain.sh it failed at the GCC step with following output:

I tried to search for the message c11 is an unknown thread package but couldn't find anything useful. Any thoughts?

I would guess that some changes were reverted in sdk-ng and possibly elsewhere.

@AfflatusX
Copy link

AfflatusX commented May 31, 2024

@cfriedt - Thanks for the quick reply! I can fork/patch your PR in the meantime. However, when I pull the v0.16.6 branch and try to build the toolchain using contrib/linux_build_toochain.sh it failed at the GCC step with following output:
I tried to search for the message c11 is an unknown thread package but couldn't find anything useful. Any thoughts?

I would guess that some changes were reverted in sdk-ng and possibly elsewhere.

hmm, I'm sitting on the v0.16.6 tag:

txie@HABOR:~/sdk-ng-atomic$ git log
commit 2d488e6bef4bc258c0061f12d33ac083acd9ae1d (HEAD -> threads, tag: v0.16.6)
Author: Stephanos Ioannidis <[email protected]>
Date:   Mon Apr 29 14:59:17 2024 +0900

    VERSION: Bump to 0.16.6

looking at the CI history, this commit passed the full CI suite, so it should build successfully right?

Is there a branch contains your latest patches with thread enabled I can give it a try locally?

my bad, forgot gcc is a sub module, need to update separately.

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

This commit adds support for std::thread and std::this_thread
when the configured C++ standard is >= C++11.

The implementation uses POSIX threads under the hood.

Signed-off-by: Christopher Friedt <[email protected]>
This commit adds tests for std::thread and std::this_thread.

Signed-off-by: Christopher Friedt <[email protected]>
Add tests for the ISO C++11 std::condition_variable.

Signed-off-by: Christopher Friedt <[email protected]>
Add tests for ISO C++ std::mutex.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt force-pushed the issue/25569/support-for-std-thread-and-std-this-thread branch from d97b95b to 984a65b Compare August 11, 2024 18:42
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Oct 11, 2024
@ycsin ycsin removed the Stale label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: C++ area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test DNM This PR should not be merged (Do Not Merge)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for std::thread and std::this_thread
9 participants