[Qemu-devel] [PATCH v3 0/2] Fix tests on recent gcc

[Qemu-devel] [PATCH v3 0/2] Fix tests on recent gcc

[Juan Quintela]

Hi

changes from v2:
- change assert from position

Please review.

[v2]

- move all uses of -1 for memtest.
- remove docs about the feature.
- use size_t for the second patch (cedric)

Please, review.


[v1]

On Fedora 26 (gcc-7.1.1 and glib2 2.52.3) tests don't compile.

In file included from /usr/include/string.h:639:0,
                 from /mnt/kvm/qemu/cleanup/include/qemu/osdep.h:69,
                 from /mnt/kvm/qemu/cleanup/tests/test-iov.c:1:
In function ‘memcpy’,
    inlined from ‘iov_from_buf’ at /mnt/kvm/qemu/cleanup/include/qemu/iov.h:51:9,
    inlined from ‘test_to_from_buf_1’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:88:14,
    inlined from ‘test_to_from_buf’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:144:9:
/usr/include/bits/string3.h:53:10: error: ‘__builtin_memcpy’: specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘memcpy’,
    inlined from ‘iov_to_buf’ at /mnt/kvm/qemu/cleanup/include/qemu/iov.h:64:9,
    inlined from ‘test_to_from_buf_1’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:94:14,
    inlined from ‘test_to_from_buf’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:144:9:
/usr/include/bits/string3.h:53:10: error: ‘__builtin_memcpy’: specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

In this case, we are abusing the functions and call a size_t argument
with -1, which gives us a very big number.  gcc gets overzealous and
gets confused about it.  Notice that this was introduced in 2015 by Paolo:

commit ad523bca56a7202d2498c550a41be5c986c4d33c
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Tue Dec 22 12:03:33 2015 +0100

    iov: avoid memcpy for "simple" iov_from_buf/iov_to_buf

I fixed it by using the real sizes in the tests insntead of -1.  It is already calculated.




In file included from /usr/include/glib-2.0/glib/glist.h:32:0,
                 from /usr/include/glib-2.0/glib/ghash.h:33,
                 from /usr/include/glib-2.0/glib.h:50,
                 from /mnt/kvm/qemu/cleanup/tests/vmgenid-test.c:11:
In function ‘acpi_find_vgia’,
    inlined from ‘read_guid_from_memory’ at /mnt/kvm/qemu/cleanup/tests/vmgenid-test.c:103:18:
/usr/include/glib-2.0/glib/gmem.h:216:10: error: argument 1 range [18446744071562067968, 18446744073709551615] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
      __p = g_##func##_n (__n, __s);   \
      ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmem.h:278:42: note: in expansion of macro ‘_G_NEW’
 #define g_new0(struct_type, n_structs)   _G_NEW (struct_type, n_structs, malloc0)
                                          ^~~~~~
/mnt/kvm/qemu/cleanup/tests/vmgenid-test.c:70:14: note: in expansion of macro ‘g_new0’
     tables = g_new0(uint32_t, tables_nr);
              ^~~~~~
/mnt/kvm/qemu/cleanup/tests/vmgenid-test.c: In function ‘read_guid_from_memory’:
/usr/include/glib-2.0/glib/gmem.h:96:10: note: in a call to allocation function ‘g_malloc0_n’ declared here
 gpointer g_malloc0_n      (gsize  n_blocks,
          ^~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/mnt/kvm/qemu/cleanup/rules.mak:66: tests/vmgenid-test.o] Error 1


this cames form line:

    tables = g_new0(uint32_t, tables_nr);

glib/gcc gets completely confused about this and think that 1st
argument can be 2^64-1.  Documentation says that you should use g_new
for struct types, so ...  I moved to g_malloc0() and call it a day.

What do you think?

Later, Juan.

Juan Quintela (2):
  tests: Use real size for iov tests
  tests: Make vmgenid test compile

 include/qemu/iov.h   |  6 ------
 tests/test-iov.c     | 10 +++++-----
 tests/vmgenid-test.c |  2 +-
 3 files changed, 6 insertions(+), 12 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v3 1/2] tests: Use real size for iov tests

