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.

Sign up for GitHub

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

Jump to bottom

Fix for higher version of C++ usage, Clang support, and some build policy change. #246

Merged
merged 5 commits into from
Feb 10, 2024
Merged

Fix for higher version of C++ usage, Clang support, and some build policy change. #246

merged 5 commits into from
Feb 10, 2024

Conversation

stripe2933
Copy link
Contributor

@stripe2933 stripe2933 commented Feb 8, 2024 •
edited
Loading

  1. I removed the hardcoding in the CMake script that enables C++20 mode, aiming to use compiler versions that support coroutines. However, it now forcefully uses C++20 mode even in higher versions (e.g., C++23, C++26). For instance, a C++23 project utilizing this library won't be able to use std::print.
  2. Although apps using this project as a dependency do not require test/example builds, the build defaults for these are set to true. The new commit activates the build only when the project is at the top level.
  3. Despite the inability to use Linux kernel functions on macOS, the corresponding build flag is enabled unless the build environment is Linux and not WebAssembly. I modified it to activate the flag only in a Linux environment that is not WebAssembly.
  4. In the libc++ environment, exception_ptr now requires inclusion of the <exception> header.

@jbaldwin
Copy link
Owner

jbaldwin commented Feb 9, 2024

Hi Thanks for the PR, these look like great additions.

(1) I agree hardcoding to C++20 was a mistake, it was initially there a few years ago when it was the latest standard but it is definitely a problem now since C++23 and onward are coming out.

(2) If I'm understanding this correctly you can now only build tests and examples if you build libcoro directly from the root checkout directory, and not if its included in another project as a dependency? I'm struggling to think of why you'd need to build tests and examples in this scenario but the change does prevent you from doing so if I'm reading it correctly.

(3) 👍

(4) Great, looks like it was also reported in #247 (@bruno-j-nicoletti looks like this PR fixes your build problem I think)

@jbaldwin jbaldwin mentioned this pull request Feb 9, 2024
Closed
@stripe2933
Copy link
Contributor Author

(2): In my case, I tried to use vcpkg for 1st tier package management, but it failed to build. So I tried second attempt with using CPMAddPackage. It directly clone the repository or release archive into project build dir, but in this case I should build libcoro's tests and examples, which is not needed for library use. Therefore, I think it is feasible to build them only if project is master (top level).

@jbaldwin
Copy link
Owner

Yeah cool, it makes sense from that perspective. I wonder if it's worth adding CI runs for the package managers to download and link it to a sample project just to know it's working properly. I'll open an issue to possibly take a look at the major ones.

