Re: [squid-dev] [PATCH] Fix libatomic detection

[Kinkie]

Hi,
  I haven't checked, but it should.
MacOS builds for me right now; probably as a result of the LIBS
side-effect (although I get a "none required" result).
Attached patch fixes, build still failes due to shm not working at
runtime (thus testRock failing), but for me it's a fix candidate.

On Tue, Jan 5, 2016 at 9:06 AM, Amos Jeffries  wrote:
> On 5/01/2016 7:34 a.m., Kinkie wrote:
>> On Mon, Jan 4, 2016 at 6:47 AM, Amos Jeffries  wrote:
>>> On 28/12/2015 10:25 p.m., Kinkie wrote:
 Hi,

 On Mon, Dec 28, 2015 at 1:32 AM, Amos Jeffries wrote:
> On 24/12/2015 11:32 a.m., Kinkie wrote:
>> Hi,
>>   libatomic detection is currently broken in configure.ac; it will
>> define -latomic even in case where it wouldn't be required (e.g.
>> because it's already provided by the compiler).
>> The attached patch fixes the broken case. Unfortunately I don't know a
>> system where this library is required, I'm convinced there's room for
>> further simplification.
>
> It was for Clang builds IIRC. At least 3.5 needs it added.
>
> Does this work instead for shorter code?
>   AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
> ATOMICLIB="$ac_cv_search___atomic_load_8"
>   ],[])

 unfortunately not, as the result could be "none required" or "no",
 too. The former is actually the error case which bought me to the
 topic.
 Documentation states that if a library is needed it should get
 automatically added to LIBS though.
>>>
>>> "Unless the 3rd and 4th parameters are specified". So it will be
>>> automatically added under your patch, but not under the existing/older code.
>>>
>>> Which was an intentional omission from LIBS to avoid build warnings on
>>> some distros about unnecessary dependencies being linked in (such as
>>> ). Since most
>>> of the helper binaries do not use atomics (yet, though we could thread
>>> them in future) the -latomic addition is only needed by squid_LDADD not
>>> the generic LIBS / LDADD.
>>>
>>> Amos
>>>
>>
>> Are you sure? This from the doc I could find:
>> ===
>> the default action-if-found code, adding the library to the LIBS
>> variables, is always executed, even if a parameter is passed
>> ===
>>
>> I have no objection on the intention, of course.
>> If you are right, then the one-liner below should then do the trick.
>> If I am right, we need to go through the
>> SQUID_SAVE_STATE/SQUID_ROLLBACK_STATE dance on top of that.
>>
>> === modified file 'configure.ac'
>> --- configure.ac2016-01-04 14:39:06 +
>> +++ configure.ac2016-01-04 18:25:48 +
>> @@ -458,7 +458,7 @@
>>  fi
>>
>>  ## check for atomics library before anything that might need it
>> -AC_SEARCH_LIBS([__atomic_load_8],[atomic])
>> +AC_SEARCH_LIBS([__atomic_load_8],[atomic],[],[])
>>  if test "x$ac_cv_search___atomic_load_8" = "-latomic"; then
>>ATOMICLIB="-latomic"
>>  fi
>>
>
> Seems I was wrong. I have just checked the two versions and the above
> patch generates exactly the same code as current trunk and sets LIBS. So
> we will have to do the state preservation dance. But I think wait until
> we have evidence of complaints for that.
>
> I also see the test is missing the "x" on the RHS of the conditional.
> Which is a bigger problem for clang where it needs to be set.
>
> PS. this is all fixes bug 4393 right?
>
> Amos
>
> ___
> squid-dev mailing list
> squid-dev@lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev



-- 
Francesco
=== modified file 'configure.ac'
--- configure.ac	2016-01-04 14:39:06 +
+++ configure.ac	2016-01-05 08:14:39 +
@@ -458,10 +458,13 @@
 fi
 
 ## check for atomics library before anything that might need it
+# AC_SEARCH_LIBS pollutes LIBS
+SQUID_STATE_SAVE(LIBATOMIC)
 AC_SEARCH_LIBS([__atomic_load_8],[atomic])
