[Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

[Nicolas Boichat]

On Fri, Mar 30, 2018 at 2:26 AM, Matt Turner <mattst88 at gmail.com> wrote:
> On Thu, Mar 29, 2018 at 1:31 AM, Nicolas Boichat <drinkcat at chromium.org> wrote:
>> From: Nicolas Boichat <drinkcat at chromium.org>
>>
>> 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 <drinkcat at chromium.org>
>> ---
>>
>> 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,