@jbaldwin jbaldwin merged commit ef29da0 into jbaldwin:main Feb 10, 2024
28 checks passed
uilianries added a commit to uilianries/libcoro that referenced this pull request Feb 15, 2024
@uilianries @jbaldwin
@ripose-jp @bruno-j-nicoletti @stripe2933
Support Conan in CI build (#1)
dad0b1a 
* CMake: Keep git hooks as optional (jbaldwin#234)

* Keep git hooks as optional since they are not required for using the library, only for development.
* Cmake LIBCORO_RUN_GITCONFIG=ON|OFF option

* Introduce concepts::io_executor (jbaldwin#238)

* Introduce concepts::io_scheduler

The dns resolver hard links against coro::io_scheduler via coro::task_container<coro::io_scheduler>. Since the concepts::executor was introduced a while ago to make it possible to pass in different executors (or even user defined executors) the resolver has apparently been forgotten about. Lets introduce an io_executor (needs poll()) and make the resolver use an agnostic concepts::executor.

The other reason for doing this is that the windows build is failing when LIBCORO_FEATURE_PLATFORM=OFF and building as a SHARED library since the coro::io_scheduler is sneaking in via the resolver class but then ultimately isn't compiled since its excluded from the build.

Closes jbaldwin#237

* Remove LIBCORO_FEATURE_PLAFTORM

It really is identical with LIBCORO_FEATURE_NETWORKING at this point, it
just wasn't so obvious.

* Add shared library option (jbaldwin#236)

* It generates static only or shared libraries according to LIBCORO_BUILD_SHARED_LIBS option via the `cmake` `BUILD_SHARED_LIBS` to propagate to all libraries built by the project.
* When generating shared library in Windows, it will produce bin/libcoro.dll and lib/libcoro.lib (exposed symbols to load in the dll)
* The CMake definition `CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS` does not export static variables
  * To export semaphore's variables a new header file is created, the `coro/export.hpp`. It's generated by `cmake` using the method `generate_export_header`, and should manage `__declspec`.
* The ctest does not include the DLL folder by default, so it is configured by using `set_tests_properties`

* Fix use after free in coro::task_container (jbaldwin#240)

In coro::task_container, this is potential for a use after free bug to
occur between the final co_return of make_cleanup_task and another
thread executing gc_internal. This can happen because there is a delay
between the std::scoped_lock being destroyed upon the co_return of
make_cleanup_task and its final suspend. This leads to a race condition
where another thread can acquire the lock and initiate a garbage
collection, where that cleanup task is destroyed before it's final
suspend. This leads to a use after free.

This commit fixes the issue by reworking how tasks get garbage
collected. Instead of unconditionally destroying tasks scheduled for
deletion, the gc_internal calls is_ready on the coro::task first. This
function will only be true in the cases where the task has entered
final suspend, has been default constructed, or has been destroyed. This
means a task can get scheduled for deletion, but not be deleted when
gc_internal is called. This allows time for the task to enter final
suspend, which avoids the use after free bug.

To make this implementation easier, several aspects of the
implementation were changed. m_task_indexes was removed in favor of
a queue which keeps track of free indices into m_tasks. This makes
things easier since it removes the hard constraint of keeping used
indices behind m_free_pos and free indices at or after.
m_tasks_to_delete was then changed to a list in order to allow for
constant time deletion of elements in place. This is helpful because it
is possible for tasks to be queue for deletion, but not yet safe to
delete. This means unconditionally clearing the m_tasks_to_delete data
structure is no longer viable.

* Upgrade tl::expected to 1.1 (jbaldwin#243)

* Creating a CI workflow for macos, (jbaldwin#249)

- Created ci-macos.yml
  - tweaked CMake to deal with clang17 location

* Fix for higher version of C++ usage, Clang support, and some build policy change. (jbaldwin#246)

* Enable test/example build only if project is top level. LIBCORO_FEATURE_NETWORKING and LIBCORO_FEATURE_TLS macro should set on Linux environment only.

* Make compilable above the C++20 mode.

* Missing include: <exception>

* Update CMakeLists.txt

* Calling coro::sync_wait with coro::ring_buffer::consume returns default constructed objects for complex return values (jbaldwin#244)

Running the test that replicates the bug via valgrind or asan it shows
that the compiler is calling sync_wait()'s promise's destructor before
moving the complex object return value. This causes the object to be
destructed and then the final move out is on deleted (use after free)
memory causing the object to be in a bad empty state.

To resolve this for now a double move has been introduced to move the
complex object off the promise object and onto the sync_wait() function
callstack. This seems to keep the object alive and not be destructed
when sync_wait() finally returns.

Other solutions could be to heap allocate the promise() object or even
the return_type but this is probably more expensive than a double move,
but will vary on use case.

Closes jbaldwin#242

* Release v0.11 (jbaldwin#250)

Closes jbaldwin#241

* Revert cmake PROJECT_IS_TOP_LEVEL (jbaldwin#252)

This only appears to work with cmake >= 3.25 which isn't ready for most
of the CI runs, so it will be reverted for now and re-worked moving
forward.

* Release v0.11.1 hotfix (jbaldwin#253)

* Added missing header '<atomic>' when compiling with clang and c++23 (jbaldwin#254)

* CI explicitly run CMAKE_CXX_STANDARD=20|23 (jbaldwin#257)

CI Added explicit 20/23 standards for:
* ci-fedora
* ci-macos
* ci-ubuntu
* ci-windows

Did not add 23 builds for
* ci-opensuse
* ci_emscripten

Closes jbaldwin#256

* Initial try to include Conan in the CI

Signed-off-by: Uilian Ries <[email protected]>

---------

Signed-off-by: Uilian Ries <[email protected]>
Co-authored-by: Josh Baldwin <[email protected]>
Co-authored-by: ripose-jp <[email protected]>
Co-authored-by: Bruno Nicoletti <[email protected]>
Co-authored-by: LEE KYOUNGHEON <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaldwin jbaldwin jbaldwin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stripe2933 @jbaldwin