[PATCH] Change configure to check if pthreads are usable without any extra flags

[PATCH] Change configure to check if pthreads are usable without any extra flags

[Max Horn]

The configure script checks whether certain flags / libraries are
required to use pthreads. But so far it did not consider the possibility
that no extra compiler flags are needed (as is the case on Mac OS X). As
a result, configure would always add "-mt" to the list of flags. This in
turn triggered a warning in clang about an unknown argument.
To solve this, we now first check if pthreads work without extra flags.

Signed-off-by: Max Horn <max@quendi.de>
---
 configure.ac | 2 +-
 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)

diff --git a/configure.ac b/configure.ac
index 4e9012f..d767ef3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1002,7 +1002,7 @@ if test -n "$USER_NOPTHREAD"; then
 # -D_REENTRANT' or some such.
 elif test -z "$PTHREAD_CFLAGS"; then
   threads_found=no
-  for opt in -mt -pthread -lpthread; do
+  for opt in "" -mt -pthread -lpthread; do
      old_CFLAGS="$CFLAGS"
      CFLAGS="$opt $CFLAGS"
      AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])
-- 
1.7.11.1.145.g4722b29.dirty

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> The configure script checks whether certain flags / libraries are
> required to use pthreads. But so far it did not consider the possibility
> that no extra compiler flags are needed (as is the case on Mac OS X). As
> a result, configure would always add "-mt" to the list of flags. This in
> turn triggered a warning in clang about an unknown argument.
> To solve this, we now first check if pthreads work without extra flags.
>
> Signed-off-by: Max Horn <max@quendi.de>
> ---
>  configure.ac | 2 +-
>  1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
>
> diff --git a/configure.ac b/configure.ac
> index 4e9012f..d767ef3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1002,7 +1002,7 @@ if test -n "$USER_NOPTHREAD"; then
>  # -D_REENTRANT' or some such.
>  elif test -z "$PTHREAD_CFLAGS"; then
>    threads_found=no
> -  for opt in -mt -pthread -lpthread; do
> +  for opt in "" -mt -pthread -lpthread; do

Hmph.  Would it work to append the new empty string at the end of
the existing list, as opposed to prepending it?  I'd prefer a
solution that is order independent, or if the change is order
dependent, then a comment to warn others from changing it later.

>       old_CFLAGS="$CFLAGS"
>       CFLAGS="$opt $CFLAGS"
>       AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])

Perhaps "for linking with POSIX Threads" would make it clearer, as
CFLAGS (rather, PTHREAD_CFLAGS) has been checked earlier separately.

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Max Horn]

On 09.07.2012, at 16:50, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>> The configure script checks whether certain flags / libraries are
>> required to use pthreads. But so far it did not consider the possibility
>> that no extra compiler flags are needed (as is the case on Mac OS X). As
>> a result, configure would always add "-mt" to the list of flags. This in
>> turn triggered a warning in clang about an unknown argument.
>> To solve this, we now first check if pthreads work without extra flags.
>> 
>> Signed-off-by: Max Horn <max@quendi.de>
>> ---
>> configure.ac | 2 +-
>> 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
>> 
>> diff --git a/configure.ac b/configure.ac
>> index 4e9012f..d767ef3 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1002,7 +1002,7 @@ if test -n "$USER_NOPTHREAD"; then
>> # -D_REENTRANT' or some such.
>> elif test -z "$PTHREAD_CFLAGS"; then
>>   threads_found=no
>> -  for opt in -mt -pthread -lpthread; do
>> +  for opt in "" -mt -pthread -lpthread; do
> 
> Hmph.  Would it work to append the new empty string at the end of
> the existing list, as opposed to prepending it?

No, because that loop aborts on the first match that "works". Since no flags are necessary on OS X, but adding "-mt" to the flags "works" in the sense that it does nothing (except triggering a warning about an unknown argument), we need to check the empty string before "-mt" that. 

>  I'd prefer a
> solution that is order independent, or if the change is order
> dependent, then a comment to warn others from changing it later.

Well, such checks are normally always order dependant, too (looking at many other autoconf tests out there). OK, so we could first test all four possibilities, recording for each whether it works or not. But after doing that, we still need to establish an order, in case more than one "works" according to the linker test. Simply using the order is the easiest way for that and works well in practice. And it avoid unnecessary, time consuming checks ;).

Regardless of all that, of course I can add a comment emphasising that the order here is important. (Although IMHO that should be self-evident for an autoconf test.)



> 
>>      old_CFLAGS="$CFLAGS"
>>      CFLAGS="$opt $CFLAGS"
>>      AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])
> 
> Perhaps "for linking with POSIX Threads" would make it clearer, as
> CFLAGS (rather, PTHREAD_CFLAGS) has been checked earlier separately.

Well, talking about clarity, looking at line 188 it says
   AC_MSG_NOTICE([Will try -pthread then -lpthread to enable POSIX Threads])

which is also bad: It is out-of-date (even before my patch, as it doesn't mention -mt), will easily get out of sync with reality again, and in any case contains information that normally shouldn't be printed anyway. 

Beyond that, the pthread code checks only for -pthread and -lpthread, thus leaving many systems out. Indeed, it might be worth a thought dropping the custom pthread detection code in git's configure.ac and instead using AX_PTHREAD <http://www.gnu.org/software/autoconf-archive/ax_pthread.html>, which covers many more systems out of the box. 

But for now, I really would just want to make this simple trivial fix that avoids tons of pointless warnings when compiling git on Mac OS X 10.7 ... ;).


Cheers,
Max

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

