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