-if test "x$ac_cv_search___atomic_load_8" = "-latomic"; then
+if test "x$ac_cv_search___atomic_load_8" = "x-latomic"; then
   ATOMICLIB="-latomic"
 fi
+SQUID_STATE_ROLLBACK(LIBATOMIC)
 AC_SUBST(ATOMICLIB)
 
 AC_SEARCH_LIBS([shm_open], [rt])

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Amos Jeffries]

On 5/01/2016 7:34 a.m., Kinkie wrote:
> On Mon, Jan 4, 2016 at 6:47 AM, Amos Jeffries  wrote:
>> On 28/12/2015 10:25 p.m., Kinkie wrote:
>>> Hi,
>>>
>>> On Mon, Dec 28, 2015 at 1:32 AM, Amos Jeffries wrote:
 On 24/12/2015 11:32 a.m., Kinkie wrote:
> Hi,
>   libatomic detection is currently broken in configure.ac; it will
> define -latomic even in case where it wouldn't be required (e.g.
> because it's already provided by the compiler).
> The attached patch fixes the broken case. Unfortunately I don't know a
> system where this library is required, I'm convinced there's room for
> further simplification.

 It was for Clang builds IIRC. At least 3.5 needs it added.

 Does this work instead for shorter code?
   AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
 ATOMICLIB="$ac_cv_search___atomic_load_8"
   ],[])
>>>
>>> unfortunately not, as the result could be "none required" or "no",
>>> too. The former is actually the error case which bought me to the
>>> topic.
>>> Documentation states that if a library is needed it should get
>>> automatically added to LIBS though.
>>
>> "Unless the 3rd and 4th parameters are specified". So it will be
>> automatically added under your patch, but not under the existing/older code.
>>
>> Which was an intentional omission from LIBS to avoid build warnings on
>> some distros about unnecessary dependencies being linked in (such as
>> ). Since most
>> of the helper binaries do not use atomics (yet, though we could thread
>> them in future) the -latomic addition is only needed by squid_LDADD not
>> the generic LIBS / LDADD.
>>
>> Amos
>>
> 
> Are you sure? This from the doc I could find:
> ===
> the default action-if-found code, adding the library to the LIBS
> variables, is always executed, even if a parameter is passed
> ===
> 
> I have no objection on the intention, of course.
> If you are right, then the one-liner below should then do the trick.
> If I am right, we need to go through the
> SQUID_SAVE_STATE/SQUID_ROLLBACK_STATE dance on top of that.
> 
> === modified file 'configure.ac'
> --- configure.ac2016-01-04 14:39:06 +
> +++ configure.ac2016-01-04 18:25:48 +
> @@ -458,7 +458,7 @@
>  fi
> 
>  ## check for atomics library before anything that might need it
> -AC_SEARCH_LIBS([__atomic_load_8],[atomic])
> +AC_SEARCH_LIBS([__atomic_load_8],[atomic],[],[])
>  if test "x$ac_cv_search___atomic_load_8" = "-latomic"; then
>ATOMICLIB="-latomic"
>  fi
> 

Seems I was wrong. I have just checked the two versions and the above
patch generates exactly the same code as current trunk and sets LIBS. So
we will have to do the state preservation dance. But I think wait until
we have evidence of complaints for that.

I also see the test is missing the "x" on the RHS of the conditional.
Which is a bigger problem for clang where it needs to be set.

PS. this is all fixes bug 4393 right?

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Amos Jeffries]

On 5/01/2016 9:24 p.m., Kinkie wrote:
> Hi,
>   I haven't checked, but it should.
> MacOS builds for me right now; probably as a result of the LIBS
> side-effect (although I get a "none required" result).
> Attached patch fixes, build still failes due to shm not working at
> runtime (thus testRock failing), but for me it's a fix candidate.

This one matches the way AC_SEARCH_LIBS sets LIBS internally to cut off
a few more lines of logic.

Amos
=== modified file 'configure.ac'
--- configure.ac2016-01-04 14:39:06 +
+++ configure.ac2016-01-05 08:53:43 +
@@ -458,10 +458,11 @@
 fi
 
 ## check for atomics library before anything that might need it