>>> diff --git a/configure.ac b/configure.ac
>>> index 4e9012f..d767ef3 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -1002,7 +1002,7 @@ if test -n "$USER_NOPTHREAD"; then
>>> # -D_REENTRANT' or some such.
>>> elif test -z "$PTHREAD_CFLAGS"; then
>>>   threads_found=no
>>> -  for opt in -mt -pthread -lpthread; do
>>> +  for opt in "" -mt -pthread -lpthread; do
>> 
>> Hmph.  Would it work to append the new empty string at the end of
>> the existing list, as opposed to prepending it?
>
> No, because that loop aborts on the first match that "works". Since no flags are necessary on OS X, but adding "-mt" to the flags "works" in the sense that it does nothing (except triggering a warning about an unknown argument), we need to check the empty string before "-mt" that. 

If the test in that "for opt ...; do" considers the linking "work",
why do you even want to tweak it, and instead let "-mt" be passed?

If the warning troubles you, would it be feasible for the purpose of
the check to tweak the definition of "works" used in the loop so that
it considers the warning as "not working"?

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Max Horn]

On 09.07.2012, at 19:44, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>>>> diff --git a/configure.ac b/configure.ac
>>>> index 4e9012f..d767ef3 100644
>>>> --- a/configure.ac
>>>> +++ b/configure.ac
>>>> @@ -1002,7 +1002,7 @@ if test -n "$USER_NOPTHREAD"; then
>>>> # -D_REENTRANT' or some such.
>>>> elif test -z "$PTHREAD_CFLAGS"; then
>>>>  threads_found=no
>>>> -  for opt in -mt -pthread -lpthread; do
>>>> +  for opt in "" -mt -pthread -lpthread; do
>>> 
>>> Hmph.  Would it work to append the new empty string at the end of
>>> the existing list, as opposed to prepending it?
>> 
>> No, because that loop aborts on the first match that "works". Since no flags are necessary on OS X, but adding "-mt" to the flags "works" in the sense that it does nothing (except triggering a warning about an unknown argument), we need to check the empty string before "-mt" that. 
> 
> If the test in that "for opt ...; do" considers the linking "work",
> why do you even want to tweak it, and instead let "-mt" be passed?
> 
> If the warning troubles you,

It does trouble me, as it means that every single compiled file now triggers a warning, making it impossible to use -Werror, or alternatively much harder to spot legit warning. In fact, I am surprised that it doesn't seem to trouble *you* :-).

Moreover, while compiling and link "works" with "-mt" present on my system, it is very easy to imagine a setup where this test does break: namely on a system where pthreads are in the C lib, but the C compiler treats unknown compiler options as an error, not a warning. Then none of the three options the test checks right now would work

As I see it, an autoconf feature test like this should be checking "which linker flags are *required* in order to use pthreads?". But what it currently does is to check "which linker flags do not prevent us from (and possibly help us to) use pthreads?"  and so they come up with flags that are not necessary, and in fact trigger warnings.


> would it be feasible for the purpose of
> the check to tweak the definition of "works" used in the loop so that
> it considers the warning as "not working"?

That would be possible, and probably a good idea. But it is also completely orthogonal to my patch. Indeed, if done without my patch, then as a result, pthreads would not be detected anymore on Mac OS X, since none of the linker flags it tries would work -- as it doesn't try what happens when no linker flags are passed.



Cheers,
Max

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

>> would it be feasible for the purpose of
>> the check to tweak the definition of "works" used in the loop so that
>> it considers the warning as "not working"?
>
> That would be possible, and probably a good idea. But it is also
> completely orthogonal to my patch. Indeed, if done without my
> patch,...

No, I was suggesting it as a possible way to make the addition of ""
order independent (which you said is impossible in your initial
reply).

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Max Horn]

On 09.07.2012, at 21:23, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>>> would it be feasible for the purpose of
>>> the check to tweak the definition of "works" used in the loop so that
>>> it considers the warning as "not working"?
>> 
>> That would be possible, and probably a good idea. But it is also
>> completely orthogonal to my patch. Indeed, if done without my
>> patch,...
> 
> No, I was suggesting it as a possible way to make the addition of ""
> order independent (which you said is impossible in your initial
> reply).

This would make things "order independent" for the specific subset of pthread implementations git supports right now. But there are systems where e.g. both -lpthreads and -lpthread work (link correctly, produce no warnings), but only one provides a POSIX compliant pthread implementation. For such systems, order will play a role, no matter what. Granted, git does not yet support such systems (with regards to pthreads, at least) at all. 

But all in all, I don't understand why this order independence seems to be so important? To me it seems merely an illusion anyway, not worth the extra effort.

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> But all in all, I don't understand why this order independence
> seems to be so important?

Not so important as long as it is made clear for later people to
patch that part of the code.  I just wanted to make sure that
somebody thought hard enough to judge that it is not worth the
effort to make it independent of the orders with a justification
better than that simply we are too lazy to do so ;-).

Re: [PATCH] Change configure to check if pthreads are usable without any extra flags

[Max Horn]

On 10.07.2012, at 00:38, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>> But all in all, I don't understand why this order independence
>> seems to be so important?
> 
> Not so important as long as it is made clear for later people to
> patch that part of the code.  I just wanted to make sure that
> somebody thought hard enough to judge that it is not worth the
> effort to make it independent of the orders with a justification
> better than that simply we are too lazy to do so ;-).

Understood ;). Well, in any case, I'll create a new patch based on your feedback. (And also for the revisions.txt docs, from that other thread), but this week I am extra busy at work, so it might take me a day or two longer, sorry.

Max