[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 <squid3 at treenet.co.nz> 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
>> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=770928>). 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.ac    2016-01-04 14:39:06 +0000
> +++ configure.ac    2016-01-04 18:25:48 +0000
> @@ -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