[Qemu-devel] Fix enablement of some compiler warning flags & add some more

[Qemu-devel] Fix enablement of some compiler warning flags & add some more

[Daniel P. Berrange]

I discovered that -Wformat-security was never enabled in QEMU
builds, despite being listed in configure. This is because
the code for checking support of compile flags was wrong. While
fixing this, I decided to see how many more GCC compiler warning
flags could usefully be enabled. The result is this patch series
which fixes several code bugs.

The last patch is a few I could not enable due to the huge number
of current violations. It would be desirable to enable at least
some of these, -Wjump-misses-init in paticular.

[Qemu-devel] [PATCH 1/9] Move all compiler warning/optimization flags to the same place

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

The list of warning/optimization flags set in QEMU_CFLAGS is
in two places in configure. Only one of the places checks
for GCC support. Merge the two separate lists into one and
ensure they are all tested. Set one flag per line to make
it easier to read the list of flags as increasing numbers
are enabled

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/configure b/configure
index 4b3adc9..cd40d17 100755
--- a/configure
+++ b/configure
@@ -252,9 +252,6 @@ pkg_config=query_pkg_config
 sdl_config="${SDL_CONFIG-${cross_prefix}sdl-config}"
 
 # default flags for all hosts
-QEMU_CFLAGS="-fno-strict-aliasing $QEMU_CFLAGS"
-QEMU_CFLAGS="-Wall -Wundef -Wwrite-strings -Wmissing-prototypes $QEMU_CFLAGS"
-QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
 QEMU_CFLAGS="-D_FORTIFY_SOURCE=2 $QEMU_CFLAGS"
 QEMU_INCLUDES="-I. -I\$(SRC_PATH) -I\$(SRC_PATH)/fpu"
@@ -1144,10 +1141,30 @@ else
     exit 1
 fi
 
-gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
-gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
-gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
-gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
+gcc_flags=
+
+# Optimization flags
+gcc_flags="$gcc_flags -fstack-protector-all"
+gcc_flags="$gcc_flags -fno-strict-aliasing"
+
+# Warning flags
+gcc_flags="$gcc_flags -Wall"
+gcc_flags="$gcc_flags -Wundef"
+gcc_flags="$gcc_flags -Wwrite-strings"
+gcc_flags="$gcc_flags -Wmissing-prototypes"
+gcc_flags="$gcc_flags -Wstrict-prototypes"
+gcc_flags="$gcc_flags -Wredundant-decls"
+gcc_flags="$gcc_flags -Wold-style-declaration"
+gcc_flags="$gcc_flags -Wold-style-definition"
+gcc_flags="$gcc_flags -Wtype-limits"
+gcc_flags="$gcc_flags -Wformat-security"
+gcc_flags="$gcc_flags -Wformat-y2k"
+gcc_flags="$gcc_flags -Winit-self"
+gcc_flags="$gcc_flags -Wignored-qualifiers"
+gcc_flags="$gcc_flags -Wmissing-include-dirs"
+gcc_flags="$gcc_flags -Wempty-body"
+gcc_flags="$gcc_flags -Wnested-externs"
+gcc_flags="$gcc_flags -Wendif-labels"
 cat > $TMPC << EOF
 int main(void) { return 0; }
 EOF
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/9] Fix checking for compiler flag support

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

Some warning flags have dependancies, eg -Wformat-security cannot
be enabled if -Wformat is not already enabled. The compiler
flag checking code was checking each flag in isolation so several
were not getting enabled. The fix is to supply all previously
confirmed flags when checking a flag

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index cd40d17..64ab4dc 100755
--- a/configure
+++ b/configure
@@ -1168,11 +1168,13 @@ gcc_flags="$gcc_flags -Wendif-labels"
 cat > $TMPC << EOF
 int main(void) { return 0; }
 EOF
+warning_flags=
 for flag in $gcc_flags; do
-    if compile_prog "-Werror $flag" "" ; then
-	QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+    if compile_prog "-Werror $warning_flags $flag" "" ; then
+	warning_flags="$warning_flags $flag"
     fi
 done
+QEMU_CFLAGS="$QEMU_CFLAGS $warning_flags"
 
 if test "$static" = "yes" ; then
   if test "$pie" = "yes" ; then
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/9] Print out progress when checking compiler flags

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

Normal practice for autoconf style scripts is to print out
progress. The QEMU configure script is getting increasingly
slow & has no progress feedback. Print out the progress of
checking each compiler flag

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 64ab4dc..44b28c8 100755
--- a/configure
+++ b/configure
@@ -1170,8 +1170,12 @@ int main(void) { return 0; }
 EOF
 warning_flags=
 for flag in $gcc_flags; do
+    echo -n "checking if $cc supports $flag... "
     if compile_prog "-Werror $warning_flags $flag" "" ; then
 	warning_flags="$warning_flags $flag"
+	echo "yes"
+    else
+	echo "no"
     fi
 done
 QEMU_CFLAGS="$QEMU_CFLAGS $warning_flags"
-- 
1.7.7.6

[Qemu-devel] [PATCH 4/9] Remove 4 MB stack frame usage from sheepdog

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

The sheepdog driver declares an instance of BDRVSheepdogState
in the stack. This struct is 4 MB in size. While the default
Linux stack size may be 10 MB, we should not assume that since
QEMU needs to be portable to other OS.

block/sheepdog.c: In function ‘sd_create’:
block/sheepdog.c:1240:1: error: the frame size of 4199888 bytes is larger than 131072 bytes [-Werror=frame-larger-than=]

* block/sheepdog.c: Allow BDRVSheepdogState on the heap
  instead of stack
* configure: Add -Wframe-larger-than to validate stack
  size does not exceed 128k

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/sheepdog.c |   43 ++++++++++++++++++++++++++-----------------
 configure        |    1 +
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 00276f6f..ff6f3d2 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1163,20 +1163,22 @@ static int sd_create(const char *filename, QEMUOptionParameter *options)
     uint32_t vid = 0, base_vid = 0;
     int64_t vdi_size = 0;
     char *backing_file = NULL;
-    BDRVSheepdogState s;
+    BDRVSheepdogState *s1;
     char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snapid;
     int prealloc = 0;
     const char *vdiname;
+    int rv = -EINVAL;
+
+    s1 = g_new0(BDRVSheepdogState, 1);
 
     strstart(filename, "sheepdog:", &vdiname);
 
-    memset(&s, 0, sizeof(s));
     memset(vdi, 0, sizeof(vdi));
     memset(tag, 0, sizeof(tag));
-    if (parse_vdiname(&s, vdiname, vdi, &snapid, tag) < 0) {
+    if (parse_vdiname(s1, vdiname, vdi, &snapid, tag) < 0) {
         error_report("invalid filename");
-        return -EINVAL;
+        goto cleanup;
     }
 
     while (options && options->name) {
@@ -1192,7 +1194,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options)
             } else {
                 error_report("Invalid preallocation mode: '%s'",
                              options->value.s);
-                return -EINVAL;
+                goto cleanup;
             }
         }
         options++;
