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 runtime: Remove pthread dependency. #4086

Merged
merged 1 commit into from
Feb 19, 2023

Conversation

ArthurSonzogni
Copy link
Contributor

ANTLR doesn't use threads, and it used not to depend on pthread library either.

It changed recently in 2022: #3708 The patch linked against pthread, because the GNU libstdc++ used to depend on it in their implementation of std::call_once.

By the way, the libstdc++ stopped it in 2020:
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=93e79ed391b9c636f087e6eb7e70f14963cd10ad So this is not more needed.

I would like to stop depending on pthread. I am using ANTLR in C++ WebAssembly for the website:
https://arthursonzogni.com/Diagon/

It doesn't compile with emscripten anymore, because by default pthread is not enabled. It could be enabled, but it would force me to deploy cross-origin-isolation:
https://web.dev/cross-origin-isolation-guide/

Solutions:

  1. Stop linking against pthread, because the libstdc++ stopped depending on it for std::call_once.
  2. Implement std::call_once ourself using std::atomic_flag
  3. Implement std::call_once ourself using a boolean flag, assuming we don't need to support threads.

I chose to do (2) in this patch.

@ArthurSonzogni ArthurSonzogni force-pushed the remove-pthread branch 2 times, most recently from 2285791 to dcaa0be Compare January 22, 2023 13:59
@ArthurSonzogni
Copy link
Contributor Author

@Krzmbrzl WDYT?

@Krzmbrzl
Copy link
Contributor

Sounds good to me. Option 1) would be an issue imo, because it restricts usability of ANTLR to somewhat recent systems only. However, implementing this ourselves appears as a solid workaround for our use case 👍

ArthurSonzogni added a commit to ArthurSonzogni/Diagon that referenced this pull request Jan 22, 2023
It now links against pthread, and this is not supported directly by
emscriten. Downgrading the ANTLR version fixed the problem.

Long-term, I submitted a PR against ANTLR to fix the issue:
antlr/antlr4#4086
ArthurSonzogni added a commit to ArthurSonzogni/Diagon that referenced this pull request Jan 22, 2023
It now links against pthread, and this is not supported directly by
emscriten. Downgrading the ANTLR version fixed the problem.

Long-term, I submitted a PR against ANTLR to fix the issue:
antlr/antlr4#4086
ANTLR doesn't use threads, and it used not to depend on pthread library
either.

It changed recently in 2022: antlr#3708
The patch linked against pthread, because the GNU libstdc++ used to
depend on it in their implementation of `std::call_once`.

By the way, the libstdc++ stopped it in 2020:
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=93e79ed391b9c636f087e6eb7e70f14963cd10ad
So this is not more needed.

I would like to stop depending on pthread. I am using ANTLR in C++
WebAssembly for the website:
https://arthursonzogni.com/Diagon/

It doesn't compile with emscripten anymore, because by default pthread
is not enabled. It could be enabled, but it would force me to deploy
cross-origin-isolation:
https://web.dev/cross-origin-isolation-guide/

Solutions:
1. Stop linking against pthread, because the libstdc++ stopped depending
   on it for std::call_once.
2. Implement std::call_once ourself using std::atomic_flag
3. Implement std::call_once ourself using a boolean flag, assuming we
   don't need to support threads.

I chose to do (2) in this patch.

Signed-off-by: ArthurSonzogni <sonzogniarthur@gmail.com>
@ArthurSonzogni
Copy link
Contributor Author

Okay. Then, I think this is ready. Thanks!
The latest CQ run failed, but that was unrelated to my patch. I rebased to run them again.

@parrt
Copy link
Member

parrt commented Feb 19, 2023

Thanks!

@parrt
Copy link
Member

parrt commented Feb 19, 2023

Hmm..seems we have a failure:

https://github.com/antlr/antlr4/actions/runs/4031407036/jobs/6930678126

CMake suite maintained and supported by Kitware (kitware.com/cmake).
/usr/local/bin/ninja
1.11.1
/usr/local/opt/ccache/libexec/gcc-9
ccache: error: Could not find compiler "gcc-9" in PATH

@parrt
Copy link
Member

parrt commented Feb 19, 2023

I'll retry it.

@parrt parrt merged commit 2073147 into antlr:master Feb 19, 2023
@ArthurSonzogni
Copy link
Contributor Author

Thanks for the code review!

@parrt
Copy link
Member

parrt commented Feb 19, 2023

damn. this was on master not dev.

parrt added a commit to parrt/antlr4 that referenced this pull request Feb 19, 2023
@parrt
Copy link
Member

parrt commented Feb 19, 2023

Can you base this on latest dev and resubmit? thanks. @ArthurSonzogni

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants