* [Qemu-devel] [PATCH v2 0/2] Fix tests on recent gcc
@ 2017-08-30 11:33 Juan Quintela
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests Juan Quintela
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 2/2] tests: Make vmgenid test compile Juan Quintela
0 siblings, 2 replies; 6+ messages in thread
From: Juan Quintela @ 2017-08-30 11:33 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx, pbonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests
2017-08-30 11:33 [Qemu-devel] [PATCH v2 0/2] Fix tests on recent gcc Juan Quintela
@ 2017-08-30 11:33 ` Juan Quintela
2017-08-30 20:44 ` Cleber Rosa
2017-08-30 21:10 ` Eric Blake
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 2/2] tests: Make vmgenid test compile Juan Quintela
1 sibling, 2 replies; 6+ messages in thread
From: Juan Quintela @ 2017-08-30 11:33 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx, pbonzini
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] tests: Make vmgenid test compile
2017-08-30 11:33 [Qemu-devel] [PATCH v2 0/2] Fix tests on recent gcc Juan Quintela
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests Juan Quintela
@ 2017-08-30 11:33 ` Juan Quintela
2017-08-30 20:43 ` Cleber Rosa
1 sibling, 1 reply; 6+ messages in thread
From: Juan Quintela @ 2017-08-30 11:33 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, lvivier, peterx, pbonzini
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] tests: Make vmgenid test compile
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 2/2] tests: Make vmgenid test compile Juan Quintela
@ 2017-08-30 20:43 ` Cleber Rosa
0 siblings, 0 replies; 6+ messages in thread
From: Cleber Rosa @ 2017-08-30 20:43 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: lvivier, pbonzini, dgilbert, peterx
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests Juan Quintela
@ 2017-08-30 20:44 ` Cleber Rosa
2017-08-30 21:10 ` Eric Blake
1 sibling, 0 replies; 6+ messages in thread
From: Cleber Rosa @ 2017-08-30 20:44 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: lvivier, pbonzini, dgilbert, peterx
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests Juan Quintela
2017-08-30 20:44 ` Cleber Rosa
@ 2017-08-30 21:10 ` Eric Blake
1 sibling, 0 replies; 6+ messages in thread
From: Eric Blake @ 2017-08-30 21:10 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: lvivier, pbonzini, dgilbert, peterx
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-30 21:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-30 11:33 [Qemu-devel] [PATCH v2 0/2] Fix tests on recent gcc Juan Quintela
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests Juan Quintela
2017-08-30 20:44 ` Cleber Rosa
2017-08-30 21:10 ` Eric Blake
2017-08-30 11:33 ` [Qemu-devel] [PATCH v2 2/2] tests: Make vmgenid test compile Juan Quintela
2017-08-30 20:43 ` Cleber Rosa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.