@@ -1200,43 +1202,50 @@ static int sd_create(const char *filename, QEMUOptionParameter *options)
 
     if (vdi_size > SD_MAX_VDI_SIZE) {
         error_report("too big image size");
-        return -EINVAL;
+        goto cleanup;
     }
 
     if (backing_file) {
         BlockDriverState *bs;
-        BDRVSheepdogState *s;
+        BDRVSheepdogState *s2;
         BlockDriver *drv;
 
         /* Currently, only Sheepdog backing image is supported. */
         drv = bdrv_find_protocol(backing_file);
         if (!drv || strcmp(drv->protocol_name, "sheepdog") != 0) {
             error_report("backing_file must be a sheepdog image");
-            return -EINVAL;
+            goto cleanup;
         }
 
         ret = bdrv_file_open(&bs, backing_file, 0);
-        if (ret < 0)
-            return -EIO;
+        if (ret < 0) {
+            rv = -EIO;
+            goto cleanup;
+        }
 
-        s = bs->opaque;
+        s2 = bs->opaque;
 
-        if (!is_snapshot(&s->inode)) {
+        if (!is_snapshot(&s2->inode)) {
             error_report("cannot clone from a non snapshot vdi");
             bdrv_delete(bs);
-            return -EINVAL;
+            goto cleanup;
         }
 
-        base_vid = s->inode.vdi_id;
+        base_vid = s2->inode.vdi_id;
         bdrv_delete(bs);
     }
 
-    ret = do_sd_create(vdi, vdi_size, base_vid, &vid, 0, s.addr, s.port);
+    ret = do_sd_create(vdi, vdi_size, base_vid, &vid, 0, s1->addr, s1->port);
     if (!prealloc || ret) {
-        return ret;
+        rv = ret;
+        goto cleanup;
     }
 
-    return sd_prealloc(filename);
+    rv = sd_prealloc(filename);
+
+ cleanup:
+    g_free(s1);
+    return rv;
 }
 
 static void sd_close(BlockDriverState *bs)
diff --git a/configure b/configure
index 44b28c8..28b5dd5 100755
--- a/configure
+++ b/configure
@@ -1165,6 +1165,7 @@ gcc_flags="$gcc_flags -Wmissing-include-dirs"
 gcc_flags="$gcc_flags -Wempty-body"
 gcc_flags="$gcc_flags -Wnested-externs"
 gcc_flags="$gcc_flags -Wendif-labels"
+gcc_flags="$gcc_flags -Wframe-larger-than=131072"
 cat > $TMPC << EOF
 int main(void) { return 0; }
 EOF
-- 
1.7.7.6

[Qemu-devel] [PATCH 5/9] Add in a large number of extra GCC warnings

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

Add in a large number of extra GCC warnings which don't have any
current code violations

* configure. Add in warning flags: -Wunused, -Wunknown-pragmas,
-Wstrict-aliasing, -Wcast-align, -Wredundant-decls, -Winvalid-pch,
-Wvolatile-register-var, -Wdisabled-optimization,
-Wbuiltin-macro-redefined, -Wmudflap, -Wpacked-bitfield-compat,
-Wsync-nand, -Wattributes, -Wcoverage-mismatch, -Wmultichar,
-Wcpp, -Wdeprecated-declarations, -Wdiv-by-zero, -Wnormalized=nfc,
-Woverflow, -Wpointer-to-int-cast, -Wpragmas, -Wtrampolines

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 28b5dd5..524458c 100755
--- a/configure
+++ b/configure
@@ -1166,6 +1166,33 @@ gcc_flags="$gcc_flags -Wempty-body"
 gcc_flags="$gcc_flags -Wnested-externs"
 gcc_flags="$gcc_flags -Wendif-labels"
 gcc_flags="$gcc_flags -Wframe-larger-than=131072"
+gcc_flags="$gcc_flags -Wunused"
+gcc_flags="$gcc_flags -Wunknown-pragmas"
+gcc_flags="$gcc_flags -Wstrict-aliasing"
+gcc_flags="$gcc_flags -Wcast-align"
+gcc_flags="$gcc_flags -Wredundant-decls"
+gcc_flags="$gcc_flags -Winvalid-pch"
+gcc_flags="$gcc_flags -Wvolatile-register-var"
+gcc_flags="$gcc_flags -Wdisabled-optimization"
+gcc_flags="$gcc_flags -Wbuiltin-macro-redefined"
+gcc_flags="$gcc_flags -Wmudflap"
+gcc_flags="$gcc_flags -Wpacked-bitfield-compat"
+gcc_flags="$gcc_flags -Wsync-nand"
+gcc_flags="$gcc_flags -Wattributes"
+gcc_flags="$gcc_flags -Wcoverage-mismatch"
+gcc_flags="$gcc_flags -Wmultichar"
+gcc_flags="$gcc_flags -Wcpp"
+gcc_flags="$gcc_flags -Wdeprecated-declarations"
+gcc_flags="$gcc_flags -Wdiv-by-zero"
+gcc_flags="$gcc_flags -Wmultichar"
+gcc_flags="$gcc_flags -Wnormalized=nfc"
+gcc_flags="$gcc_flags -Woverflow"
+gcc_flags="$gcc_flags -Wpointer-to-int-cast"
+gcc_flags="$gcc_flags -Wpragmas"
+gcc_flags="$gcc_flags -Wtrampolines"
+gcc_flags="$gcc_flags -Wmissing-parameter-type"
+gcc_flags="$gcc_flags -Wuninitialized"
+
 cat > $TMPC << EOF
 int main(void) { return 0; }
 EOF
-- 
1.7.7.6

[Qemu-devel] [PATCH 6/9] Fix bit test to use & instead of && and enable -Wlogical-op warning

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

* configure: Enable -Wlogical-op
* hw/exynos4210_uart.c: s/&&/&/

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure            |    1 +
 hw/exynos4210_uart.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 524458c..8ee6cdb 100755
--- a/configure
+++ b/configure
@@ -1192,6 +1192,7 @@ gcc_flags="$gcc_flags -Wpragmas"
 gcc_flags="$gcc_flags -Wtrampolines"
 gcc_flags="$gcc_flags -Wmissing-parameter-type"
 gcc_flags="$gcc_flags -Wuninitialized"
+gcc_flags="$gcc_flags -Wlogical-op"
 
 cat > $TMPC << EOF
 int main(void) { return 0; }
diff --git a/hw/exynos4210_uart.c b/hw/exynos4210_uart.c
index 73a9c18..4b20105 100644
--- a/hw/exynos4210_uart.c
+++ b/hw/exynos4210_uart.c
@@ -246,7 +246,7 @@ static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
     uint32_t level = 0;
     uint32_t reg;
 
