-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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: Link to threads library #3794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there also needs to be a find_dependency(Threads)
call added to runtime/Cpp/cmake/antlr4-runtime.cmake.in
c6cb02b
to
306a41a
Compare
@Technius like this? |
I'm nervous about changing the C++ target since I know so little about C++ these days. What will this break in existing code if we link it this way? |
Looks like we are getting C++ errors:
|
Well without this, it just doesn't work at all. See #3708 |
Does it provide some more detailed info on that matter somewhere? |
Indeed i see std::call_once invoked. How does that link w/o pthreads ever? it's called in lexer init so must execute. This is created during testing antlr. Does it look like pthreads or abseil?
Ah. looks like |
Wow. That CMakeLists.txt file is insane. Ugh. Anyway, having trouble getting cmake to build arm64 on mac silicon. This works but defaults to x86 otherwise (latest cmake):
Then linking against, this works:
|
ac94618
to
7adb18f
Compare
I'm not sure - maybe the symbol lookup is deferred until the lib is actually loaded and if no suitable candidate is found, the function just throws the encountered exception 🤷 |
Ah, I figured out why the CI is failing:
|
7adb18f
to
8c30bc1
Compare
As detailed in antlr#3708, it is necessary to link against the (p)threads library in order to be able to use std::call_once without producing linker errors. Since this function is frequently used in ANTLR's cpp version, this commit ensures that the respective library is always linked against in order to avoid this error, even if downstream users are not explicitly linking against an appropriate threads library. Fixes antlr#3708 Signed-off-by: Robert Adam <dev@robert-adam.de> Co-authored-by: Bryan Tan <Technius@users.noreply.github.com>
8c30bc1
to
f55d251
Compare
hooray! ok, so from your perspective this is now ready for merging? It looks a lot safer now (and I also learned something about C++ again!). |
Yep, from my POV this is ready :) |
As detailed in #3708, it is necessary to link against the (p)threads
library in order to be able to use std::call_once without producing
linker errors.
Since this function is frequently used in ANTLR's cpp version, this
commit ensures that the respective library is always linked against in
order to avoid this error, even if downstream users are not explicitly
linking against an appropriate threads library.
Fixes #3708