[Mesa-dev] [PATCH 1/2] configure: check if -latomic is needed for __atomic_*

[Emil Velikov]

On 19 September 2017 at 10:12, Grazvydas Ignotas <notasas at gmail.com> wrote:
> On Mon, Sep 18, 2017 at 11:30 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Mon, Sep 18, 2017 at 12:28 PM, Grazvydas Ignotas <notasas at gmail.com> wrote:
>>> On some platforms, gcc generates library calls when __atomic_* functions
>>> are used, but does not link the required library automatically. Detect
>>> this and add the library when needed.
>>>
>>> This change was tested on armel (library was added) and on x86_64 (was
>>> not, as expected).
>>>
On one hand I see the reason why things are as-is. On the other its
rather odd that I couldn't find an official GCC doc that describes the
lot.
Most likely I failed at searching.

Can anyone point me to the man page that recommends this type of checks/linking?
Objections if we add it to the commit message?

>>> Fixes: 8915f0c0 "util: use GCC atomic intrinsics with explicit memory model"
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102573
>>> Signed-off-by: Grazvydas Ignotas <notasas at gmail.com>
>>> ---
>>>  configure.ac         | 13 +++++++++++++
>>>  src/util/Makefile.am |  3 ++-
>>>  2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 605c9b4..b46f29d 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -377,12 +377,25 @@ int main() {
>>>      int n;
>>>      return __atomic_load_n(&n, __ATOMIC_ACQUIRE);
>>>  }]])], GCC_ATOMIC_BUILTINS_SUPPORTED=1)
>>>  if test "x$GCC_ATOMIC_BUILTINS_SUPPORTED" = x1; then
>>>      DEFINES="$DEFINES -DUSE_GCC_ATOMIC_BUILTINS"
>>> +    dnl On some platforms, new-style atomics need a helper library
>>> +    AC_MSG_CHECKING(whether -latomic is needed)
>>> +    AC_LINK_IFELSE([AC_LANG_SOURCE([[
>>> +    #include <stdint.h>
>>> +    uint64_t v;
>>> +    int main() {
>>> +        return (int)__atomic_load_n(&v, __ATOMIC_ACQUIRE);
>>> +    }]])], GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC=no, GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC=yes)
>>> +    AC_MSG_RESULT($GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC)
>>> +    if test "x$GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC" = xyes; then
>>> +        LIBATOMIC_LIBS="-latomic"
>>> +    fi
>>>  fi
>>>  AM_CONDITIONAL([GCC_ATOMIC_BUILTINS_SUPPORTED], [test x$GCC_ATOMIC_BUILTINS_SUPPORTED = x1])
>>> +AC_SUBST([LIBATOMIC_LIBS])
>>>
>>>  dnl Check if host supports 64-bit atomics
>>>  dnl note that lack of support usually results in link (not compile) error
>>>  AC_MSG_CHECKING(whether __sync_add_and_fetch_8 is supported)
>>>  AC_LINK_IFELSE([AC_LANG_SOURCE([[
>>> diff --git a/src/util/Makefile.am b/src/util/Makefile.am
>>> index 9885bbe..c37afff 100644
>>> --- a/src/util/Makefile.am
>>> +++ b/src/util/Makefile.am
>>> @@ -46,11 +46,12 @@ libmesautil_la_SOURCES = \
>>>         $(MESA_UTIL_FILES) \
>>>         $(MESA_UTIL_GENERATED_FILES)
>>>
>>>  libmesautil_la_LIBADD = \
>>>         $(CLOCK_LIB) \
>>> -       $(ZLIB_LIBS)
>>> +       $(ZLIB_LIBS) \
>>> +       $(LIBATOMIC_LIBS)
>>
>> I can't remember how this works -- will $(LIBATOMIC_LIBS) be added
>> transitively to anything that LDADDs libmesautil? I hope so...
>
> I'm not sure either, I've simply tried it and it seemed to work. Maybe
> Emil can comment if it's the right way...
>
Yes, adding LIBATOMIC_LIBS here will be propagated into all the
targets that use libmesautil.la

>> Oh, also, these should get a Cc: mesa-stable tag.
>
> It has been said (and documented in docs/submittingpatches.html) that
> Fixes: tag is enough for stable, and 2/2 it's strictly necessary I
> think.
True. Adding the stable won't hurt ;-)

Thanks
Emil