-    reg = (s->reg[I_(UFCON)] && UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
+    reg = (s->reg[I_(UFCON)] & UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
             UFCON_Tx_FIFO_TRIGGER_LEVEL_SHIFT;
 
     switch (s->channel) {
@@ -277,7 +277,7 @@ static void exynos4210_uart_update_irq(Exynos4210UartState *s)
      */
     if (s->reg[I_(UFCON)] && UFCON_FIFO_ENABLE) {
 
-        uint32_t count = (s->reg[I_(UFSTAT)] && UFSTAT_Tx_FIFO_COUNT) >>
+        uint32_t count = (s->reg[I_(UFSTAT)] & UFSTAT_Tx_FIFO_COUNT) >>
                 UFSTAT_Tx_FIFO_COUNT_SHIFT;
 
         if (count <= exynos4210_uart_Tx_FIFO_trigger_level(s)) {
-- 
1.7.7.6

[Qemu-devel] [PATCH 7/9] Add -Wmissing-format-attribute & fix problems it finds

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

* configure: Add -Wmissing-format-attribute
* hw/qxl.c: Add missing format attribute to qxl_guest_bug
  and fix format specifiers in a caller of it
* qtest.c: Add missing format attribute to qtest_send

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure |    1 +
 hw/qxl.c  |    4 ++--
 qtest.c   |    2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 8ee6cdb..3d47440 100755
--- a/configure
+++ b/configure
@@ -1193,6 +1193,7 @@ gcc_flags="$gcc_flags -Wtrampolines"
 gcc_flags="$gcc_flags -Wmissing-parameter-type"
 gcc_flags="$gcc_flags -Wuninitialized"
 gcc_flags="$gcc_flags -Wlogical-op"
+gcc_flags="$gcc_flags -Wmissing-format-attribute"
 
 cat > $TMPC << EOF
 int main(void) { return 0; }
diff --git a/hw/qxl.c b/hw/qxl.c
index 47a162e..33b2288 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -124,7 +124,7 @@ static void qxl_reset_memslots(PCIQXLDevice *d);
 static void qxl_reset_surfaces(PCIQXLDevice *d);
 static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
 
-void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
+GCC_FMT_ATTR(2, 3) void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
 {
     qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
     if (qxl->guestdebug) {
@@ -1370,7 +1370,7 @@ async_common:
     case QXL_IO_DESTROY_SURFACE_WAIT:
         if (val >= NUM_SURFACES) {
             qxl_guest_bug(d, "QXL_IO_DESTROY_SURFACE (async=%d):"
-                             "%d >= NUM_SURFACES", async, val);
+                             "%"PRIx64" >= NUM_SURFACES", async, val);
             goto cancel_async;
         }
         qxl_spice_destroy_surface_wait(d, val, async);
diff --git a/qtest.c b/qtest.c
index cd7186c..2b71de3 100644
--- a/qtest.c
+++ b/qtest.c
@@ -156,7 +156,7 @@ static void qtest_send_prefix(CharDriverState *chr)
             tv.tv_sec, tv.tv_usec);
 }
 
-static void qtest_send(CharDriverState *chr, const char *fmt, ...)
+GCC_FMT_ATTR(2, 3) static void qtest_send(CharDriverState *chr, const char *fmt, ...)
 {
     va_list ap;
     char buffer[1024];
-- 
1.7.7.6

[Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

Add -Wformat-contains-nul, -Wformat-extra-args,
-Wformat-zero-length and -Wformat-nonliteral to the compiler
flags & fix the issues they identify

It is desirable to have these warnings enabled, even though
it is not practical to fix all violations. Introduce some
macros GCC_WARNINGS_SAVE, GCC_WARNINGS_RESTORE &
GCC_WARNINGS_IGNORE, which allow specific warnings to be
ignored for small blocks of code

* cmd.c, block/vmdk.c: Pass format strings directly to
  snprintf instead of storing then in intermediate
  'const char*' variables, so that GCC can validate
  args fully
* configure: Add -Wformat-contains-nul, -Wformat-extra-args,
  -Wformat-zero-length, -Wformat-nonliteral
* compiler.h: Add macros for turning off warnings selectively
* bsd-user/strace.c, linux-user/strace.c: Disable warnings
  about non-literal format args
* ui/vnc.c, ui/vnc.h, ui/vnc-auth-sasl.c: Refactor
  vnc_socket_local_addr & vnc_socket_remote_addr to
  just take a separator instead of format string

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/vmdk.c        |   64 ++++++++++++++++++++++++---------------------------
 bsd-user/strace.c   |    4 +++
 cmd.c               |   63 +++++++++++++++++++++++++++-----------------------
 compiler.h          |   11 ++++++++
 configure           |    4 +++
 linux-user/strace.c |    6 ++++
 ui/vnc-auth-sasl.c  |    7 ++++-
 ui/vnc.c            |   20 ++++++++-------
 ui/vnc.h            |    4 +-
 9 files changed, 107 insertions(+), 76 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 45c003a..12cad53 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1356,28 +1356,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     char ext_desc_lines[BUF_SIZE] = "";
     char path[PATH_MAX], prefix[PATH_MAX], postfix[PATH_MAX];
     const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
-    const char *desc_extent_line;
     char parent_desc_line[BUF_SIZE] = "";
     uint32_t parent_cid = 0xffffffff;
-    const char desc_template[] =
-        "# Disk DescriptorFile\n"
-        "version=1\n"
-        "CID=%x\n"
-        "parentCID=%x\n"
-        "createType=\"%s\"\n"
-        "%s"
-        "\n"
-        "# Extent description\n"
-        "%s"
-        "\n"
-        "# The Disk Data Base\n"
-        "#DDB\n"
-        "\n"
-        "ddb.virtualHWVersion = \"%d\"\n"
-        "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
-        "ddb.geometry.heads = \"16\"\n"
-        "ddb.geometry.sectors = \"63\"\n"
-        "ddb.adapterType = \"ide\"\n";
 
     if (filename_decompose(filename, path, prefix, postfix, PATH_MAX)) {
         return -EINVAL;
@@ -1411,11 +1391,6 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     flat = !(strcmp(fmt, "monolithicFlat") &&
              strcmp(fmt, "twoGbMaxExtentFlat"));
     compress = !strcmp(fmt, "streamOptimized");
-    if (flat) {
-        desc_extent_line = "RW %lld FLAT \"%s\" 0\n";
-    } else {
-        desc_extent_line = "RW %lld SPARSE \"%s\"\n";
-    }
     if (flat && backing_file) {
         /* not supporting backing file for flat image */
         return -ENOTSUP;
@@ -1471,18 +1446,39 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
 
         /* Format description line */
         snprintf(desc_line, sizeof(desc_line),
-                    desc_extent_line, size / 512, desc_filename);
+                 (flat
+                  ? "RW %" PRId64 " FLAT \"%s\" 0\n"
+                  : "RW %" PRId64 " SPARSE \"%s\"\n"),
+                 size / 512, desc_filename);
         pstrcat(ext_desc_lines, sizeof(ext_desc_lines), desc_line);
     }
     /* generate descriptor file */
-    snprintf(desc, sizeof(desc), desc_template,
-            (unsigned int)time(NULL),
-            parent_cid,
-            fmt,
-            parent_desc_line,
-            ext_desc_lines,
-            (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
-            total_size / (int64_t)(63 * 16 * 512));
+    snprintf(desc, sizeof(desc),
+             "# Disk DescriptorFile\n"
+             "version=1\n"
+             "CID=%x\n"
+             "parentCID=%x\n"
+             "createType=\"%s\"\n"
+             "%s"
+             "\n"
+             "# Extent description\n"
+             "%s"
+             "\n"
+             "# The Disk Data Base\n"
+             "#DDB\n"
+             "\n"
+             "ddb.virtualHWVersion = \"%d\"\n"
+             "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
+             "ddb.geometry.heads = \"16\"\n"
+             "ddb.geometry.sectors = \"63\"\n"
+             "ddb.adapterType = \"ide\"\n",
+             (unsigned int)time(NULL),
+             parent_cid,
+             fmt,
+             parent_desc_line,
+             ext_desc_lines,
+             (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
+             total_size / (int64_t)(63 * 16 * 512));
     if (split || flat) {
         fd = open(
                 filename,
diff --git a/bsd-user/strace.c b/bsd-user/strace.c
index d73bbca..73ae567 100644
--- a/bsd-user/strace.c
+++ b/bsd-user/strace.c
@@ -90,6 +90,9 @@ static const struct syscallname openbsd_scnames[] = {
 #include "openbsd/strace.list"
 };
 
+
+GCC_WARNINGS_SAVE
+GCC_WARNINGS_IGNORE("-Wformat-nonliteral")
 static void
 print_syscall(int num, const struct syscallname *scnames, unsigned int nscnames,
               abi_long arg1, abi_long arg2, abi_long arg3,
@@ -119,6 +122,7 @@ print_syscall(int num, const struct syscallname *scnames, unsigned int nscnames,
         }
     gemu_log("Unknown syscall %d\n", num);
 }
+GCC_WARNINGS_RESTORE
 
 static void
 print_syscall_ret(int num, abi_long ret, const struct syscallname *scnames,
diff --git a/cmd.c b/cmd.c
index 0806e18..0490d23 100644
--- a/cmd.c
+++ b/cmd.c
@@ -414,36 +414,41 @@ cvtnum(
 
 void
 cvtstr(
-	double		value,
-	char		*str,
-	size_t		size)
+        double                value,
+        char                *str,
+        size_t                size)
 {
-	const char	*fmt;
-	int		precise;
-
-	precise = ((double)value * 1000 == (double)(int)value * 1000);
-
-	if (value >= EXABYTES(1)) {
-		fmt = precise ? "%.f EiB" : "%.3f EiB";
-		snprintf(str, size, fmt, TO_EXABYTES(value));
-	} else if (value >= PETABYTES(1)) {
-		fmt = precise ? "%.f PiB" : "%.3f PiB";
-		snprintf(str, size, fmt, TO_PETABYTES(value));
-	} else if (value >= TERABYTES(1)) {
-		fmt = precise ? "%.f TiB" : "%.3f TiB";
-		snprintf(str, size, fmt, TO_TERABYTES(value));
-	} else if (value >= GIGABYTES(1)) {
-		fmt = precise ? "%.f GiB" : "%.3f GiB";
-		snprintf(str, size, fmt, TO_GIGABYTES(value));
-	} else if (value >= MEGABYTES(1)) {
-		fmt = precise ? "%.f MiB" : "%.3f MiB";
-		snprintf(str, size, fmt, TO_MEGABYTES(value));
-	} else if (value >= KILOBYTES(1)) {
-		fmt = precise ? "%.f KiB" : "%.3f KiB";
-		snprintf(str, size, fmt, TO_KILOBYTES(value));
-	} else {
-		snprintf(str, size, "%f bytes", value);
-	}
+        int                precise;
+
+        precise = ((double)value * 1000 == (double)(int)value * 1000);
+
+        if (value >= EXABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f EiB" : "%.3f EiB",
+                         TO_EXABYTES(value));
+        } else if (value >= PETABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f PiB" : "%.3f PiB",
+                         TO_PETABYTES(value));
+        } else if (value >= TERABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f TiB" : "%.3f TiB",
+                         TO_TERABYTES(value));
+        } else if (value >= GIGABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f GiB" : "%.3f GiB",
+                         TO_GIGABYTES(value));
+        } else if (value >= MEGABYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f MiB" : "%.3f MiB",
+                         TO_MEGABYTES(value));
+        } else if (value >= KILOBYTES(1)) {
+                snprintf(str, size,
+                         precise ? "%.f KiB" : "%.3f KiB",
+                         TO_KILOBYTES(value));
+        } else {
+                snprintf(str, size, "%f bytes", value);
+        }
 }
 
 struct timeval
diff --git a/compiler.h b/compiler.h
index 736e770..145d848 100644
--- a/compiler.h
+++ b/compiler.h
@@ -50,4 +50,15 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+#if defined __GNUC__
+# define GCC_WARNINGS_SAVE      _Pragma("GCC diagnostic push")
+# define GCC_WARNINGS_RESTORE   _Pragma("GCC diagnostic pop")
+# define DO_PRAGMA(x)           _Pragma(#x)
+# define GCC_WARNINGS_IGNORE(x) DO_PRAGMA(GCC diagnostic ignored x)
+#else
+# define GCC_WARNINGS_SAVE
+# define GCC_WARNINGS_RESTORE
+# define GCC_WARNINGS_IGNORE(x)
+#endif
+
 #endif /* COMPILER_H */
diff --git a/configure b/configure
index 3d47440..175901f 100755
--- a/configure
+++ b/configure
@@ -1194,6 +1194,10 @@ gcc_flags="$gcc_flags -Wmissing-parameter-type"
 gcc_flags="$gcc_flags -Wuninitialized"
 gcc_flags="$gcc_flags -Wlogical-op"
 gcc_flags="$gcc_flags -Wmissing-format-attribute"
+gcc_flags="$gcc_flags -Wformat-contains-nul"
+gcc_flags="$gcc_flags -Wformat-extra-args"
+gcc_flags="$gcc_flags -Wformat-zero-length"
+gcc_flags="$gcc_flags -Wformat-nonliteral"
 
 cat > $TMPC << EOF
 int main(void) { return 0; }
diff --git a/linux-user/strace.c b/linux-user/strace.c
index 05a0d3e..75fd70b 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -627,6 +627,8 @@ print_string(abi_long addr, int last)
  * Prints out raw parameter using given format.  Caller needs
  * to do byte swapping if needed.
  */
+GCC_WARNINGS_SAVE
+GCC_WARNINGS_IGNORE("-Wformat-nonliteral")
 static void
 print_raw_param(const char *fmt, abi_long param, int last)
 {
@@ -635,6 +637,7 @@ print_raw_param(const char *fmt, abi_long param, int last)
     (void) snprintf(format, sizeof (format), "%s%s", fmt, get_comma(last));
     gemu_log(format, param);
 }
+GCC_WARNINGS_RESTORE
 
 static void
 print_pointer(abi_long p, int last)
@@ -1488,6 +1491,8 @@ static int nsyscalls = ARRAY_SIZE(scnames);
 /*
  * The public interface to this module.
  */
+GCC_WARNINGS_SAVE
+GCC_WARNINGS_IGNORE("-Wformat-nonliteral")
 void
 print_syscall(int num,
               abi_long arg1, abi_long arg2, abi_long arg3,
@@ -1513,6 +1518,7 @@ print_syscall(int num,
         }
     gemu_log("Unknown syscall %d\n", num);
 }
+GCC_WARNINGS_RESTORE
 
 
 void
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 8fba770..07c4f5a 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -500,10 +500,13 @@ void start_auth_sasl(VncState *vs)
     VNC_DEBUG("Initialize SASL auth %d\n", vs->csock);
 
     /* Get local & remote client addresses in form  IPADDR;PORT */
-    if (!(localAddr = vnc_socket_local_addr("%s;%s", vs->csock)))
+    localAddr = vnc_socket_local_addr(';', vs->csock);
+    if (!localAddr) {
         goto authabort;
+    }
 
-    if (!(remoteAddr = vnc_socket_remote_addr("%s;%s", vs->csock))) {
+    remoteAddr = vnc_socket_remote_addr(';', vs->csock);
+    if (!remoteAddr) {
         g_free(localAddr);
         goto authabort;
     }
diff --git a/ui/vnc.c b/ui/vnc.c
index deb9ecd..86c5c3b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -71,7 +71,7 @@ static void vnc_set_share_mode(VncState *vs, VncShareMode mode)
     }
 }
 
-static char *addr_to_string(const char *format,
+static char *addr_to_string(char separator,
                             struct sockaddr_storage *sa,
                             socklen_t salen) {
     char *addr;
@@ -89,18 +89,19 @@ static char *addr_to_string(const char *format,
         return NULL;
     }
 
-    /* Enough for the existing format + the 2 vars we're
+    /* Enough for the separator + the 2 vars we're
      * substituting in. */
-    addrlen = strlen(format) + strlen(host) + strlen(serv);
+    addrlen = strlen(host) + 1 + strlen(serv);
     addr = g_malloc(addrlen + 1);
-    snprintf(addr, addrlen, format, host, serv);
+    snprintf(addr, addrlen, "%s%c%s", host, separator, serv);
     addr[addrlen] = '\0';
 
     return addr;
 }
 
 
-char *vnc_socket_local_addr(const char *format, int fd) {
+char *vnc_socket_local_addr(char separator, int fd)
+{
     struct sockaddr_storage sa;
     socklen_t salen;
 
@@ -108,10 +109,11 @@ char *vnc_socket_local_addr(const char *format, int fd) {
     if (getsockname(fd, (struct sockaddr*)&sa, &salen) < 0)
         return NULL;
 
-    return addr_to_string(format, &sa, salen);
+    return addr_to_string(separator, &sa, salen);
 }
 
-char *vnc_socket_remote_addr(const char *format, int fd) {
+char *vnc_socket_remote_addr(char separator, int fd)
+{
     struct sockaddr_storage sa;
     socklen_t salen;
 
@@ -119,7 +121,7 @@ char *vnc_socket_remote_addr(const char *format, int fd) {
     if (getpeername(fd, (struct sockaddr*)&sa, &salen) < 0)
         return NULL;
 
-    return addr_to_string(format, &sa, salen);
+    return addr_to_string(separator, &sa, salen);
 }
 
 static int put_addr_qdict(QDict *qdict, struct sockaddr_storage *sa,
@@ -2857,7 +2859,7 @@ char *vnc_display_local_addr(DisplayState *ds)
 {
     VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
     
-    return vnc_socket_local_addr("%s:%s", vs->lsock);
+    return vnc_socket_local_addr(':', vs->lsock);
 }
 
 int vnc_display_open(DisplayState *ds, const char *display)
diff --git a/ui/vnc.h b/ui/vnc.h
index a851ebd..fc5be66 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -533,8 +533,8 @@ void buffer_append(Buffer *buffer, const void *data, size_t len);
 
 /* Misc helpers */
 
-char *vnc_socket_local_addr(const char *format, int fd);
-char *vnc_socket_remote_addr(const char *format, int fd);
+char *vnc_socket_local_addr(char separator, int fd);
+char *vnc_socket_remote_addr(char separator, int fd);
 
 static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
     return (vs->features & (1 << feature));
-- 
1.7.7.6

[Qemu-devel] [PATCH 9/9] Add note about some other options potentially worth enabling

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

There are a few other GCC warning options likely worth
enabling, but it is not practical with the level of warnings
generated. Add a note about them for anyone motiviated to
address it in the future

* configure: Add -Wclobbered, -Wmissing-field-initializers,
  -Woverride-init, -Wsign-compare, -Wunused-parameter,
  -Wunused-but-set-parameter, -Wpointer-arith, -Wmissing-noreturn,
  -Wjump-misses-init but leave disabled for now

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 175901f..2d62337 100755
--- a/configure
+++ b/configure
@@ -1199,6 +1199,26 @@ gcc_flags="$gcc_flags -Wformat-extra-args"
 gcc_flags="$gcc_flags -Wformat-zero-length"
 gcc_flags="$gcc_flags -Wformat-nonliteral"
 
+# Some other potentially worth enabling once issues are fixed
+# False positives in cpu-exec.c
+#gcc_flags="$gcc_flags -Wclobbered"
+# Many many violations
+#gcc_flags="$gcc_flags -Wmissing-field-initializers"
+# Quite a few (intentional?) overrides
+#gcc_flags="$gcc_flags -Woverride-init"
+# Many many violations
+#gcc_flags="$gcc_flags -Wsign-compare"
+# Many many violations
+#gcc_flags="$gcc_flags -Wunused-parameter"
+# Strange violations in mips helper
+#gcc_flags="$gcc_flags -Wunused-but-set-parameter"
+# Many many violations
+#gcc_flags="$gcc_flags -Wpointer-arith"
+# Many many violations
+#gcc_flags="$gcc_flags -Wmissing-noreturn"
+# Many many violations
+#gcc_flags="$gcc_flags -Wjump-misses-init"
+
 cat > $TMPC << EOF
 int main(void) { return 0; }
 EOF
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Peter Maydell]

On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> +#if defined __GNUC__
> +# define GCC_WARNINGS_SAVE      _Pragma("GCC diagnostic push")
> +# define GCC_WARNINGS_RESTORE   _Pragma("GCC diagnostic pop")
> +# define DO_PRAGMA(x)           _Pragma(#x)
> +# define GCC_WARNINGS_IGNORE(x) DO_PRAGMA(GCC diagnostic ignored x)
> +#else
> +# define GCC_WARNINGS_SAVE
> +# define GCC_WARNINGS_RESTORE
> +# define GCC_WARNINGS_IGNORE(x)
> +#endif

Do these pragmas work on all versions of gcc that we support?
Google suggests that the push/pop ones are only gcc 4.6 or better,
for example.

-- PMM

Re: [Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Daniel P. Berrange]

On Mon, Apr 02, 2012 at 01:13:56PM +0100, Peter Maydell wrote:
> On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> > +#if defined __GNUC__
> > +# define GCC_WARNINGS_SAVE      _Pragma("GCC diagnostic push")
> > +# define GCC_WARNINGS_RESTORE   _Pragma("GCC diagnostic pop")
> > +# define DO_PRAGMA(x)           _Pragma(#x)
> > +# define GCC_WARNINGS_IGNORE(x) DO_PRAGMA(GCC diagnostic ignored x)
> > +#else
> > +# define GCC_WARNINGS_SAVE
> > +# define GCC_WARNINGS_RESTORE
> > +# define GCC_WARNINGS_IGNORE(x)
> > +#endif
> 
> Do these pragmas work on all versions of gcc that we support?
> Google suggests that the push/pop ones are only gcc 4.6 or better,
> for example.

Hmm, the gcc info pages didn't mention any version constraints, but I'll
investigate this


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 6/9] Fix bit test to use & instead of && and enable -Wlogical-op warning

[Peter Maydell]

On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> diff --git a/hw/exynos4210_uart.c b/hw/exynos4210_uart.c
> index 73a9c18..4b20105 100644
> --- a/hw/exynos4210_uart.c
> +++ b/hw/exynos4210_uart.c
> @@ -246,7 +246,7 @@ static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
>     uint32_t level = 0;
>     uint32_t reg;
>
> -    reg = (s->reg[I_(UFCON)] && UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
> +    reg = (s->reg[I_(UFCON)] & UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
>             UFCON_Tx_FIFO_TRIGGER_LEVEL_SHIFT;
>
>     switch (s->channel) {
> @@ -277,7 +277,7 @@ static void exynos4210_uart_update_irq(Exynos4210UartState *s)
>      */
>     if (s->reg[I_(UFCON)] && UFCON_FIFO_ENABLE) {
>
> -        uint32_t count = (s->reg[I_(UFSTAT)] && UFSTAT_Tx_FIFO_COUNT) >>
> +        uint32_t count = (s->reg[I_(UFSTAT)] & UFSTAT_Tx_FIFO_COUNT) >>
>                 UFSTAT_Tx_FIFO_COUNT_SHIFT;
>
>         if (count <= exynos4210_uart_Tx_FIFO_trigger_level(s)) {

Nice catch. Note that the '&& UFCON_FIFO_ENABLE' you can see in the context
to the second hunk is also wrong and needs fixing.

I'll take the exynos changes via arm-devs.next, but not the configure
change. Please can you submit a version of the patch that only fixes
the bugs and doesn't also change the gcc warning flags?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/9] Fix checking for compiler flag support

[Peter Maydell]

On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> diff --git a/configure b/configure
> index cd40d17..64ab4dc 100755
> --- a/configure
> +++ b/configure
> @@ -1168,11 +1168,13 @@ gcc_flags="$gcc_flags -Wendif-labels"
>  cat > $TMPC << EOF
>  int main(void) { return 0; }
>  EOF
> +warning_flags=
>  for flag in $gcc_flags; do
> -    if compile_prog "-Werror $flag" "" ; then
> -       QEMU_CFLAGS="$QEMU_CFLAGS $flag"
> +    if compile_prog "-Werror $warning_flags $flag" "" ; then
> +       warning_flags="$warning_flags $flag"
>     fi
>  done
> +QEMU_CFLAGS="$QEMU_CFLAGS $warning_flags"

The compile_prog function honours QEMU_CFLAGS, so
adding each flag to QEMU_CFLAGS as we confirm it to work
should have the same effect, surely?

-- PMM

Re: [Qemu-devel] [PATCH 7/9] Add -Wmissing-format-attribute & fix problems it finds

[Andreas Färber]

Am 02.04.2012 12:50, schrieb Daniel P. Berrange:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> * configure: Add -Wmissing-format-attribute
> * hw/qxl.c: Add missing format attribute to qxl_guest_bug
>   and fix format specifiers in a caller of it
> * qtest.c: Add missing format attribute to qtest_send

There were patches for both of these on the list already:

http://patchwork.ozlabs.org/patch/149983/
http://patchwork.ozlabs.org/patch/149835/

Andreas

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  configure |    1 +
>  hw/qxl.c  |    4 ++--
>  qtest.c   |    2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 8ee6cdb..3d47440 100755
> --- a/configure
> +++ b/configure
> @@ -1193,6 +1193,7 @@ gcc_flags="$gcc_flags -Wtrampolines"
>  gcc_flags="$gcc_flags -Wmissing-parameter-type"
>  gcc_flags="$gcc_flags -Wuninitialized"
>  gcc_flags="$gcc_flags -Wlogical-op"
> +gcc_flags="$gcc_flags -Wmissing-format-attribute"
>  
>  cat > $TMPC << EOF
>  int main(void) { return 0; }
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 47a162e..33b2288 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -124,7 +124,7 @@ static void qxl_reset_memslots(PCIQXLDevice *d);
>  static void qxl_reset_surfaces(PCIQXLDevice *d);
>  static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
>  
> -void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
> +GCC_FMT_ATTR(2, 3) void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
>  {
>      qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
>      if (qxl->guestdebug) {
> @@ -1370,7 +1370,7 @@ async_common:
>      case QXL_IO_DESTROY_SURFACE_WAIT:
>          if (val >= NUM_SURFACES) {
>              qxl_guest_bug(d, "QXL_IO_DESTROY_SURFACE (async=%d):"
> -                             "%d >= NUM_SURFACES", async, val);
> +                             "%"PRIx64" >= NUM_SURFACES", async, val);
>              goto cancel_async;
>          }
>          qxl_spice_destroy_surface_wait(d, val, async);
> diff --git a/qtest.c b/qtest.c
> index cd7186c..2b71de3 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -156,7 +156,7 @@ static void qtest_send_prefix(CharDriverState *chr)
>              tv.tv_sec, tv.tv_usec);
>  }
>  
> -static void qtest_send(CharDriverState *chr, const char *fmt, ...)
> +GCC_FMT_ATTR(2, 3) static void qtest_send(CharDriverState *chr, const char *fmt, ...)
>  {
>      va_list ap;
>      char buffer[1024];

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/9] Print out progress when checking compiler flags

[Peter Maydell]

On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
>
> Normal practice for autoconf style scripts is to print out
> progress. The QEMU configure script is getting increasingly
> slow & has no progress feedback. Print out the progress of
> checking each compiler flag
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  configure |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/configure b/configure
> index 64ab4dc..44b28c8 100755
> --- a/configure
> +++ b/configure
> @@ -1170,8 +1170,12 @@ int main(void) { return 0; }
>  EOF
>  warning_flags=
>  for flag in $gcc_flags; do
> +    echo -n "checking if $cc supports $flag... "
>     if compile_prog "-Werror $warning_flags $flag" "" ; then
>        warning_flags="$warning_flags $flag"
> +       echo "yes"
> +    else
> +       echo "no"
>     fi
>  done
>  QEMU_CFLAGS="$QEMU_CFLAGS $warning_flags"

If we're going to do this we should do it consistently,
ie add messages for all tests, not just this one.
(Bonus points for also adding messages to config.log so you
can see which test compiles correspond to which feature tests.)

(Also, "echo -n" isn't portable; we use printf where we don't
want the trailing newline.)

-- PMM

Re: [Qemu-devel] [PATCH 3/9] Print out progress when checking compiler flags

[Daniel P. Berrange]

On Mon, Apr 02, 2012 at 02:56:17PM +0100, Peter Maydell wrote:
> On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> >
> > Normal practice for autoconf style scripts is to print out
> > progress. The QEMU configure script is getting increasingly
> > slow & has no progress feedback. Print out the progress of
> > checking each compiler flag
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  configure |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 64ab4dc..44b28c8 100755
> > --- a/configure
> > +++ b/configure
> > @@ -1170,8 +1170,12 @@ int main(void) { return 0; }
> >  EOF
> >  warning_flags=
> >  for flag in $gcc_flags; do
> > +    echo -n "checking if $cc supports $flag... "
> >     if compile_prog "-Werror $warning_flags $flag" "" ; then
> >        warning_flags="$warning_flags $flag"
> > +       echo "yes"
> > +    else
> > +       echo "no"
> >     fi
> >  done
> >  QEMU_CFLAGS="$QEMU_CFLAGS $warning_flags"
> 
> If we're going to do this we should do it consistently,
> ie add messages for all tests, not just this one.
> (Bonus points for also adding messages to config.log so you
> can see which test compiles correspond to which feature tests.)

I did wonder about doing that. If people are generally for the idea of
adding progress messages throughout configure, then I'll make some time
to work on that.

Don't want to spend all the effort if such a patch is going to get rejected
though...

Any further opinions for/against ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Peter Maydell]

On 2 April 2012 13:17, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Mon, Apr 02, 2012 at 01:13:56PM +0100, Peter Maydell wrote:
>> On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
>> > +#if defined __GNUC__
>> > +# define GCC_WARNINGS_SAVE      _Pragma("GCC diagnostic push")
>> > +# define GCC_WARNINGS_RESTORE   _Pragma("GCC diagnostic pop")
>> > +# define DO_PRAGMA(x)           _Pragma(#x)
>> > +# define GCC_WARNINGS_IGNORE(x) DO_PRAGMA(GCC diagnostic ignored x)
>> > +#else
>> > +# define GCC_WARNINGS_SAVE
>> > +# define GCC_WARNINGS_RESTORE
>> > +# define GCC_WARNINGS_IGNORE(x)
>> > +#endif
>>
>> Do these pragmas work on all versions of gcc that we support?
>> Google suggests that the push/pop ones are only gcc 4.6 or better,
>> for example.
>
> Hmm, the gcc info pages didn't mention any version constraints, but I'll
> investigate this

Having thought about it a little more, to be honest I'm not really
convinced that we should have these anyway. Better just to not enable
the format-nonliteral warning. (format-security should catch the cases
we care most about anyway, I think.)

-- PMM

Re: [Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Daniel P. Berrange]

On Mon, Apr 02, 2012 at 03:04:54PM +0100, Peter Maydell wrote:
> On 2 April 2012 13:17, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Mon, Apr 02, 2012 at 01:13:56PM +0100, Peter Maydell wrote:
> >> On 2 April 2012 11:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> >> > +#if defined __GNUC__
> >> > +# define GCC_WARNINGS_SAVE      _Pragma("GCC diagnostic push")
> >> > +# define GCC_WARNINGS_RESTORE   _Pragma("GCC diagnostic pop")
> >> > +# define DO_PRAGMA(x)           _Pragma(#x)
> >> > +# define GCC_WARNINGS_IGNORE(x) DO_PRAGMA(GCC diagnostic ignored x)
> >> > +#else
> >> > +# define GCC_WARNINGS_SAVE
> >> > +# define GCC_WARNINGS_RESTORE
> >> > +# define GCC_WARNINGS_IGNORE(x)
> >> > +#endif
> >>
> >> Do these pragmas work on all versions of gcc that we support?
> >> Google suggests that the push/pop ones are only gcc 4.6 or better,
> >> for example.
> >
> > Hmm, the gcc info pages didn't mention any version constraints, but I'll
> > investigate this
> 
> Having thought about it a little more, to be honest I'm not really
> convinced that we should have these anyway. Better just to not enable
> the format-nonliteral warning. (format-security should catch the cases
> we care most about anyway, I think.)

The -Wformat-security option can only catch problems if the format
string is a literal. eg so it'd miss this:

  void foo(void) {
     int notastring = 1;
     const char *format = "String is %s";

     sprintf(format, notastring);
  }

There are a handful of places in QEMU which do that with non-trivial
format strings & were easy to fix in this patch, which I think is a
worthwhile improvement. The cases in the *-user/strace.c file though
are not practical to fix, without significant re-design of the code
in question.

I'm fine if we just include those easy fixes, while leaving the actual
warning flag disabled for now - someone can revisit later if desired.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Peter Maydell]

On 2 April 2012 15:22, Daniel P. Berrange <berrange@redhat.com> wrote:
> The -Wformat-security option can only catch problems if the format
> string is a literal. eg so it'd miss this:
>
>  void foo(void) {
>     int notastring = 1;
>     const char *format = "String is %s";
>
>     sprintf(format, notastring);
>  }
>
> There are a handful of places in QEMU which do that with non-trivial
> format strings & were easy to fix in this patch, which I think is a
> worthwhile improvement. The cases in the *-user/strace.c file though
> are not practical to fix, without significant re-design of the code
> in question.

To be honest I couldn't tell from your patch whether you'd actually
fixed any bugs or if you were just moving things around to turn non
literals into literals.

(Some of the cleanup looks like a good idea anyway, eg the vnc bits.)

-- PMM

Re: [Qemu-devel] [PATCH 8/9] Add more format string warning flags

[Daniel P. Berrange]

On Mon, Apr 02, 2012 at 03:32:51PM +0100, Peter Maydell wrote:
> On 2 April 2012 15:22, Daniel P. Berrange <berrange@redhat.com> wrote:
> > The -Wformat-security option can only catch problems if the format
> > string is a literal. eg so it'd miss this:
> >
> >  void foo(void) {
> >     int notastring = 1;
> >     const char *format = "String is %s";
> >
> >     sprintf(format, notastring);
> >  }
> >
> > There are a handful of places in QEMU which do that with non-trivial
> > format strings & were easy to fix in this patch, which I think is a
> > worthwhile improvement. The cases in the *-user/strace.c file though
> > are not practical to fix, without significant re-design of the code
> > in question.
> 
> To be honest I couldn't tell from your patch whether you'd actually
> fixed any bugs or if you were just moving things around to turn non
> literals into literals.

There were no actual bugs fixed - it was just the change you describe
from non-literal to literal - to protect against future possible bugs.

> (Some of the cleanup looks like a good idea anyway, eg the vnc bits.)

Yep, I don't know why I didn't write that VNC code this way in the
first place now :-)

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 6/9] Fix bit test to use & instead of && and enable -Wlogical-op warning

[Maksim Kozlov]

02.04.2012 16:27, Peter Maydell пишет:
> On 2 April 2012 11:50, Daniel P. Berrange<berrange@redhat.com>  wrote:
>> diff --git a/hw/exynos4210_uart.c b/hw/exynos4210_uart.c
>> index 73a9c18..4b20105 100644
>> --- a/hw/exynos4210_uart.c
>> +++ b/hw/exynos4210_uart.c
>> @@ -246,7 +246,7 @@ static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
>>      uint32_t level = 0;
>>      uint32_t reg;
>>
>> -    reg = (s->reg[I_(UFCON)]&&  UFCON_Tx_FIFO_TRIGGER_LEVEL)>>
>> +    reg = (s->reg[I_(UFCON)]&  UFCON_Tx_FIFO_TRIGGER_LEVEL)>>
>>              UFCON_Tx_FIFO_TRIGGER_LEVEL_SHIFT;
>>
>>      switch (s->channel) {
>> @@ -277,7 +277,7 @@ static void exynos4210_uart_update_irq(Exynos4210UartState *s)
>>       */
>>      if (s->reg[I_(UFCON)]&&  UFCON_FIFO_ENABLE) {
>>
>> -        uint32_t count = (s->reg[I_(UFSTAT)]&&  UFSTAT_Tx_FIFO_COUNT)>>
>> +        uint32_t count = (s->reg[I_(UFSTAT)]&  UFSTAT_Tx_FIFO_COUNT)>>
>>                  UFSTAT_Tx_FIFO_COUNT_SHIFT;
>>
>>          if (count<= exynos4210_uart_Tx_FIFO_trigger_level(s)) {
>
> Nice catch. Note that the '&&  UFCON_FIFO_ENABLE' you can see in the context
> to the second hunk is also wrong and needs fixing.
>
really nice catch :) ridiculous mistake

> I'll take the exynos changes via arm-devs.next, but not the configure
> change. Please can you submit a version of the patch that only fixes
> the bugs and doesn't also change the gcc warning flags?
>
> thanks
> -- PMM
>
>

Re: [Qemu-devel] [PATCH 1/9] Move all compiler warning/optimization flags to the same place

[Stefan Weil]

Am 02.04.2012 12:50, schrieb Daniel P. Berrange:
> From: "Daniel P. Berrange" <berrange@redhat.com>
>
> The list of warning/optimization flags set in QEMU_CFLAGS is
> in two places in configure. Only one of the places checks
> for GCC support. Merge the two separate lists into one and
> ensure they are all tested. Set one flag per line to make
> it easier to read the list of flags as increasing numbers
> are enabled

The first list includes compiler flags which don't need
a test because all versions of gcc support them.

Only the flags in the second list must be tested.

Merging both list and testing all flags increases the time
needed for configure when the current test method is used.

It's possible to reduce the number of compiler invocations
during configure by passing all flags together to the
compiler and analyzing the error message which is returned.

Example:

gcc -Wtest1 -Wunsupported2 -E - < /dev/null
cc1: error: unrecognized command line option "-Wtest1"
cc1: error: unrecognized command line option "-Wunsupported2"

>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> configure | 31 ++++++++++++++++++++++++-------
> 1 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/configure b/configure
> index 4b3adc9..cd40d17 100755
> --- a/configure
> +++ b/configure
> @@ -252,9 +252,6 @@ pkg_config=query_pkg_config
> sdl_config="${SDL_CONFIG-${cross_prefix}sdl-config}"
>
> # default flags for all hosts
> -QEMU_CFLAGS="-fno-strict-aliasing $QEMU_CFLAGS"
> -QEMU_CFLAGS="-Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
> $QEMU_CFLAGS"
> -QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
> QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> $QEMU_CFLAGS"
> QEMU_CFLAGS="-D_FORTIFY_SOURCE=2 $QEMU_CFLAGS"
> QEMU_INCLUDES="-I. -I\$(SRC_PATH) -I\$(SRC_PATH)/fpu"
> @@ -1144,10 +1141,30 @@ else
> exit 1
> fi
>
> -gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
> -gcc_flags="-Wformat-security -Wformat-y2k -Winit-self 
> -Wignored-qualifiers $gcc_flags"
> -gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs 
> $gcc_flags"
> -gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
> +gcc_flags=
> +
> +# Optimization flags
> +gcc_flags="$gcc_flags -fstack-protector-all"
> +gcc_flags="$gcc_flags -fno-strict-aliasing"
> +
> +# Warning flags
> +gcc_flags="$gcc_flags -Wall"
> +gcc_flags="$gcc_flags -Wundef"
> +gcc_flags="$gcc_flags -Wwrite-strings"
> +gcc_flags="$gcc_flags -Wmissing-prototypes"
> +gcc_flags="$gcc_flags -Wstrict-prototypes"
> +gcc_flags="$gcc_flags -Wredundant-decls"
> +gcc_flags="$gcc_flags -Wold-style-declaration"
> +gcc_flags="$gcc_flags -Wold-style-definition"
> +gcc_flags="$gcc_flags -Wtype-limits"
> +gcc_flags="$gcc_flags -Wformat-security"
> +gcc_flags="$gcc_flags -Wformat-y2k"
> +gcc_flags="$gcc_flags -Winit-self"
> +gcc_flags="$gcc_flags -Wignored-qualifiers"
> +gcc_flags="$gcc_flags -Wmissing-include-dirs"
> +gcc_flags="$gcc_flags -Wempty-body"
> +gcc_flags="$gcc_flags -Wnested-externs"
> +gcc_flags="$gcc_flags -Wendif-labels"

Could you sort the -Wxxx flags? Then it's easier to avoid
duplicates, and merge conflicts will occur less often when
new flags are added.

> cat > $TMPC << EOF
> int main(void) { return 0; }
> EOF

Regards,

Stefan Weil

Re: [Qemu-devel] [PATCH 2/9] Fix checking for compiler flag support

[Stefan Weil]

Am 02.04.2012 12:50, schrieb Daniel P. Berrange:
> From: "Daniel P. Berrange" <berrange@redhat.com>
>
> Some warning flags have dependancies, eg -Wformat-security cannot
> be enabled if -Wformat is not already enabled. The compiler
> flag checking code was checking each flag in isolation so several
> were not getting enabled. The fix is to supply all previously
> confirmed flags when checking a flag
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> configure | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>

This patch is not needed, because the current test adds
the supported flags to QEMU_CFLAGS which is used in compile_prog,
so it does exactly the same thing which you try to achieve.

For flags which have dependencies, the order is important, so
a comment for such cases would be good to avoid accidental
reordering (see your previous patch 1/9).

Regards,
Stefan W.

Re: [Qemu-devel] [PATCH 3/9] Print out progress when checking compiler flags

[Stefan Weil]

Am 02.04.2012 12:50, schrieb Daniel P. Berrange:
> From: "Daniel P. Berrange"<berrange@redhat.com>
>
> Normal practice for autoconf style scripts is to print out
> progress. The QEMU configure script is getting increasingly
> slow&  has no progress feedback. Print out the progress of
> checking each compiler flag
>
> Signed-off-by: Daniel P. Berrange<berrange@redhat.com>
> ---
>   configure |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/configure b/configure
> index 64ab4dc..44b28c8 100755
> --- a/configure
> +++ b/configure
> @@ -1170,8 +1170,12 @@ int main(void) { return 0; }
>   EOF
>   warning_flags=
>   for flag in $gcc_flags; do
> +    echo -n "checking if $cc supports $flag... "
>       if compile_prog "-Werror $warning_flags $flag" "" ; then
>   	warning_flags="$warning_flags $flag"
> +	echo "yes"
> +    else
> +	echo "no"
>       fi
>   done
>   QEMU_CFLAGS="$QEMU_CFLAGS $warning_flags"

This is not needed if we check all flags in a single step
(see my comment to patch 1/9).

IMHO, configure should not print progress feedback by default.

Re: [Qemu-devel] [PATCH 9/9] Add note about some other options potentially worth enabling

[Stefan Weil]

Am 02.04.2012 12:50, schrieb Daniel P. Berrange:
> From: "Daniel P. Berrange" <berrange@redhat.com>
>
> There are a few other GCC warning options likely worth
> enabling, but it is not practical with the level of warnings
> generated. Add a note about them for anyone motiviated to
> address it in the future
>
> * configure: Add -Wclobbered, -Wmissing-field-initializers,
> -Woverride-init, -Wsign-compare, -Wunused-parameter,
> -Wunused-but-set-parameter, -Wpointer-arith, -Wmissing-noreturn,
> -Wjump-misses-init but leave disabled for now
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> configure | 20 ++++++++++++++++++++
> 1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/configure b/configure
> index 175901f..2d62337 100755
> --- a/configure
> +++ b/configure
> @@ -1199,6 +1199,26 @@ gcc_flags="$gcc_flags -Wformat-extra-args"
> gcc_flags="$gcc_flags -Wformat-zero-length"
> gcc_flags="$gcc_flags -Wformat-nonliteral"
>
> +# Some other potentially worth enabling once issues are fixed
> +# False positives in cpu-exec.c
> +#gcc_flags="$gcc_flags -Wclobbered"
> +# Many many violations
> +#gcc_flags="$gcc_flags -Wmissing-field-initializers"
> +# Quite a few (intentional?) overrides
> +#gcc_flags="$gcc_flags -Woverride-init"
> +# Many many violations
> +#gcc_flags="$gcc_flags -Wsign-compare"
> +# Many many violations
> +#gcc_flags="$gcc_flags -Wunused-parameter"
> +# Strange violations in mips helper
> +#gcc_flags="$gcc_flags -Wunused-but-set-parameter"

Well, those violations in the MIPS code are not strange:
they are simply errors in the code, so that comment should
say "Bugs in mips helper".

By the way, I also frequently compile with additional warnings
and use -Wextra (and -Wno-xxx for those errors which I don't
want to see because there are many many violations :-)).
That's how I found the MIPS errors - a patch was sent and is
waiting for reviewers (http://patchwork.ozlabs.org/patch/144478/).

Cheers,
Stefan W.