-AC_SEARCH_LIBS([__atomic_load_8],[atomic])
-if test "x$ac_cv_search___atomic_load_8" = "-latomic"; then
-  ATOMICLIB="-latomic"
-fi
+# AC_SEARCH_LIBS pollutes LIBS
+SQUID_STATE_SAVE(LIBATOMIC)
+AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
+  test "$ac_res" = "none required" || ATOMICLIB=$ac_res],[])
+SQUID_STATE_ROLLBACK(LIBATOMIC)
 AC_SUBST(ATOMICLIB)
 
 AC_SEARCH_LIBS([shm_open], [rt])

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Kinkie]

looks good. +1

On Tue, Jan 5, 2016 at 10:01 AM, Amos Jeffries  wrote:
> On 5/01/2016 9:24 p.m., Kinkie wrote:
>> Hi,
>>   I haven't checked, but it should.
>> MacOS builds for me right now; probably as a result of the LIBS
>> side-effect (although I get a "none required" result).
>> Attached patch fixes, build still failes due to shm not working at
>> runtime (thus testRock failing), but for me it's a fix candidate.
>
> This one matches the way AC_SEARCH_LIBS sets LIBS internally to cut off
> a few more lines of logic.
>
> Amos



-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Amos Jeffries]

On 5/01/2016 10:41 p.m., Kinkie wrote:
> looks good. +1
> 

Applied as rev.14477.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Kinkie]

On Mon, Jan 4, 2016 at 6:47 AM, Amos Jeffries  wrote:
> On 28/12/2015 10:25 p.m., Kinkie wrote:
>> Hi,
>>
>> On Mon, Dec 28, 2015 at 1:32 AM, Amos Jeffries wrote:
>>> On 24/12/2015 11:32 a.m., Kinkie wrote:
 Hi,
   libatomic detection is currently broken in configure.ac; it will
 define -latomic even in case where it wouldn't be required (e.g.
 because it's already provided by the compiler).
 The attached patch fixes the broken case. Unfortunately I don't know a
 system where this library is required, I'm convinced there's room for
 further simplification.
>>>
>>> It was for Clang builds IIRC. At least 3.5 needs it added.
>>>
>>> Does this work instead for shorter code?
>>>   AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
>>> ATOMICLIB="$ac_cv_search___atomic_load_8"
>>>   ],[])
>>
>> unfortunately not, as the result could be "none required" or "no",
>> too. The former is actually the error case which bought me to the
>> topic.
>> Documentation states that if a library is needed it should get
>> automatically added to LIBS though.
>
> "Unless the 3rd and 4th parameters are specified". So it will be
> automatically added under your patch, but not under the existing/older code.
>
> Which was an intentional omission from LIBS to avoid build warnings on
> some distros about unnecessary dependencies being linked in (such as
> ). Since most
> of the helper binaries do not use atomics (yet, though we could thread
> them in future) the -latomic addition is only needed by squid_LDADD not
> the generic LIBS / LDADD.
>
> Amos
>

Are you sure? This from the doc I could find:
===
the default action-if-found code, adding the library to the LIBS
variables, is always executed, even if a parameter is passed
===

I have no objection on the intention, of course.
If you are right, then the one-liner below should then do the trick.
If I am right, we need to go through the
SQUID_SAVE_STATE/SQUID_ROLLBACK_STATE dance on top of that.

=== modified file 'configure.ac'
--- configure.ac2016-01-04 14:39:06 +
+++ configure.ac2016-01-04 18:25:48 +
@@ -458,7 +458,7 @@
 fi

 ## check for atomics library before anything that might need it
-AC_SEARCH_LIBS([__atomic_load_8],[atomic])
+AC_SEARCH_LIBS([__atomic_load_8],[atomic],[],[])
 if test "x$ac_cv_search___atomic_load_8" = "-latomic"; then
   ATOMICLIB="-latomic"
 fi



-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Amos Jeffries]

