Re: [PATCH 2/3] qemu/atomic: Drop special case for unsupported compiler

[Peter Maydell]

On Mon, 28 Sep 2020 at 14:12, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Since commit efc6c070aca ("configure: Add a test for the
> minimum compiler version") the minimum compiler version
> required for GCC is 4.8, which has the GCC BZ#36793 bug fixed.
>
> We can safely remove the special case introduced in commit
> a281ebc11a6 ("virtio: add missing mb() on notification").

> -/*
> - * We use GCC builtin if it's available, as that can use mfence on
> - * 32-bit as well, e.g. if built with -march=pentium-m. However, on
> - * i386 the spec is buggy, and the implementation followed it until
> - * 4.3 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36793).
> - */
> -#if defined(__i386__) || defined(__x86_64__)
> -#if !QEMU_GNUC_PREREQ(4, 4)
> -#if defined __x86_64__
> -#define smp_mb()    ({ asm volatile("mfence" ::: "memory"); (void)0; })
> -#else
> -#define smp_mb()    ({ asm volatile("lock; addl $0,0(%%esp) " ::: "memory"); 
> (void)0; })
> -#endif
> -#endif
> -#endif

You need to be a bit cautious about removing QEMU_GNUC_PREREQ()
checks, because clang advertises itself as GCC 4.2 via the
__GNUC__ and __GNUC_MINOR__ macros that QEMU_GNUC_PREREQ()
tests, and so unless the code has explicitly handled clang
via a different ifdef or feature test clang will be using
the fallback codepath in cases like this. So you also need
to find out whether all our supported versions of clang
are OK with the gcc-4.4-and-up version of this code...
(I think that deleting this set of #ifs means we end up
with the "just use __sync_synchronize()" version of smp_mb()
later on in the header?)

thanks
-- PMM