[Juan Quintela]

We were using -1 instead of the real size because the functions check
what is bigger, size in bytes or the size of the iov.  Recent gcc's
barf at this.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
--

Remove comments about this feature.
Fix missing -1.
---
 include/qemu/iov.h |  6 ------
 tests/test-iov.c   | 10 +++++-----
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index bd9fd55b0a..72d4c559b4 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -31,11 +31,6 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
  * Number of bytes actually copied will be returned, which is
  *  min(bytes, iov_size(iov)-offset)
  * `Offset' must point to the inside of iovec.
- * It is okay to use very large value for `bytes' since we're
- * limited by the size of the iovec anyway, provided that the
- * buffer pointed to by buf has enough space.  One possible
- * such "large" value is -1 (sinice size_t is unsigned),
- * so specifying `-1' as `bytes' means 'up to the end of iovec'.
  */
 size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
                          size_t offset, const void *buf, size_t bytes);
@@ -76,7 +71,6 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
  * up to the end of it, will be filled with the specified value.
  * Function return actual number of bytes processed, which is
  * min(size, iov_size(iov) - offset).
- * Again, it is okay to use large value for `bytes' to mean "up to the end".
  */
 size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
                   size_t offset, int fillc, size_t bytes);
diff --git a/tests/test-iov.c b/tests/test-iov.c
index fa3d75aee1..458ca25099 100644
--- a/tests/test-iov.c
+++ b/tests/test-iov.c
@@ -81,17 +81,17 @@ static void test_to_from_buf_1(void)
           * skip whole vector and process exactly 0 bytes */
 
          /* first set bytes [i..sz) to some "random" value */
-         n = iov_memset(iov, niov, 0, 0xff, -1);
+         n = iov_memset(iov, niov, 0, 0xff, sz);
          g_assert(n == sz);
 
          /* next copy bytes [i..sz) from ibuf to iovec */
-         n = iov_from_buf(iov, niov, i, ibuf + i, -1);
+         n = iov_from_buf(iov, niov, i, ibuf + i, sz - i);
          g_assert(n == sz - i);
 
          /* clear part of obuf */
          memset(obuf + i, 0, sz - i);
          /* and set this part of obuf to values from iovec */
-         n = iov_to_buf(iov, niov, i, obuf + i, -1);
+         n = iov_to_buf(iov, niov, i, obuf + i, sz - i);
          g_assert(n == sz - i);
 
          /* now compare resulting buffers */
@@ -109,7 +109,7 @@ static void test_to_from_buf_1(void)
               * with j in [i..sz]. */
 
              /* clear iovec */
-             n = iov_memset(iov, niov, 0, 0xff, -1);
+             n = iov_memset(iov, niov, 0, 0xff, sz);
              g_assert(n == sz);
 
              /* copy bytes [i..j) from ibuf to iovec */
@@ -225,7 +225,7 @@ static void test_io(void)
        for (i = 0; i <= sz; ++i) {
            for (j = i; j <= sz; ++j) {
                k = i;
-               iov_memset(iov, niov, 0, 0xff, -1);
+               iov_memset(iov, niov, 0, 0xff, sz);
                do {
                    s = g_test_rand_int_range(0, j - k + 1);
                    r = iov_recv(sv[0], iov, niov, k, s);
-- 
2.13.5

[Qemu-devel] [PATCH v3 2/2] tests: Make vmgenid test compile

[Juan Quintela]

Just make sure that nr_tables is size_t not int.

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

--

Drop the s/g_new0/g_malloc0/ change.
---
 tests/vmgenid-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 3d5c1c3615..bcd3d7ec07 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
     AcpiRsdpDescriptor rsdp_table;
     uint32_t rsdt;
     AcpiRsdtDescriptorRev1 rsdt_table;
-    int tables_nr;
+    size_t tables_nr;
     uint32_t *tables;
     AcpiTableHeader ssdt_table;
     VgidTable vgid_table;
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 0/2] Fix tests on recent gcc

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> Hi
>
> changes from v2:
> - change assert from position

SelfNAK

Just sent the v2 patches instead of v3.  A v4 is already on list with
the right patches.

Sorry, Juan.