diff mbox series

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

Message ID 20200928125859.734287-2-philmd@redhat.com
State New
Headers show
Series qemu/compiler: Remove unused special case code for GCC < 4.8 | expand

Commit Message

Philippe Mathieu-Daudé Sept. 28, 2020, 12:58 p.m. UTC
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(-)

Comments

Peter Maydell Sept. 28, 2020, 1:43 p.m. UTC | #1
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
Daniel P. Berrangé Sept. 28, 2020, 2:04 p.m. UTC | #2
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
Peter Maydell Sept. 28, 2020, 2:14 p.m. UTC | #3
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
Daniel P. Berrangé Sept. 28, 2020, 2:23 p.m. UTC | #4
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
Peter Maydell Sept. 28, 2020, 2:32 p.m. UTC | #5
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
Daniel P. Berrangé Sept. 28, 2020, 2:39 p.m. UTC | #6
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
Paolo Bonzini Sept. 28, 2020, 4:49 p.m. UTC | #7
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
diff mbox series

Patch

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)