[PATCH 0/3] qemu/compiler: Remove unused special case code for GCC < 4.8

[PATCH 0/3] qemu/compiler: Remove unused special case code for GCC < 4.8

[Philippe Mathieu-Daudé]

Since commit efc6c070aca we require GCC 4.8 minimum.
Drop the special cases for older versions.

Philippe Mathieu-Daudé (3):
  qemu/compiler: Simplify as all compilers support attribute
    'gnu_printf'
  qemu/atomic: Drop special case for unsupported compiler
  accel/tcg: Remove special case for GCC < 4.6

 include/qemu/atomic.h   | 17 -----------------
 include/qemu/compiler.h | 19 ++++++++-----------
 accel/tcg/cpu-exec.c    |  2 +-
 3 files changed, 9 insertions(+), 29 deletions(-)

-- 
2.26.2

[PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Philippe Mathieu-Daudé]

Since commit efc6c070aca ("configure: Add a test for the minimum
compiler version") the minimum compiler version required for GCC
is 4.8, which supports the gnu_printf attribute.

We can safely remove the code introduced in commit 9c9e7d51bf0
("Move macros GCC_ATTR and GCC_FMT_ATTR to common header file").

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/compiler.h | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index c76281f3540..207e3bd4feb 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -104,17 +104,14 @@
                                    sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)))
 
 #if defined __GNUC__
-# if !QEMU_GNUC_PREREQ(4, 4)
-   /* gcc versions before 4.4.x don't support gnu_printf, so use printf. */
-#  define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m)))
-# else
-   /* Use gnu_printf when supported (qemu uses standard format strings). */
-#  define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m)))
-#  if defined(_WIN32)
-    /* Map __printf__ to __gnu_printf__ because we want standard format strings
-     * even when MinGW or GLib include files use __printf__. */
-#   define __printf__ __gnu_printf__
-#  endif
+  /* Use gnu_printf when supported (qemu uses standard format strings). */
+# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m)))
+# if defined(_WIN32)
+   /*
+    * Map __printf__ to __gnu_printf__ because we want standard format strings
+    * even when MinGW or GLib include files use __printf__.
+    */
+#  define __printf__ __gnu_printf__
 # endif
 #else
 #define GCC_FMT_ATTR(n, m)
-- 
2.26.2

Re: [PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Peter Maydell]

On Mon, 28 Sep 2020 at 14:37, 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 supports the gnu_printf attribute.
>
> We can safely remove the code introduced in commit 9c9e7d51bf0
> ("Move macros GCC_ATTR and GCC_FMT_ATTR to common header file").

clang doesn't support the gnu_printf attribute, though:

$ clang --version
clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ cat /tmp/zz9.c
#include <stdio.h>

int monitor_printf(void *mon, const char *fmt, ...)
__attribute__((format(gnu_printf, 2, 3)));

int main(void) {
  printf("hello\n");
  return 0;
}
$ clang -Wall -o /tmp/zz9  /tmp/zz9.c
/tmp/zz9.c:3:68: warning: 'format' attribute argument not supported:
gnu_printf [-Wignored-attributes]
int monitor_printf(void *mon, const char *fmt, ...)
__attribute__((format(gnu_printf, 2, 3)));
                                                                   ^
1 warning generated.

thanks
-- PMM

Re: [PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Daniel P. Berrangé]

On Mon, Sep 28, 2020 at 02:58:57PM +0200, Philippe Mathieu-Daudé wrote:
> Since commit efc6c070aca ("configure: Add a test for the minimum
> compiler version") the minimum compiler version required for GCC
> is 4.8, which supports the gnu_printf attribute.
> 
> We can safely remove the code introduced in commit 9c9e7d51bf0
> ("Move macros GCC_ATTR and GCC_FMT_ATTR to common header file").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/compiler.h | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index c76281f3540..207e3bd4feb 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -104,17 +104,14 @@
>                                     sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)))
>  
>  #if defined __GNUC__
> -# if !QEMU_GNUC_PREREQ(4, 4)
> -   /* gcc versions before 4.4.x don't support gnu_printf, so use printf. */
> -#  define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m)))
> -# else
> -   /* Use gnu_printf when supported (qemu uses standard format strings). */
> -#  define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m)))
> -#  if defined(_WIN32)
> -    /* Map __printf__ to __gnu_printf__ because we want standard format strings
> -     * even when MinGW or GLib include files use __printf__. */
> -#   define __printf__ __gnu_printf__
> -#  endif
> +  /* Use gnu_printf when supported (qemu uses standard format strings). */
> +# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m)))
> +# if defined(_WIN32)
> +   /*
> +    * Map __printf__ to __gnu_printf__ because we want standard format strings
> +    * even when MinGW or GLib include files use __printf__.
> +    */
> +#  define __printf__ __gnu_printf__
>  # endif
>  #else
>  #define GCC_FMT_ATTR(n, m)

