[cfe-dev] [Release-testers] [7.0.0 Release] rc1 has been tagged

[Hans Wennborg via cfe-dev]

On Tue, Aug 7, 2018 at 9:49 PM, Dimitry Andric <dimitry at andric.com> wrote:
> On 6 Aug 2018, at 18:23, Hans Wennborg <hans at chromium.org> wrote:
>>
>> On Sun, Aug 5, 2018 at 5:49 PM, Dimitry Andric <dimitry at andric.com> wrote:
>>> On 3 Aug 2018, at 13:37, Hans Wennborg via Release-testers <release-testers at lists.llvm.org> wrote:
>>>>
>>>> 7.0.0-rc1 was just tagged (from the branch at r338847).
> ...
>>
>>> Hmm, I'm running into a rather nasty snag now on i386-freebsd11, due to our lack of atomic 64 bit primitives; Phase2's configure dies with:
>>>
>>>    -- Performing Test HAVE_CXX_ATOMICS_WITHOUT_LIB
>>>    -- Performing Test HAVE_CXX_ATOMICS_WITHOUT_LIB - Success
>>>    -- Performing Test HAVE_CXX_ATOMICS64_WITHOUT_LIB
>>>    -- Performing Test HAVE_CXX_ATOMICS64_WITHOUT_LIB - Failed
>>>    -- Looking for __atomic_load_8 in atomic
>>>    -- Looking for __atomic_load_8 in atomic - not found
> ...
>>> Apparently, with clang 6.0 it didn't generate libcalls to atomic functions, but just put in cmpxchg8b's, I guess?  And with clang 7.0 this seems to have changed.
> ...
>> Did you file a bug to track this? (Sorry if you already did, I haven't
>> read through the list today.)
>
> This is a regression caused by https://reviews.llvm.org/rL323281:
>
> ------------------------------------------------------------------------
> r323281 | wmi | 2018-01-23 23:27:57 +0000 (Tue, 23 Jan 2018) | 12 lines
>
> Adjust MaxAtomicInlineWidth for i386/i486 targets.
>
> This is to fix the bug reported in https://bugs.llvm.org/show_bug.cgi?id=34347#c6.
> Currently, all  MaxAtomicInlineWidth of x86-32 targets are set to 64. However,
> i386 doesn't support any cmpxchg related instructions. i486 only supports cmpxchg.
> So in this patch MaxAtomicInlineWidth is reset as follows:
> For i386, the MaxAtomicInlineWidth should be 0 because no cmpxchg is supported.
> For i486, the MaxAtomicInlineWidth should be 32 because it supports cmpxchg.
> For others 32 bits x86 cpu, the MaxAtomicInlineWidth should be 64 because of cmpxchg8b.
>
> Differential Revision: https://reviews.llvm.org/D42154
> ------------------------------------------------------------------------
>
> I'm not sure whether we should report this as an LLVM bug though, since
> the problem really lies with FreeBSD: we never had proper atomic
> libcalls in our libc (at least the 64 bit ones for 32-bit x86), nor a
> separate libatomic.
>
> Before r323281, and with all released versions of clang to date, it
> actually generated incorrect instructions for the default target CPU,
> if you used the triple i386-unknown-freebsd, e.g.:
>
> $ ~/obj/clang-323280/bin/clang -m32 -O2 -S atomic-test.cpp -o -
> [...]
>         pushl   %ebp
>         movl    %esp, %ebp
>         pushl   %ebx
>         xorl    %eax, %eax
>         xorl    %edx, %edx
>         xorl    %ecx, %ecx
>         xorl    %ebx, %ebx
>         lock            cmpxchg8b       x
>         xorl    %eax, %eax
>         popl    %ebx
>         popl    %ebp
>         retl
>
> After r323281, this is corrected:
>
> $ ~/obj/clang-323281/bin/clang -m32 -O2 -S atomic-test.cpp -o -
> [...]
>         pushl   %ebp
>         movl    %esp, %ebp
>         pushl   $0
>         pushl   $x
>         calll   __atomic_load_8
>         addl    $8, %esp
>         xorl    %eax, %eax
>         popl    %ebp
>         retl
>
> Targeting i586 or higher makes the cmpxchg8b come back:
>
> $ ~/obj/clang-323281/bin/clang -m32 -march=i586 -O2 -S atomic-test.cpp -o -
> [...]
>         pushl   %ebp
>         movl    %esp, %ebp
>         pushl   %ebx
>         xorl    %eax, %eax
>         xorl    %edx, %edx
>         xorl    %ecx, %ecx
>         xorl    %ebx, %ebx
>         lock            cmpxchg8b       x
>         xorl    %eax, %eax
>         popl    %ebx
>         popl    %ebp
>         retl
>
> I think there are other scenarios where you could cause atomic libcalls
> to appear, though, even if you target higher CPUs.
>
> That said, we'll have to at least discuss this in the FreeBSD community,
> since I think it is high time it is fixed on *that* side.

Thanks for the explanation. Let me know what comes out of the discussion.

Cheers,
Hans