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

[Ser, Simon]

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)

> > > 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>

> 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