On 28/12/2015 10:25 p.m., Kinkie wrote:
> Hi,
> 
> On Mon, Dec 28, 2015 at 1:32 AM, Amos Jeffries wrote:
>> On 24/12/2015 11:32 a.m., Kinkie wrote:
>>> Hi,
>>>   libatomic detection is currently broken in configure.ac; it will
>>> define -latomic even in case where it wouldn't be required (e.g.
>>> because it's already provided by the compiler).
>>> The attached patch fixes the broken case. Unfortunately I don't know a
>>> system where this library is required, I'm convinced there's room for
>>> further simplification.
>>
>> It was for Clang builds IIRC. At least 3.5 needs it added.
>>
>> Does this work instead for shorter code?
>>   AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
>> ATOMICLIB="$ac_cv_search___atomic_load_8"
>>   ],[])
> 
> unfortunately not, as the result could be "none required" or "no",
> too. The former is actually the error case which bought me to the
> topic.
> Documentation states that if a library is needed it should get
> automatically added to LIBS though.

"Unless the 3rd and 4th parameters are specified". So it will be
automatically added under your patch, but not under the existing/older code.

Which was an intentional omission from LIBS to avoid build warnings on
some distros about unnecessary dependencies being linked in (such as
). Since most
of the helper binaries do not use atomics (yet, though we could thread
them in future) the -latomic addition is only needed by squid_LDADD not
the generic LIBS / LDADD.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Kinkie]

10 days without vetoes, committing as revno 14474.

On Mon, Dec 28, 2015 at 10:25 AM, Kinkie  wrote:
> Hi,
>
> On Mon, Dec 28, 2015 at 1:32 AM, Amos Jeffries  wrote:
>> On 24/12/2015 11:32 a.m., Kinkie wrote:
>>> Hi,
>>>   libatomic detection is currently broken in configure.ac; it will
>>> define -latomic even in case where it wouldn't be required (e.g.
>>> because it's already provided by the compiler).
>>> The attached patch fixes the broken case. Unfortunately I don't know a
>>> system where this library is required, I'm convinced there's room for
>>> further simplification.
>>
>> It was for Clang builds IIRC. At least 3.5 needs it added.
>>
>> Does this work instead for shorter code?
>>   AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
>> ATOMICLIB="$ac_cv_search___atomic_load_8"
>>   ],[])
>
> unfortunately not, as the result could be "none required" or "no",
> too. The former is actually the error case which bought me to the
> topic.
> Documentation states that if a library is needed it should get
> automatically added to LIBS though.
>
>
> --
> Francesco



-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Kinkie]

Hi,

On Mon, Dec 28, 2015 at 1:32 AM, Amos Jeffries  wrote:
> On 24/12/2015 11:32 a.m., Kinkie wrote:
>> Hi,
>>   libatomic detection is currently broken in configure.ac; it will
>> define -latomic even in case where it wouldn't be required (e.g.
>> because it's already provided by the compiler).
>> The attached patch fixes the broken case. Unfortunately I don't know a
>> system where this library is required, I'm convinced there's room for
>> further simplification.
>
> It was for Clang builds IIRC. At least 3.5 needs it added.
>
> Does this work instead for shorter code?
>   AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
> ATOMICLIB="$ac_cv_search___atomic_load_8"
>   ],[])

unfortunately not, as the result could be "none required" or "no",
too. The former is actually the error case which bought me to the
topic.
Documentation states that if a library is needed it should get
automatically added to LIBS though.


-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Fix libatomic detection

[Amos Jeffries]

On 24/12/2015 11:32 a.m., Kinkie wrote:
> Hi,
>   libatomic detection is currently broken in configure.ac; it will
> define -latomic even in case where it wouldn't be required (e.g.
> because it's already provided by the compiler).
> The attached patch fixes the broken case. Unfortunately I don't know a
> system where this library is required, I'm convinced there's room for
> further simplification.

It was for Clang builds IIRC. At least 3.5 needs it added.

Does this work instead for shorter code?
  AC_SEARCH_LIBS([__atomic_load_8],[atomic],[
ATOMICLIB="$ac_cv_search___atomic_load_8"
  ],[])


Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev