[igt-dev] [PATCH i-g-t v3 1/4] meson: add libatomic dependency

[Guillaume Tucker]

On 19/06/2019 07:42, Ser, Simon wrote:
> On Tue, 2019-06-18 at 17:03 +0100, Guillaume Tucker wrote:
>> On 18/06/2019 15:37, Ser, Simon wrote:
>>> On Tue, 2019-06-18 at 14:59 +0100, Guillaume Tucker wrote:
>>>> On 18/06/2019 14:20, Ser, Simon wrote:
>>>>> On Tue, 2019-06-18 at 13:27 +0100, Guillaume Tucker wrote:
>>>>>> Add conditional dependency on libatomic in order to be able to use the
>>>>>> __atomic_* functions instead of the older __sync_* ones.  The
>>>>>> libatomic library is only needed when there aren't any native support
>>>>>> on the current architecture, so a linker test is used for this
>>>>>> purpose.  This enables atomic operations to be on a wider number of
>>>>>> architectures including MIPS.
>>>>>>
>>>>>> Signed-off-by: Guillaume Tucker <guillaume.tucker at collabora.com>
>>>>>> ---
>>>>>>
>>>>>> Notes:
>>>>>>     v2: add linker test for libatomic
>>>>>>     v3: use null_dep
>>>>>>
>>>>>>  meson.build | 14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/meson.build b/meson.build
>>>>>> index 6268c58d3634..118ad667ffb5 100644
>>>>>> --- a/meson.build
>>>>>> +++ b/meson.build
>>>>>> @@ -180,6 +180,20 @@ realtime = cc.find_library('rt')
>>>>>>  dlsym = cc.find_library('dl')
>>>>>>  zlib = cc.find_library('z')
>>>>>>  
>>>>>> +if cc.links('''
>>>>>> +#include <stdint.h>
>>>>>> +int main(void) {
>>>>>> +  uint32_t x32 = 0;
>>>>>> +  uint64_t x64 = 0;
>>>>>> +  __atomic_load_n(&x32, __ATOMIC_SEQ_CST);
>>>>>> +  __atomic_load_n(&x64, __ATOMIC_SEQ_CST);
>>>>>
>>>>> See my reply for v2. I've looked into this a little bit more and it
>>>>> looks like __atomic_* functions are a GCC implementation detail. OIn
>>>>> other words, the C11 standard [1] defines only atomic_* functions, and
>>>>> GCC implements them with __atomic_* builtins when the platform supports
>>>>> it, but other compilers might not expose those builtins and still
>>>>> support atomic_* functions without them. This also seems to be what [2]
>>>>> explains:
>>>>>
>>>>>> The first set of library functions are named __atomic_*. This set has
>>>>>> been “standardized” by GCC, and is described below. (See also GCC’s
>>>>>> documentation)
>>>>>
>>>>> (Notice the quotes around “standardized”, meaning they are a GCC
>>>>> extension)
>>>>
>>>> Quite, and while the stdatomic.h API is part of the C11 standard,
>>>> libatomic is part of GCC.  So this test is to determine whether
>>>> linking against GCC's libatomic.so is needed for its __atomic_*
>>>> fallback implementation.
>>>>
>>>> It raises the question of what to do with other compilers, but
>>>> igt has other build errors with clang on mips at the moment.
>>>> With a quick search, it looks like its __atomic_* functions are
>>>> part of libclang.so for clang.
>>>
>>> I don't see anything in `readelf -s /usr/lib/libclang.so.8`.
>>
>> Yes, well I did this:
>>
>> $ for f in $(find . -name "*.so"); do strings $f | grep __atomic_load && echo $f; done
>> __atomic_load
>> __atomic_load_1
>> __atomic_load_2
>> __atomic_load_4
>> __atomic_load_8
>> ./gcc/mips-linux-gnu/8/libatomic.so
>> __atomic_load
>> __atomic_load_1
>> __atomic_load_2
>> __atomic_load_4
>> __atomic_load_8
>> __atomic_load_16
>> ./mips-linux-gnu/libLLVM-7.so
>>
>> although it's true that they don't appear as proper symbols with
>> readelf.  It would take a bit more investigation in the LLVM
>> source code to get to the bottom of that, but I don't think it's
>> necessary to solve the problem at hand.
> 
> Are you sure these are not undefined symbols? (That is, symbols used in
> the library because it's linked to libatomic)

I'm not sure but I would be surprised if LLVM was linked against
GCC's libatomic library.

>>>> Maybe this test should only be used when the compiler name is
>>>> gcc?  In practice it does work with both gcc and clang though, as
>>>> they both use the same naming convention for atomic built-ins.
>>>
>>> Hmm. I'm still not quite sure I understand why checking with __atomic_*
>>> is preferred.
>>>
>>> - If the compiler has __atomic_* builtins: this won't link with
>>>   libatomic
>>> - If the compiler doesn't have __atomic_* builtins: this will link with
>>>   libatomic even if stdatomic.h works without it
>>>
>>> What we're really interested in is stdatomic.h support, not __atomic_*.
>>> So I still think checking for atomic_* is better than __atomic_*. Am I
>>> missing something?
>>
>> I think the issue is that there is no absolute relationship
>> between stdatomic.h and the __atomic_* functions.  So the test is
>> currently designed from libatomic's point of view, and it might
>> add libatomic dependency even if stdatomic.h doesn't use the
>> __atomic_* functions.  Then conversely, using the C11 atomic_*
>> instead in the test means that we would add dependency on
>> libatomic if it fails to link without being completely sure that
>> it is the missing library.
>>
>> If you take the current test on its own, it doesn't claim to
>> cover stdatomic.h support but just libatomic dependency.  So
>> that's why I prefer it.
>>
>> But in practice, both variants (__atomic_* and C11 atomic_*) can
>> be used in the test with existing versions of GCC and I'm not
>> trying to cover Clang MIPS builds in this series.  I think
>> there's no perfect solution here, and if you still think it makes
>> more sense to use the C11 atomic_* functions then fine, I can
>> change the test to do that instead.
> 
> Fair enough. We can adjust the check when needed.
> 
> Reviewed-by: Simon Ser <simon.ser at intel.com>

Thanks,
Guillaume

>>>>> [1]: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
>>>>> [2]: https://llvm.org/docs/Atomics.html
>>>>>
>>>>>> +  return 0;
>>>>>> +}''', name : 'built-in atomics')
>>>>>> +	libatomic = null_dep
>>>>>> +else
>>>>>> +	libatomic = cc.find_library('atomic')
>>>>>> +endif
>>>>>> +
>>>>>>  if cc.has_header('linux/kd.h')
>>>>>>  	config.set('HAVE_LINUX_KD_H', 1)
>>>>>>  endif