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

build: fix 'threadsafe' feature detection for older gcc #12127

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Oct 15, 2023

  • Add 'threadsafe' to the feature list shown during build if POSIX threads are being used.

This is a follow-up to 5adb600 which added support for building a thread-safe libcurl with older versions of gcc where atomic is not available but pthread is.

Reported-by: Dan Fandrich

Fixes #12125
Closes #xxxx

@dfandrich
Copy link
Contributor

I don't know how to easily get cmake going on this ancient FreeBSD, but I tested the autoconf version but it didn't work. The problem is that HAVE_PTHREAD_H isn't set as an environment variable anywhere. Replacing that clause with x"$ac_cv_header_pthread_h" = x"yes" (the superfluous xs are in there for compatibility with ancient shells that don't handle empty arguments well) fixes it. Thanks!

@jay jay force-pushed the fix_threadsafe_feature_detect branch from 8373a27 to 4585c67 Compare October 15, 2023 18:30
@jay
Copy link
Member Author

jay commented Oct 15, 2023

I tested the autoconf version but it didn't work. The problem is that HAVE_PTHREAD_H isn't set as an environment variable anywhere.

That is confusing because it is supposed to be defined when pthreads is used isn't it?

curl/configure.ac

Lines 3741 to 3744 in d755a5f

dnl detect pthreads
if test "$want_pthreads" != "no"; then
AC_CHECK_HEADER(pthread.h,
[ AC_DEFINE(HAVE_PTHREAD_H, 1, [if you have <pthread.h>])

Replacing that clause with x"$ac_cv_header_pthread_h" = x"yes" (the superfluous xs are in there for compatibility with ancient shells that don't handle empty arguments well) fixes it. Thanks!

Thanks I have incorporated your feedback. Please try the most recently pushed version. I updated both conditionals to prepend the x but I notice in the file it's done both ways. When is one more appropriate than the other? If I understand correctly test $foo = "bar" would be a problem if $foo was some command line switch like --qux because then test would interpret it wrong, but in practice when is that actually going to happen with the variables that we set? Or if $foo was unset then wouldn't we have noticed it by now with all the conditionals elsewhere that don't prepend the x?

@dfandrich
Copy link
Contributor

dfandrich commented Oct 15, 2023 via email

@dfandrich
Copy link
Contributor

I tested the update patch with autotools and it works fine. I simulated a missing stdatomic.h in a cmake build on another system and that worked as well.

@dfandrich
Copy link
Contributor

Just noticed the && in the test command. I'm pretty sure that's a GNUism and should be changed to -a for portability.

- Add 'threadsafe' to the feature list shown during build if POSIX
  threads are being used.

This is a follow-up to 5adb600 which added support for building a
thread-safe libcurl with older versions of gcc where atomic is not
available but pthread is.

Reported-by: Dan Fandrich

Fixes curl#12125
Closes #xxxx
@jay jay force-pushed the fix_threadsafe_feature_detect branch from 4585c67 to 2e4a545 Compare October 16, 2023 22:27
@jay
Copy link
Member Author

jay commented Oct 16, 2023

Thanks. I have added your change, please take another look.

@dfandrich
Copy link
Contributor

I mis-parsed the statement. It's fine as-is since the shell spawns a second test for the second test clause, but it would avoid a fork if && test is replaced with -a.

@dfandrich
Copy link
Contributor

Yes, that's exactly what I was thinking.

@dfandrich
Copy link
Contributor

W.r.t. the x's in the x"$FOO" = xyz question, yes, we likely would have seen fallout already from all the cases where we don't do that. It's mostly just really, really old shells that have an issue with testing against an empty string. POSIX requires it so it's only pre-POSIX environments where it could make a difference, but since POSIX predates C89 so there could theoretically be a system where it's needed. But, I wouldn't lose too much sleep over it.

@jay jay closed this in e160d17 Oct 17, 2023
@jay jay deleted the fix_threadsafe_feature_detect branch October 17, 2023 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Test 1014 failure on FreeBSD 6.2
2 participants