On Fri, Mar 30, 2018 at 2:26 AM, Matt Turner <[email protected]> wrote: > On Thu, Mar 29, 2018 at 1:31 AM, Nicolas Boichat <[email protected]> > wrote: >> From: Nicolas Boichat <[email protected]> >> >> When compiling with LLVM 6.0, the test fails to detect that >> -latomic is actually required, as the atomic call is inlined. >> >> In the code itself (src/util/disk_cache.c), we see this pattern: >> p_atomic_add(cache->size, - (uint64_t)size); >> where cache->size is an uint64_t *, and results in the following >> link time error without -latomic: >> src/util/disk_cache.c:628: error: undefined reference to >> '__atomic_fetch_add_8' >> >> Fix the configure/meson test to replicate this pattern, which then >> correctly realizes the need for -latomic. >> >> Signed-off-by: Nicolas Boichat <[email protected]> >> --- >> >> Changes since v1: >> - Updated meson.build as well (untested) >> >> configure.ac | 6 ++++-- >> meson.build | 6 ++++-- >> 2 files changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/configure.ac b/configure.ac >> index e874f8ebfb2..eff9a0ef88f 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -445,9 +445,11 @@ if test "x$GCC_ATOMIC_BUILTINS_SUPPORTED" = x1; then >> AC_MSG_CHECKING(whether -latomic is needed) >> AC_LINK_IFELSE([AC_LANG_SOURCE([[ >> #include <stdint.h> >> - uint64_t v; >> + struct { >> + uint64_t* v; > > I wouldn't care expect that you put the * with the v in the Meson case. :)
Argh ,-( I'll send a v3, let's see if anyone has further comments, first. > Also, on what platform does this occur? This is ARC++ (Android 32-bit x86) with clang version: Android (4639204 based on r316199) clang version 6.0.1 (https://android.googlesource.com/toolchain/clang 279c0d3a962121a6d1d535e7b0b5d9d36d3c829d) (https://android.googlesource.com/toolchain/llvm aadd87ffb6a2eafcb577913073d46b20195a9cdc) (based on LLVM 6.0.1svn) > Looking at this code, I would expect it to behave the same as before. > Do you have an idea why this fixes it, or why the original code didn't > work? I'm guess it's about the compiler's ability to recognize that it > knows the location of the variable. With the original code, objdump looks like this: 08048400 <main>: 8048400: 53 push %ebx 8048401: 56 push %esi 8048402: e8 00 00 00 00 call 8048407 <main+0x7> 8048407: 5e pop %esi 8048408: 81 c6 ed 1b 00 00 add $0x1bed,%esi 804840e: 31 c0 xor %eax,%eax 8048410: 31 d2 xor %edx,%edx 8048412: 31 c9 xor %ecx,%ecx 8048414: 31 db xor %ebx,%ebx 8048416: f0 0f c7 8e 24 00 00 lock cmpxchg8b 0x24(%esi) 804841d: 00 804841e: 5e pop %esi 804841f: 5b pop %ebx 8048420: c3 ret Looks like LLVM figures out that &v is constant, and uses some 64-bit atomic swap operations on it directly. With the updated code (building with -latomic, it fails otherwise) 08048480 <main>: 8048480: 53 push %ebx 8048481: 83 ec 08 sub $0x8,%esp 8048484: e8 00 00 00 00 call 8048489 <main+0x9> 8048489: 5b pop %ebx 804848a: 81 c3 6b 1b 00 00 add $0x1b6b,%ebx 8048490: 83 ec 08 sub $0x8,%esp 8048493: 6a 02 push $0x2 8048495: ff b3 8c 10 00 00 pushl 0x108c(%ebx) 804849b: e8 05 00 00 00 call 80484a5 <__atomic_load_8> 80484a0: 83 c4 18 add $0x18,%esp 80484a3: 5b pop %ebx 80484a4: c3 ret I think the the code is trying to protect both x.v (address) _and_ its value *x.v? Or maybe LLVM does not see the pattern... (I don't see why cmpxchg8b wouldn't work here too, otherwise...) Actually, the test can be made simpler, by just using: uint64_t *v; ... __atomic_load_n(v, ... But then it does not match the usage pattern in the code, so I feel a little bit more confident that the current test will actually capture when -latomic is needed. Thanks, _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
