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

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

[Juan Quintela]

Hi

Changes from v1:
- 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 v2 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>

--

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 a22d71fd2c..f40f8f5260 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 */
@@ -226,7 +226,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

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

[Cleber Rosa]

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



On 08/30/2017 07:33 AM, Juan Quintela wrote:
> 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>
> 
> --
> 
> 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 a22d71fd2c..f40f8f5260 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 */
> @@ -226,7 +226,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);
> 

Tested-by: Cleber Rosa <crosa@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Eric Blake]

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

On 08/30/2017 06:33 AM, Juan Quintela wrote:
> 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>
> 
> --
> 
> 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.

Is this part of the comment still okay?

>  One possible
> - * such "large" value is -1 (sinice size_t is unsigned),

Nice that we're losing the typo in the process :)

> - * so specifying `-1' as `bytes' means 'up to the end of iovec'.

I agree with dropping this though.  As mentioned elsewhere, we had
crossed mails between v1 reviews and v2 submission, so we'll probably
need a v3 anyways.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v2 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>

--

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 v2 2/2] tests: Make vmgenid test compile

[Cleber Rosa]

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



On 08/30/2017 07:33 AM, Juan Quintela wrote:
> 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>
> 
> --
> 
> 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;
> 

Tested-by: Cleber Rosa <crosa@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]