I think this can be simplified even more by using GLib's macros

  #define GCC_FMT_ATTR(n, m)  G_GNUC_PRINTF(n, m)

and ideally we'd then convert all crrent usage to the latter and
drop GCC_FMT_ATTR.

https://developer.gnome.org/glib/2.64/glib-Miscellaneous-Macros.html

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Peter Maydell]

On Mon, 28 Sep 2020 at 15:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
> I think this can be simplified even more by using GLib's macros
>
>   #define GCC_FMT_ATTR(n, m)  G_GNUC_PRINTF(n, m)

At least on my system G_GNUC_PRINTF() expands to
__format__(__printf__,...), not gnu_printf, so it is
not quite what we want. (The difference is that on Windows
hosts we still want to mark up our our logging functions as
taking the glibc style format handling, not whatever the
MS C library format escapes happen to be.)
At a minimum you'd need to keep in the "on Windows,
redefine __printf__ to __gnu_printf__" logic.

See also commit 95df51a4a02a853.

thanks
-- PMM

Re: [PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Daniel P. Berrangé]

On Mon, Sep 28, 2020 at 03:14:45PM +0100, Peter Maydell wrote:
> On Mon, 28 Sep 2020 at 15:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > I think this can be simplified even more by using GLib's macros
> >
> >   #define GCC_FMT_ATTR(n, m)  G_GNUC_PRINTF(n, m)
> 
> At least on my system G_GNUC_PRINTF() expands to
> __format__(__printf__,...), not gnu_printf, so it is
> not quite what we want. (The difference is that on Windows
> hosts we still want to mark up our our logging functions as
> taking the glibc style format handling, not whatever the
> MS C library format escapes happen to be.)
> At a minimum you'd need to keep in the "on Windows,
> redefine __printf__ to __gnu_printf__" logic.
> 
> See also commit 95df51a4a02a853.

Oh, that's a bug in old GLib versions. I thought we had a new enough
min to avoid that problem, but i guess not after all.

Modern GLib always uses gnu_printf even on Windows, as they're using a
replacement GNU compatible printf impl for all the GLib APIs that take
format strings.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Peter Maydell]

On Mon, 28 Sep 2020 at 15:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Sep 28, 2020 at 03:14:45PM +0100, Peter Maydell wrote:
> > On Mon, 28 Sep 2020 at 15:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > I think this can be simplified even more by using GLib's macros
> > >
> > >   #define GCC_FMT_ATTR(n, m)  G_GNUC_PRINTF(n, m)
> >
> > At least on my system G_GNUC_PRINTF() expands to
> > __format__(__printf__,...), not gnu_printf, so it is
> > not quite what we want. (The difference is that on Windows
> > hosts we still want to mark up our our logging functions as
> > taking the glibc style format handling, not whatever the
> > MS C library format escapes happen to be.)
> > At a minimum you'd need to keep in the "on Windows,
> > redefine __printf__ to __gnu_printf__" logic.
> >
> > See also commit 95df51a4a02a853.
>
> Oh, that's a bug in old GLib versions. I thought we had a new enough
> min to avoid that problem, but i guess not after all.

Looks like the implementation changed 2 years ago:
https://gitlab.gnome.org/GNOME/glib/-/commit/98a0ab929d8c59ee27e5f470f11d077bb6a56749
not sure which glib version that would correspond to.

thanks
-- PMM

Re: [PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Daniel P. Berrangé]

