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

[Matt Turner]

On Thu, Mar 29, 2018 at 4:10 PM, Nicolas Boichat <drinkcat at chromium.org> wrote:
> 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.

Yeah. That makes sense, and seems like a good idea to me.