On Mon, Sep 28, 2020 at 03:32:45PM +0100, Peter Maydell wrote:
> On Mon, 28 Sep 2020 at 15:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 03:14:45PM +0100, Peter Maydell wrote:
> > > On Mon, 28 Sep 2020 at 15:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > I think this can be simplified even more by using GLib's macros
> > > >
> > > >   #define GCC_FMT_ATTR(n, m)  G_GNUC_PRINTF(n, m)
> > >
> > > At least on my system G_GNUC_PRINTF() expands to
> > > __format__(__printf__,...), not gnu_printf, so it is
> > > not quite what we want. (The difference is that on Windows
> > > hosts we still want to mark up our our logging functions as
> > > taking the glibc style format handling, not whatever the
> > > MS C library format escapes happen to be.)
> > > At a minimum you'd need to keep in the "on Windows,
> > > redefine __printf__ to __gnu_printf__" logic.
> > >
> > > See also commit 95df51a4a02a853.
> >
> > Oh, that's a bug in old GLib versions. I thought we had a new enough
> > min to avoid that problem, but i guess not after all.
> 
> Looks like the implementation changed 2 years ago:
> https://gitlab.gnome.org/GNOME/glib/-/commit/98a0ab929d8c59ee27e5f470f11d077bb6a56749
> not sure which glib version that would correspond to.

Looks like 2.58.0, which is still a fair bit newer than our 2.48 min.

NB, only the macro changed - they were using GNU printf impl for many
many years before that but simply had the wrong macro definition

We can just sacrifice -Wformat checking for Windows builds when using
old GLib. People building natively on Windows with MSys probably have
brand new GLib, and those using Fedora mingw / Debian MXE also have
pretty new GLib.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/3] qemu/compiler: Simplify as all compilers support attribute 'gnu_printf'

[Paolo Bonzini]

On 28/09/20 16:39, Daniel P. Berrangé wrote:
> Looks like 2.58.0, which is still a fair bit newer than our 2.48 min.
> 
> NB, only the macro changed - they were using GNU printf impl for many
> many years before that but simply had the wrong macro definition
> 
> We can just sacrifice -Wformat checking for Windows builds when using
> old GLib. People building natively on Windows with MSys probably have
> brand new GLib, and those using Fedora mingw / Debian MXE also have
> pretty new GLib.

In the past we also had different minimal GLib versions for Windows and
POSIX systems.

Paolo

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

[Philippe Mathieu-Daudé]

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").

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/atomic.h | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index c1d211a3519..8f4b3a80fbd 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -241,23 +241,6 @@
 
 #else /* __ATOMIC_RELAXED */
 
-/*
- * 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
-
-
 #ifdef __alpha__
 #define smp_read_barrier_depends()   asm volatile("mb":::"memory")
 #endif
-- 
2.26.2

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

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]

Hi

On Mon, Sep 28, 2020 at 5:49 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> 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?)
>

With clang 3.8 (xenial amd64) __ATOMIC_RELAXED is defined, so the chunk to
remove which is x86-specific anyway, isn't reached.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2867 bytes --]

[PATCH 3/3] accel/tcg: Remove special case for GCC < 4.6

[Philippe Mathieu-Daudé]

Since commit efc6c070aca ("configure: Add a test for the
minimum compiler version") the minimum compiler version
required for GCC is 4.8.

We can safely remove the special case for GCC 4.6 introduced
in commit 0448f5f8b81 ("cpu-exec: Fix compiler warning
(-Werror=clobbered)").
No change for Clang as we don't know.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 accel/tcg/cpu-exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index e10b46283cc..bf149bb824e 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -703,7 +703,7 @@ int cpu_exec(CPUState *cpu)
 
     /* prepare setjmp context for exception handling */
     if (sigsetjmp(cpu->jmp_env, 0) != 0) {
-#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
+#if defined(__clang__)
         /* Some compilers wrongly smash all local variables after
          * siglongjmp. There were bug reports for gcc 4.5.0 and clang.
          * Reload essential local variables here for those compilers.
-- 
2.26.2

Re: [PATCH 3/3] accel/tcg: Remove special case for GCC < 4.6

[Peter Maydell]

On Mon, 28 Sep 2020 at 14:40, 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.
>
> We can safely remove the special case for GCC 4.6 introduced
> in commit 0448f5f8b81 ("cpu-exec: Fix compiler warning
> (-Werror=clobbered)").
> No change for Clang as we don't know.

Looking through the git history the inclusion of "clang" in
this workaround is because a bug was noticed with the FreeBSD
clang which prompted commit 6c78f29a2424622bfc9c3 in 2013.
It's quite tempting to assume newer clangs have fixed this,
but still I guess it's safest to leave the workaround in place...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM