Hi,
Fedora 26 has gcc 7.0.1 which has the normal compliment
of new fussy warnings; so far I've posted :
tests/check-qdict: Fix missing brackets
slirp/smb: Replace constant strings by glib string
that fix one actual mistake and work around something it's being
fussy over.
But I've also got a pile of hacks, attached below that I'm
not too sure what I'll do with them yet, but they're attached
for anyone else trying to build. Note they're smoke-only-tested.
I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
filed for what I reckon is a couple of overly pessimistic warnings.
Enjoy,
Dave
From 15353ce59e35e1d85927138982241491ea65cee2 Mon Sep 17 00:00:00 2001
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Date: Thu, 6 Apr 2017 15:44:50 +0100
Subject: [HACK!] Hacks for f26 build
---
block/blkdebug.c | 4 ++--
block/blkverify.c | 4 ++--
hw/usb/bus.c | 5 +++--
include/qemu/iov.h | 4 ++--
tests/bios-tables-test.c | 2 +-
5 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 67e8024e36..34c645d095 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -689,9 +689,9 @@ static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
}
if (!force_json && bs->file->bs->exact_filename[0]) {
- snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+ g_assert_cmpint(snprintf(bs->exact_filename, sizeof(bs->exact_filename),
"blkdebug:%s:%s", s->config_file ?: "",
- bs->file->bs->exact_filename);
+ bs->file->bs->exact_filename), <, sizeof(bs->exact_filename));
}
opts = qdict_new();
diff --git a/block/blkverify.c b/block/blkverify.c
index 9a1e21c6ad..d038947a5a 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -305,10 +305,10 @@ static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options)
if (bs->file->bs->exact_filename[0]
&& s->test_file->bs->exact_filename[0])
{
- snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+ g_assert_cmpint(snprintf(bs->exact_filename, sizeof(bs->exact_filename),
"blkverify:%s:%s",
bs->file->bs->exact_filename,
- s->test_file->bs->exact_filename);
+ s->test_file->bs->exact_filename), <, sizeof(bs->exact_filename));
}
}
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 24f1608b4b..6023f3b419 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -8,6 +8,7 @@
#include "monitor/monitor.h"
#include "trace.h"
#include "qemu/cutils.h"
+#include <glib.h>
static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
@@ -407,8 +408,8 @@ void usb_register_companion(const char *masterbus, USBPort *ports[],
void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
{
if (upstream) {
- snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
- upstream->path, portnr);
+ g_assert_cmpint(snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
+ upstream->path, portnr), <, sizeof(downstream->path));
downstream->hubcount = upstream->hubcount + 1;
} else {
snprintf(downstream->path, sizeof(downstream->path), "%d", portnr);
diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index bd9fd55b0a..ebb0221140 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -46,7 +46,7 @@ static inline size_t
iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
size_t offset, const void *buf, size_t bytes)
{
- if (__builtin_constant_p(bytes) && iov_cnt &&
+ if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
memcpy(iov[0].iov_base + offset, buf, bytes);
return bytes;
@@ -59,7 +59,7 @@ static inline size_t
iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
size_t offset, void *buf, size_t bytes)
{
- if (__builtin_constant_p(bytes) && iov_cnt &&
+ if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
memcpy(buf, iov[0].iov_base + offset, bytes);
return bytes;
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 88dbf97853..c55de4f65b 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -98,7 +98,7 @@ static void test_acpi_rsdt_table(test_data *data)
AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
uint32_t addr = data->rsdp_table.rsdt_physical_address;
uint32_t *tables;
- int tables_nr;
+ unsigned int tables_nr;
uint8_t checksum;
/* read the header */
--
2.12.2
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hi Dave,
On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote:
> Hi,
> Fedora 26 has gcc 7.0.1 which has the normal compliment
> of new fussy warnings; so far I've posted :
>
> tests/check-qdict: Fix missing brackets
> slirp/smb: Replace constant strings by glib string
>
> that fix one actual mistake and work around something it's being
> fussy over.
>
> But I've also got a pile of hacks, attached below that I'm
> not too sure what I'll do with them yet, but they're attached
> for anyone else trying to build. Note they're smoke-only-tested.
>
> I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
> filed for what I reckon is a couple of overly pessimistic warnings.
>
> Enjoy,
>
> Dave
>
> From 15353ce59e35e1d85927138982241491ea65cee2 Mon Sep 17 00:00:00 2001
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Date: Thu, 6 Apr 2017 15:44:50 +0100
> Subject: [HACK!] Hacks for f26 build
>
> ---
> block/blkdebug.c | 4 ++--
> block/blkverify.c | 4 ++--
> hw/usb/bus.c | 5 +++--
> include/qemu/iov.h | 4 ++--
> tests/bios-tables-test.c | 2 +-
> 5 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 67e8024e36..34c645d095 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -689,9 +689,9 @@ static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
> }
>
> if (!force_json && bs->file->bs->exact_filename[0]) {
> - snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> + g_assert_cmpint(snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> "blkdebug:%s:%s", s->config_file ?: "",
> - bs->file->bs->exact_filename);
> + bs->file->bs->exact_filename), <, sizeof(bs->exact_filename));
I'm not sure this works as expected if you compile with CPPFLAGS
+=-DG_DISABLE_ASSERT.
I added in me docker TODO "also build with -DG_DISABLE_ASSERT".
> }
>
> opts = qdict_new();
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 9a1e21c6ad..d038947a5a 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -305,10 +305,10 @@ static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options)
> if (bs->file->bs->exact_filename[0]
> && s->test_file->bs->exact_filename[0])
> {
> - snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> + g_assert_cmpint(snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> "blkverify:%s:%s",
> bs->file->bs->exact_filename,
> - s->test_file->bs->exact_filename);
> + s->test_file->bs->exact_filename), <, sizeof(bs->exact_filename));
> }
> }
>
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 24f1608b4b..6023f3b419 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -8,6 +8,7 @@
> #include "monitor/monitor.h"
> #include "trace.h"
> #include "qemu/cutils.h"
> +#include <glib.h>
>
> static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>
> @@ -407,8 +408,8 @@ void usb_register_companion(const char *masterbus, USBPort *ports[],
> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
> {
> if (upstream) {
> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
> - upstream->path, portnr);
> + g_assert_cmpint(snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
> + upstream->path, portnr), <, sizeof(downstream->path));
> downstream->hubcount = upstream->hubcount + 1;
> } else {
> snprintf(downstream->path, sizeof(downstream->path), "%d", portnr);
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index bd9fd55b0a..ebb0221140 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -46,7 +46,7 @@ static inline size_t
> iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
> size_t offset, const void *buf, size_t bytes)
> {
> - if (__builtin_constant_p(bytes) && iov_cnt &&
> + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> memcpy(iov[0].iov_base + offset, buf, bytes);
> return bytes;
> @@ -59,7 +59,7 @@ static inline size_t
> iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> size_t offset, void *buf, size_t bytes)
> {
> - if (__builtin_constant_p(bytes) && iov_cnt &&
> + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> memcpy(buf, iov[0].iov_base + offset, bytes);
> return bytes;
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 88dbf97853..c55de4f65b 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -98,7 +98,7 @@ static void test_acpi_rsdt_table(test_data *data)
> AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> uint32_t addr = data->rsdp_table.rsdt_physical_address;
> uint32_t *tables;
> - int tables_nr;
> + unsigned int tables_nr;
Personally I prefer size_t here.
> uint8_t checksum;
>
> /* read the header */
>
regards,
Phil.
On 04/07/2017 04:21 PM, Philippe Mathieu-Daudé wrote:
> Hi Dave,
>
> On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote:
>> Hi,
>> Fedora 26 has gcc 7.0.1 which has the normal compliment
>> of new fussy warnings; so far I've posted :
>>
>> tests/check-qdict: Fix missing brackets
>> slirp/smb: Replace constant strings by glib string
>>
>> that fix one actual mistake and work around something it's being
>> fussy over.
>>
>> But I've also got a pile of hacks, attached below that I'm
>> not too sure what I'll do with them yet, but they're attached
ping ... What do we do with them?
>> for anyone else trying to build. Note they're smoke-only-tested.
>>
>> I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
>> filed for what I reckon is a couple of overly pessimistic warnings.
>>
>> Enjoy,
>>
>> Dave
>>
>> From 15353ce59e35e1d85927138982241491ea65cee2 Mon Sep 17 00:00:00 2001
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Date: Thu, 6 Apr 2017 15:44:50 +0100
>> Subject: [HACK!] Hacks for f26 build
>>
>> ---
>> block/blkdebug.c | 4 ++--
>> block/blkverify.c | 4 ++--
>> hw/usb/bus.c | 5 +++--
>> include/qemu/iov.h | 4 ++--
>> tests/bios-tables-test.c | 2 +-
>> 5 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 67e8024e36..34c645d095 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -689,9 +689,9 @@ static void
>> blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
>> }
>>
>> if (!force_json && bs->file->bs->exact_filename[0]) {
>> - snprintf(bs->exact_filename, sizeof(bs->exact_filename),
>> + g_assert_cmpint(snprintf(bs->exact_filename,
>> sizeof(bs->exact_filename),
>> "blkdebug:%s:%s", s->config_file ?: "",
>> - bs->file->bs->exact_filename);
>> + bs->file->bs->exact_filename), <,
>> sizeof(bs->exact_filename));
>
> I'm not sure this works as expected if you compile with CPPFLAGS
> +=-DG_DISABLE_ASSERT.
>
> I added in me docker TODO "also build with -DG_DISABLE_ASSERT".
>
>> }
>>
>> opts = qdict_new();
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index 9a1e21c6ad..d038947a5a 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -305,10 +305,10 @@ static void
>> blkverify_refresh_filename(BlockDriverState *bs, QDict *options)
>> if (bs->file->bs->exact_filename[0]
>> && s->test_file->bs->exact_filename[0])
>> {
>> - snprintf(bs->exact_filename, sizeof(bs->exact_filename),
>> + g_assert_cmpint(snprintf(bs->exact_filename,
>> sizeof(bs->exact_filename),
>> "blkverify:%s:%s",
>> bs->file->bs->exact_filename,
>> - s->test_file->bs->exact_filename);
>> + s->test_file->bs->exact_filename), <,
>> sizeof(bs->exact_filename));
>> }
>> }
>>
>> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
>> index 24f1608b4b..6023f3b419 100644
>> --- a/hw/usb/bus.c
>> +++ b/hw/usb/bus.c
>> @@ -8,6 +8,7 @@
>> #include "monitor/monitor.h"
>> #include "trace.h"
>> #include "qemu/cutils.h"
>> +#include <glib.h>
>>
>> static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int
>> indent);
>>
>> @@ -407,8 +408,8 @@ void usb_register_companion(const char *masterbus,
>> USBPort *ports[],
>> void usb_port_location(USBPort *downstream, USBPort *upstream, int
>> portnr)
>> {
>> if (upstream) {
>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
>> - upstream->path, portnr);
>> + g_assert_cmpint(snprintf(downstream->path,
>> sizeof(downstream->path), "%s.%d",
>> + upstream->path, portnr), <, sizeof(downstream->path));
>> downstream->hubcount = upstream->hubcount + 1;
>> } else {
>> snprintf(downstream->path, sizeof(downstream->path), "%d",
>> portnr);
>> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
>> index bd9fd55b0a..ebb0221140 100644
>> --- a/include/qemu/iov.h
>> +++ b/include/qemu/iov.h
>> @@ -46,7 +46,7 @@ static inline size_t
>> iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
>> size_t offset, const void *buf, size_t bytes)
>> {
>> - if (__builtin_constant_p(bytes) && iov_cnt &&
>> + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
>> offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
>> memcpy(iov[0].iov_base + offset, buf, bytes);
>> return bytes;
>> @@ -59,7 +59,7 @@ static inline size_t
>> iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
>> size_t offset, void *buf, size_t bytes)
>> {
>> - if (__builtin_constant_p(bytes) && iov_cnt &&
>> + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
>> offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
>> memcpy(buf, iov[0].iov_base + offset, bytes);
>> return bytes;
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index 88dbf97853..c55de4f65b 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -98,7 +98,7 @@ static void test_acpi_rsdt_table(test_data *data)
>> AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
>> uint32_t addr = data->rsdp_table.rsdt_physical_address;
>> uint32_t *tables;
>> - int tables_nr;
>> + unsigned int tables_nr;
>
> Personally I prefer size_t here.
>
>> uint8_t checksum;
>>
>> /* read the header */
>>
>
> regards,
> Phil.
On 07/13/2017 08:07 AM, Philippe Mathieu-Daudé wrote: > On 04/07/2017 04:21 PM, Philippe Mathieu-Daudé wrote: >> Hi Dave, >> >> On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote: >>> Hi, >>> Fedora 26 has gcc 7.0.1 which has the normal compliment >>> of new fussy warnings; so far I've posted : >>> >>> tests/check-qdict: Fix missing brackets >>> slirp/smb: Replace constant strings by glib string >>> >>> that fix one actual mistake and work around something it's being >>> fussy over. >>> >>> But I've also got a pile of hacks, attached below that I'm >>> not too sure what I'll do with them yet, but they're attached > > ping ... What do we do with them? Well, now that I've upgraded to the just-released Fedora 26, it is now mainline gcc and affecting my builds. So we should really try and find patches that silence the warnings (although it counts as bug fixes, so it won't hurt if it doesn't make tomorrow's freeze deadline). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 07/17/2017 08:42 AM, Eric Blake wrote:
> On 07/13/2017 08:07 AM, Philippe Mathieu-Daudé wrote:
>> On 04/07/2017 04:21 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Dave,
>>>
>>> On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote:
>>>> Hi,
>>>> Fedora 26 has gcc 7.0.1 which has the normal compliment
>>>> of new fussy warnings; so far I've posted :
>>>>
>>>> tests/check-qdict: Fix missing brackets
>>>> slirp/smb: Replace constant strings by glib string
>>>>
>>>> that fix one actual mistake and work around something it's being
>>>> fussy over.
>>>>
>>>> But I've also got a pile of hacks, attached below that I'm
>>>> not too sure what I'll do with them yet, but they're attached
>>
>> ping ... What do we do with them?
>
> Well, now that I've upgraded to the just-released Fedora 26, it is now
> mainline gcc and affecting my builds. So we should really try and find
> patches that silence the warnings (although it counts as bug fixes, so
> it won't hurt if it doesn't make tomorrow's freeze deadline).
FWIW, most of these have been fixed in the meantime; the only remaining
hack I had to add was:
diff --git i/hw/usb/bus.c w/hw/usb/bus.c
index 5939b273b9..bce011058b 100644
--- i/hw/usb/bus.c
+++ w/hw/usb/bus.c
@@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
USBPort *ports[],
void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
{
if (upstream) {
- snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
- upstream->path, portnr);
+ int l = snprintf(downstream->path, sizeof(downstream->path),
"%s.%d",
+ upstream->path, portnr);
+ assert(l < sizeof(downstream->path));
downstream->hubcount = upstream->hubcount + 1;
} else {
snprintf(downstream->path, sizeof(downstream->path), "%d", portnr);
Gerd, MAINTAINERS lists you; can you come up with something more robust?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Hi,
> FWIW, most of these have been fixed in the meantime; the only
> remaining
> hack I had to add was:
>
> diff --git i/hw/usb/bus.c w/hw/usb/bus.c
> index 5939b273b9..bce011058b 100644
> --- i/hw/usb/bus.c
> +++ w/hw/usb/bus.c
> @@ -407,8 +407,9 @@ void usb_register_companion(const char
> *masterbus,
> USBPort *ports[],
> void usb_port_location(USBPort *downstream, USBPort *upstream, int
> portnr)
> {
> if (upstream) {
> - snprintf(downstream->path, sizeof(downstream->path),
> "%s.%d",
> - upstream->path, portnr);
> + int l = snprintf(downstream->path, sizeof(downstream->path),
> "%s.%d",
> + upstream->path, portnr);
> + assert(l < sizeof(downstream->path));
Approach looks ok to me.
Maximum hub chain length is 5, number of ports hubs is limited too,
you'll never need more that two digits for port numbers. So 2*5 plus 4
connecting dots => 14 chars is the max strlen. path size is 16, so it
will fit, including the terminating \0.
Trying things like "assert(portnr <= 99)" have no effect on the
possible string length calculated by gcc7.
cheers,
Gerd
On 07/17/2017 09:16 AM, Gerd Hoffmann wrote:
> Hi,
>
>> FWIW, most of these have been fixed in the meantime; the only
>> remaining
>> hack I had to add was:
>>
>> diff --git i/hw/usb/bus.c w/hw/usb/bus.c
>> index 5939b273b9..bce011058b 100644
>> --- i/hw/usb/bus.c
>> +++ w/hw/usb/bus.c
>> @@ -407,8 +407,9 @@ void usb_register_companion(const char
>> *masterbus,
>> USBPort *ports[],
>> void usb_port_location(USBPort *downstream, USBPort *upstream, int
>> portnr)
>> {
>> if (upstream) {
>> - snprintf(downstream->path, sizeof(downstream->path),
>> "%s.%d",
>> - upstream->path, portnr);
>> + int l = snprintf(downstream->path, sizeof(downstream->path),
>> "%s.%d",
>> + upstream->path, portnr);
>> + assert(l < sizeof(downstream->path));
>
> Approach looks ok to me.
>
> Maximum hub chain length is 5, number of ports hubs is limited too,
> you'll never need more that two digits for port numbers. So 2*5 plus 4
> connecting dots => 14 chars is the max strlen. path size is 16, so it
> will fit, including the terminating \0.
Cool! With that text in the commit message and/or code comment, we can
turn this from a hack into an actual patch, with no change in code.
I'll go ahead and spin this as a formal patch.
>
> Trying things like "assert(portnr <= 99)" have no effect on the
> possible string length calculated by gcc7.
Perhaps a limitation that ought to be reported to the gcc folks.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
* Eric Blake (eblake@redhat.com) wrote:
> On 07/17/2017 08:42 AM, Eric Blake wrote:
> > On 07/13/2017 08:07 AM, Philippe Mathieu-Daudé wrote:
> >> On 04/07/2017 04:21 PM, Philippe Mathieu-Daudé wrote:
> >>> Hi Dave,
> >>>
> >>> On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote:
> >>>> Hi,
> >>>> Fedora 26 has gcc 7.0.1 which has the normal compliment
> >>>> of new fussy warnings; so far I've posted :
> >>>>
> >>>> tests/check-qdict: Fix missing brackets
> >>>> slirp/smb: Replace constant strings by glib string
> >>>>
> >>>> that fix one actual mistake and work around something it's being
> >>>> fussy over.
> >>>>
> >>>> But I've also got a pile of hacks, attached below that I'm
> >>>> not too sure what I'll do with them yet, but they're attached
> >>
> >> ping ... What do we do with them?
> >
> > Well, now that I've upgraded to the just-released Fedora 26, it is now
> > mainline gcc and affecting my builds. So we should really try and find
> > patches that silence the warnings (although it counts as bug fixes, so
> > it won't hurt if it doesn't make tomorrow's freeze deadline).
>
> FWIW, most of these have been fixed in the meantime; the only remaining
> hack I had to add was:
>
> diff --git i/hw/usb/bus.c w/hw/usb/bus.c
> index 5939b273b9..bce011058b 100644
> --- i/hw/usb/bus.c
> +++ w/hw/usb/bus.c
> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
> USBPort *ports[],
> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
> {
> if (upstream) {
> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
> - upstream->path, portnr);
> + int l = snprintf(downstream->path, sizeof(downstream->path),
> "%s.%d",
> + upstream->path, portnr);
> + assert(l < sizeof(downstream->path));
You may find this doesn't help in some windows builds; the assert
functions aren't always marked as noreturn (because they pop up a dialog
that asks you whether you want to run into a debugger etc).
Dave
> downstream->hubcount = upstream->hubcount + 1;
> } else {
> snprintf(downstream->path, sizeof(downstream->path), "%d", portnr);
>
> Gerd, MAINTAINERS lists you; can you come up with something more robust?
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 17 July 2017 at 17:46, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > You may find this doesn't help in some windows builds; the assert > functions aren't always marked as noreturn (because they pop up a dialog > that asks you whether you want to run into a debugger etc). Oh, is that why? In any case, we rely on g_assert() and friends from glib being definitely marked noreturn, so we could use them instead. thanks -- PMM
On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote:
>> +++ w/hw/usb/bus.c
>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
>> USBPort *ports[],
>> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
>> {
>> if (upstream) {
>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
>> - upstream->path, portnr);
>> + int l = snprintf(downstream->path, sizeof(downstream->path),
>> "%s.%d",
>> + upstream->path, portnr);
>> + assert(l < sizeof(downstream->path));
>
> You may find this doesn't help in some windows builds; the assert
> functions aren't always marked as noreturn (because they pop up a dialog
> that asks you whether you want to run into a debugger etc).
How would it not help? Are we using gcc 7 on windows builds? Adding
the assert is enough to shut up new gcc; old gcc was already silent; and
if mingw is still on old gcc, it doesn't matter whether assert() is
marked noreturn for what this patch is doing.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote:
> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote:
>
>>> +++ w/hw/usb/bus.c
>>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
>>> USBPort *ports[],
>>> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
>>> {
>>> if (upstream) {
>>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
>>> - upstream->path, portnr);
>>> + int l = snprintf(downstream->path, sizeof(downstream->path),
>>> "%s.%d",
>>> + upstream->path, portnr);
>>> + assert(l < sizeof(downstream->path));
>>
>> You may find this doesn't help in some windows builds; the assert
>> functions aren't always marked as noreturn (because they pop up a dialog
>> that asks you whether you want to run into a debugger etc).
>
> How would it not help? Are we using gcc 7 on windows builds?
At some point in the future we are likely to, because my
w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64
packages, and so as I upgrade my desktop they will move
forward to newer gcc versions. (More generally our users
may do so before me ;-))
We should be consistent -- if we can't trust assert() to
be marked nonreturn, as it seems we can't, then we shouldn't
write new code that assumes it always is, even if today
it doesn't happen to bite us on the compiler/host combinations
we're testing right now.
thanks
-- PMM
On 07/17/2017 12:36 PM, Peter Maydell wrote:
> On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote:
>> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote:
>>
>>>> +++ w/hw/usb/bus.c
>>>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
>>>> USBPort *ports[],
>>>> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
>>>> {
>>>> if (upstream) {
>>>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
>>>> - upstream->path, portnr);
>>>> + int l = snprintf(downstream->path, sizeof(downstream->path),
>>>> "%s.%d",
>>>> + upstream->path, portnr);
>>>> + assert(l < sizeof(downstream->path));
>>>
>>> You may find this doesn't help in some windows builds; the assert
>>> functions aren't always marked as noreturn (because they pop up a dialog
>>> that asks you whether you want to run into a debugger etc).
>>
>> How would it not help? Are we using gcc 7 on windows builds?
>
> At some point in the future we are likely to, because my
> w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64
> packages, and so as I upgrade my desktop they will move
> forward to newer gcc versions. (More generally our users
> may do so before me ;-))
So, does gcc's warning actually depend on the no-return-ness of the
assert() statement added here?
Would there be anything wrong with making osdep.h do this on mingw:
#include <assert.h>
#undef assert
#define assert(expr) g_assert(expr)
so that we can reliably get no-return handling that we desire, without
having to audit lots of other code?
>
> We should be consistent -- if we can't trust assert() to
> be marked nonreturn, as it seems we can't, then we shouldn't
> write new code that assumes it always is, even if today
> it doesn't happen to bite us on the compiler/host combinations
> we're testing right now.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
* Eric Blake (eblake@redhat.com) wrote:
> On 07/17/2017 12:36 PM, Peter Maydell wrote:
> > On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote:
> >> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote:
> >>
> >>>> +++ w/hw/usb/bus.c
> >>>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
> >>>> USBPort *ports[],
> >>>> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
> >>>> {
> >>>> if (upstream) {
> >>>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
> >>>> - upstream->path, portnr);
> >>>> + int l = snprintf(downstream->path, sizeof(downstream->path),
> >>>> "%s.%d",
> >>>> + upstream->path, portnr);
> >>>> + assert(l < sizeof(downstream->path));
> >>>
> >>> You may find this doesn't help in some windows builds; the assert
> >>> functions aren't always marked as noreturn (because they pop up a dialog
> >>> that asks you whether you want to run into a debugger etc).
> >>
> >> How would it not help? Are we using gcc 7 on windows builds?
> >
> > At some point in the future we are likely to, because my
> > w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64
> > packages, and so as I upgrade my desktop they will move
> > forward to newer gcc versions. (More generally our users
> > may do so before me ;-))
>
> So, does gcc's warning actually depend on the no-return-ness of the
> assert() statement added here?
I'm not sure in this case, I'd hit it previously though, see my comment
from 2015 in:
https://sourceforge.net/p/mingw-w64/bugs/306/
> Would there be anything wrong with making osdep.h do this on mingw:
>
> #include <assert.h>
> #undef assert
> #define assert(expr) g_assert(expr)
>
> so that we can reliably get no-return handling that we desire, without
> having to audit lots of other code?
It's a bit nasty, but hmm.
I only just about trust glib not to change, all of the fancier g_assert
variants can return, but g_assert is still safe.
Dave
> >
> > We should be consistent -- if we can't trust assert() to
> > be marked nonreturn, as it seems we can't, then we shouldn't
> > write new code that assumes it always is, even if today
> > it doesn't happen to bite us on the compiler/host combinations
> > we're testing right now.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/17/2017 02:36 PM, Peter Maydell wrote: > On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote: >> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: [...] >>> You may find this doesn't help in some windows builds; the assert >>> functions aren't always marked as noreturn (because they pop up a dialog >>> that asks you whether you want to run into a debugger etc). >> >> How would it not help? Are we using gcc 7 on windows builds? > > At some point in the future we are likely to, because my > w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64 > packages, and so as I upgrade my desktop they will move > forward to newer gcc versions. (More generally our users > may do so before me ;-)) The MXE toolchain (mxe.cc) provides GCC version 5.4.0 and compiles fine: win32: https://app.shippable.com/github/philmd/qemu/runs/32/2/console # file i386-softmmu/qemu-system-i386.exe i386-softmmu/qemu-system-i386.exe: PE32 executable (console) Intel 80386, for MS Windows and win64: https://app.shippable.com/github/philmd/qemu/runs/32/3/console # file x86_64-softmmu/qemu-system-x86_64.exe x86_64-softmmu/qemu-system-x86_64.exe: PE32+ executable (console) x86-64, for MS Windows > We should be consistent -- if we can't trust assert() to > be marked nonreturn, as it seems we can't, then we shouldn't > write new code that assumes it always is, even if today > it doesn't happen to bite us on the compiler/host combinations > we're testing right now. And there is also the problem when you compiles with CPPFLAGS+=-DNDEBUG some oldschool guys still have in their ~/.cshrc ;) Phil.
On 07/18/2017 10:04 AM, Philippe Mathieu-Daudé wrote: >> We should be consistent -- if we can't trust assert() to >> be marked nonreturn, as it seems we can't, then we shouldn't >> write new code that assumes it always is, even if today >> it doesn't happen to bite us on the compiler/host combinations >> we're testing right now. > > And there is also the problem when you compiles with CPPFLAGS+=-DNDEBUG > some oldschool guys still have in their ~/.cshrc ;) We don't have problems with people defining NDEBUG in their environment; such people would already hit at least: hw/scsi/mptsas.c:#ifdef NDEBUG hw/scsi/mptsas.c:#error building with NDEBUG is not supported (maybe we should hoist that to osdep.h, though) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 18.07.2017 17:10, Eric Blake wrote: > On 07/18/2017 10:04 AM, Philippe Mathieu-Daudé wrote: >>> We should be consistent -- if we can't trust assert() to >>> be marked nonreturn, as it seems we can't, then we shouldn't >>> write new code that assumes it always is, even if today >>> it doesn't happen to bite us on the compiler/host combinations >>> we're testing right now. >> >> And there is also the problem when you compiles with CPPFLAGS+=-DNDEBUG >> some oldschool guys still have in their ~/.cshrc ;) > > We don't have problems with people defining NDEBUG in their environment; > such people would already hit at least: > > hw/scsi/mptsas.c:#ifdef NDEBUG > hw/scsi/mptsas.c:#error building with NDEBUG is not supported > > (maybe we should hoist that to osdep.h, though) Yes, please. Not every target is build with CONFIG_MPTSAS_SCSI_PCI so we should move that to a more central place. Thomas
* Eric Blake (eblake@redhat.com) wrote:
> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote:
>
> >> +++ w/hw/usb/bus.c
> >> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
> >> USBPort *ports[],
> >> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
> >> {
> >> if (upstream) {
> >> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
> >> - upstream->path, portnr);
> >> + int l = snprintf(downstream->path, sizeof(downstream->path),
> >> "%s.%d",
> >> + upstream->path, portnr);
> >> + assert(l < sizeof(downstream->path));
> >
> > You may find this doesn't help in some windows builds; the assert
> > functions aren't always marked as noreturn (because they pop up a dialog
> > that asks you whether you want to run into a debugger etc).
>
> How would it not help? Are we using gcc 7 on windows builds? Adding
> the assert is enough to shut up new gcc; old gcc was already silent; and
> if mingw is still on old gcc, it doesn't matter whether assert() is
> marked noreturn for what this patch is doing.
[dgilbert@dgilbert-t530 ~]$ x86_64-w64-mingw32-gcc -v
Using built-in specs.
COLLECT_GCC=/usr/bin/x86_64-w64-mingw32-gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-w64-mingw32/7.1.0/lto-wrapper
Target: x86_64-w64-mingw32
Configured with: ../configure --prefix=/usr --bindir=/usr/bin --includedir=/usr/include --mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --with-gnu-as --with-gnu-ld --verbose --without-newlib --disable-multilib --disable-plugin --with-system-zlib --disable-nls --without-included-gettext --disable-win32-registry --enable-languages=c,c++,objc,obj-c++,fortran --with-bugurl=http://bugzilla.redhat.com/bugzilla --with-cloog --enable-threads=posix --enable-libgomp --target=x86_64-w64-mingw32 --with-sysroot=/usr/x86_64-w64-mingw32/sys-root --with-gxx-include-dir=/usr/x86_64-w64-mingw32/sys-root/mingw/include/c++
Thread model: posix
gcc version 7.1.0 20170502 (Fedora MinGW 7.1.0-1.fc26) (GCC)
Dave
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Mon, Jul 17, 2017 at 12:29:08PM -0500, Eric Blake wrote:
> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote:
>
> >> +++ w/hw/usb/bus.c
> >> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
> >> USBPort *ports[],
> >> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
> >> {
> >> if (upstream) {
> >> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
> >> - upstream->path, portnr);
> >> + int l = snprintf(downstream->path, sizeof(downstream->path),
> >> "%s.%d",
> >> + upstream->path, portnr);
> >> + assert(l < sizeof(downstream->path));
> >
> > You may find this doesn't help in some windows builds; the assert
> > functions aren't always marked as noreturn (because they pop up a dialog
> > that asks you whether you want to run into a debugger etc).
>
> How would it not help? Are we using gcc 7 on windows builds? Adding
> the assert is enough to shut up new gcc; old gcc was already silent; and
> if mingw is still on old gcc, it doesn't matter whether assert() is
> marked noreturn for what this patch is doing.
Mingw isn't using a fork of GCC anymore, its all mainline. Thus Fedora's
mingw gcc packages track native gcc packages. IOW i686-w64-mingw32-gcc
is already on version 7.1.0 in Fedora
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 07/18/2017 02:23 AM, Daniel P. Berrange wrote: >> How would it not help? Are we using gcc 7 on windows builds? Adding >> the assert is enough to shut up new gcc; old gcc was already silent; and >> if mingw is still on old gcc, it doesn't matter whether assert() is >> marked noreturn for what this patch is doing. > > Mingw isn't using a fork of GCC anymore, its all mainline. Thus Fedora's > mingw gcc packages track native gcc packages. IOW i686-w64-mingw32-gcc > is already on version 7.1.0 in Fedora So I guess that means running 'make docker-test-mingw@fedora' on Fedora 26 will find out if we need further tweaks? Trying it now... Nope, didn't get very far :( $ make docker-test-mingw@fedora BUILD fedora make[1]: Entering directory '/home/eblake/qemu' ARCHIVE qemu.tgz usage: git archive [<options>] <tree-ish> [<path>...] ... make[1]: *** [/home/eblake/qemu/tests/docker/Makefile.include:35: docker-src.2017-07-18-07.35.20.4101] Error 129 make[1]: Leaving directory '/home/eblake/qemu' make: *** [/home/eblake/qemu/tests/docker/Makefile.include:156: docker-run-test-mingw@fedora] Error 2 Am I not doing it right? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Jul 18, 2017 at 07:35:58AM -0500, Eric Blake wrote: > On 07/18/2017 02:23 AM, Daniel P. Berrange wrote: > >> How would it not help? Are we using gcc 7 on windows builds? Adding > >> the assert is enough to shut up new gcc; old gcc was already silent; and > >> if mingw is still on old gcc, it doesn't matter whether assert() is > >> marked noreturn for what this patch is doing. > > > > Mingw isn't using a fork of GCC anymore, its all mainline. Thus Fedora's > > mingw gcc packages track native gcc packages. IOW i686-w64-mingw32-gcc > > is already on version 7.1.0 in Fedora > > So I guess that means running 'make docker-test-mingw@fedora' on Fedora > 26 will find out if we need further tweaks? Trying it now... > > Nope, didn't get very far :( > > $ make docker-test-mingw@fedora > BUILD fedora > make[1]: Entering directory '/home/eblake/qemu' > ARCHIVE qemu.tgz > usage: git archive [<options>] <tree-ish> [<path>...] > ... > make[1]: *** [/home/eblake/qemu/tests/docker/Makefile.include:35: > docker-src.2017-07-18-07.35.20.4101] Error 129 > make[1]: Leaving directory '/home/eblake/qemu' > make: *** [/home/eblake/qemu/tests/docker/Makefile.include:156: > docker-run-test-mingw@fedora] Error 2 > > Am I not doing it right? That works fine for me - perhaps V=1 will tell you what's wrong with the git archive command. In any case, despite having gcc 7.1.0, I don't see the error messages in question when running a mingw build. It could be that the new problems GCC 7 reports are only triggered when combined with a suitable C library like GCC, to get the symbols annotated to enable fortify source to check them. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 07/17/2017 03:48 AM, Eric Blake wrote:
> On 07/17/2017 08:42 AM, Eric Blake wrote:
>> On 07/13/2017 08:07 AM, Philippe Mathieu-Daudé wrote:
>>> On 04/07/2017 04:21 PM, Philippe Mathieu-Daudé wrote:
>>>> Hi Dave,
>>>>
>>>> On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote:
>>>>> Hi,
>>>>> Fedora 26 has gcc 7.0.1 which has the normal compliment
>>>>> of new fussy warnings; so far I've posted :
>>>>>
>>>>> tests/check-qdict: Fix missing brackets
>>>>> slirp/smb: Replace constant strings by glib string
>>>>>
>>>>> that fix one actual mistake and work around something it's being
>>>>> fussy over.
>>>>>
>>>>> But I've also got a pile of hacks, attached below that I'm
>>>>> not too sure what I'll do with them yet, but they're attached
>>>
>>> ping ... What do we do with them?
>>
>> Well, now that I've upgraded to the just-released Fedora 26, it is now
>> mainline gcc and affecting my builds. So we should really try and find
>> patches that silence the warnings (although it counts as bug fixes, so
>> it won't hurt if it doesn't make tomorrow's freeze deadline).
>
> FWIW, most of these have been fixed in the meantime; the only remaining
> hack I had to add was:
>
> diff --git i/hw/usb/bus.c w/hw/usb/bus.c
> index 5939b273b9..bce011058b 100644
> --- i/hw/usb/bus.c
> +++ w/hw/usb/bus.c
> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
> USBPort *ports[],
> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
> {
> if (upstream) {
> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
> - upstream->path, portnr);
> + int l = snprintf(downstream->path, sizeof(downstream->path),
> "%s.%d",
> + upstream->path, portnr);
> + assert(l < sizeof(downstream->path));
Do you really need an assert there, or will
(void)l; /* "used" */
work as well? You didn't mention what the reported error is, so I'm guessing.
r~
On 07/18/2017 06:59 PM, Richard Henderson wrote:
>> +++ w/hw/usb/bus.c
>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus,
>> USBPort *ports[],
>> void usb_port_location(USBPort *downstream, USBPort *upstream, int
>> portnr)
>> {
>> if (upstream) {
>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
>> - upstream->path, portnr);
>> + int l = snprintf(downstream->path, sizeof(downstream->path),
>> "%s.%d",
>> + upstream->path, portnr);
>> + assert(l < sizeof(downstream->path));
>
> Do you really need an assert there, or will
>
> (void)l; /* "used" */
>
> work as well? You didn't mention what the reported error is, so I'm
> guessing.
The original error is that gcc 7 complains that snprintf is prone to
buffer overflow if the input is unbounded. Adding the assert that we
KNOW the input is not unbounded is enough to shut up gcc, on Linux.
What was then drawn into question is whether assert still has that
property on mingw (since assert on mingw lacks the noreturn marking that
it has on Linux).
At this point, unless someone posts an actual failure of gcc 7 compiling
this code for mingw, I don't see why we have to change it; shutting up
the warning on Linux is good enough for the purpose of this patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20170407143847.GM2138@work-vm
Subject: [Qemu-devel] Hacks for building on gcc 7 / Fedora 26
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
ad00ac3 Hacks for building on gcc 7 / Fedora 26
=== OUTPUT BEGIN ===
Checking PATCH 1/1: Hacks for building on gcc 7 / Fedora 26...
WARNING: line over 80 characters
#63: FILE: block/blkverify.c:311:
+ s->test_file->bs->exact_filename), <, sizeof(bs->exact_filename));
WARNING: line over 80 characters
#85: FILE: hw/usb/bus.c:411:
+ g_assert_cmpint(snprintf(downstream->path, sizeof(downstream->path), "%s.%d",
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 2 warnings, 64 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On Fri, Apr 07, 2017 at 03:38:47PM +0100, Dr. David Alan Gilbert wrote:
> Hi,
> Fedora 26 has gcc 7.0.1 which has the normal compliment
> of new fussy warnings; so far I've posted :
>
> tests/check-qdict: Fix missing brackets
> slirp/smb: Replace constant strings by glib string
>
> that fix one actual mistake and work around something it's being
> fussy over.
>
> But I've also got a pile of hacks, attached below that I'm
> not too sure what I'll do with them yet, but they're attached
> for anyone else trying to build. Note they're smoke-only-tested.
>
> I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
> filed for what I reckon is a couple of overly pessimistic warnings.
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index bd9fd55b0a..ebb0221140 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -46,7 +46,7 @@ static inline size_t
> iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
> size_t offset, const void *buf, size_t bytes)
> {
> - if (__builtin_constant_p(bytes) && iov_cnt &&
> + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> memcpy(iov[0].iov_base + offset, buf, bytes);
> return bytes;
> @@ -59,7 +59,7 @@ static inline size_t
> iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> size_t offset, void *buf, size_t bytes)
> {
> - if (__builtin_constant_p(bytes) && iov_cnt &&
> + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> memcpy(buf, iov[0].iov_base + offset, bytes);
> return bytes;
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 88dbf97853..c55de4f65b 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -98,7 +98,7 @@ static void test_acpi_rsdt_table(test_data *data)
> AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> uint32_t addr = data->rsdp_table.rsdt_physical_address;
> uint32_t *tables;
> - int tables_nr;
> + unsigned int tables_nr;
> uint8_t checksum;
>
> /* read the header */
Unless I've missed a patch somwhere, I think the problems that these two
chunks are fixing are still needed for current git master, to stop warnings
in the unit tests.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 07/20/2017 05:50 AM, Daniel P. Berrange wrote:
> On Fri, Apr 07, 2017 at 03:38:47PM +0100, Dr. David Alan Gilbert wrote:
>> Hi,
>> Fedora 26 has gcc 7.0.1 which has the normal compliment
>> of new fussy warnings; so far I've posted :
>>
>> +++ b/include/qemu/iov.h
>> @@ -46,7 +46,7 @@ static inline size_t
>> iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
>> size_t offset, const void *buf, size_t bytes)
>> {
>> - if (__builtin_constant_p(bytes) && iov_cnt &&
>> + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
>> offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
>> memcpy(iov[0].iov_base + offset, buf, bytes);
>> return bytes;
> Unless I've missed a patch somwhere, I think the problems that these two
> chunks are fixing are still needed for current git master, to stop warnings
> in the unit tests.
Huh. I guess I'm not seeing warnings (aka -Werror failures) in those
spots there because I typically compile with -g instead of -O2 for
development. (It's annoying that the set of warnings issued by gcc
depends on your optimization levels, but such is life)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Fri, Apr 07, 2017 at 03:38:47PM +0100, Dr. David Alan Gilbert wrote:
> > Hi,
> > Fedora 26 has gcc 7.0.1 which has the normal compliment
> > of new fussy warnings; so far I've posted :
> >
> > tests/check-qdict: Fix missing brackets
> > slirp/smb: Replace constant strings by glib string
> >
> > that fix one actual mistake and work around something it's being
> > fussy over.
> >
> > But I've also got a pile of hacks, attached below that I'm
> > not too sure what I'll do with them yet, but they're attached
> > for anyone else trying to build. Note they're smoke-only-tested.
> >
> > I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
> > filed for what I reckon is a couple of overly pessimistic warnings.
>
>
> > diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> > index bd9fd55b0a..ebb0221140 100644
> > --- a/include/qemu/iov.h
> > +++ b/include/qemu/iov.h
> > @@ -46,7 +46,7 @@ static inline size_t
> > iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
> > size_t offset, const void *buf, size_t bytes)
> > {
> > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > memcpy(iov[0].iov_base + offset, buf, bytes);
> > return bytes;
> > @@ -59,7 +59,7 @@ static inline size_t
> > iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> > size_t offset, void *buf, size_t bytes)
> > {
> > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > memcpy(buf, iov[0].iov_base + offset, bytes);
> > return bytes;
tbh I don't know what the right fix for this is; the gcc discussion
confused me as to why it thinks it can be a valid case.
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index 88dbf97853..c55de4f65b 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -98,7 +98,7 @@ static void test_acpi_rsdt_table(test_data *data)
> > AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> > uint32_t addr = data->rsdp_table.rsdt_physical_address;
> > uint32_t *tables;
> > - int tables_nr;
> > + unsigned int tables_nr;
> > uint8_t checksum;
> >
> > /* read the header */
> Unless I've missed a patch somwhere, I think the problems that these two
> chunks are fixing are still needed for current git master, to stop warnings
> in the unit tests.
>
The second one of those I can post a patch for; the trick is to change
the g_assert_cmpint to a g_assert and then gcc realises the bad case
can't happen. I'll post that in a few mins.
Dave
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Jul 20, 2017 at 05:15:49PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Fri, Apr 07, 2017 at 03:38:47PM +0100, Dr. David Alan Gilbert wrote:
> > > Hi,
> > > Fedora 26 has gcc 7.0.1 which has the normal compliment
> > > of new fussy warnings; so far I've posted :
> > >
> > > tests/check-qdict: Fix missing brackets
> > > slirp/smb: Replace constant strings by glib string
> > >
> > > that fix one actual mistake and work around something it's being
> > > fussy over.
> > >
> > > But I've also got a pile of hacks, attached below that I'm
> > > not too sure what I'll do with them yet, but they're attached
> > > for anyone else trying to build. Note they're smoke-only-tested.
> > >
> > > I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
> > > filed for what I reckon is a couple of overly pessimistic warnings.
> >
> >
> > > diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> > > index bd9fd55b0a..ebb0221140 100644
> > > --- a/include/qemu/iov.h
> > > +++ b/include/qemu/iov.h
> > > @@ -46,7 +46,7 @@ static inline size_t
> > > iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
> > > size_t offset, const void *buf, size_t bytes)
> > > {
> > > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > > memcpy(iov[0].iov_base + offset, buf, bytes);
> > > return bytes;
> > > @@ -59,7 +59,7 @@ static inline size_t
> > > iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> > > size_t offset, void *buf, size_t bytes)
> > > {
> > > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > > memcpy(buf, iov[0].iov_base + offset, bytes);
> > > return bytes;
>
> tbh I don't know what the right fix for this is; the gcc discussion
> confused me as to why it thinks it can be a valid case.
Even if gcc is broken in issuing a warning here, we still need to
make it quiet so people on F26 and similarly new distros can build
without warnings.
IMHO your patch is ok, or we could be alittle more explicit about
catching just the case where you pass -1 for bytes, and have
&& bytes != -1
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Thu, Jul 20, 2017 at 05:15:49PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > On Fri, Apr 07, 2017 at 03:38:47PM +0100, Dr. David Alan Gilbert wrote:
> > > > Hi,
> > > > Fedora 26 has gcc 7.0.1 which has the normal compliment
> > > > of new fussy warnings; so far I've posted :
> > > >
> > > > tests/check-qdict: Fix missing brackets
> > > > slirp/smb: Replace constant strings by glib string
> > > >
> > > > that fix one actual mistake and work around something it's being
> > > > fussy over.
> > > >
> > > > But I've also got a pile of hacks, attached below that I'm
> > > > not too sure what I'll do with them yet, but they're attached
> > > > for anyone else trying to build. Note they're smoke-only-tested.
> > > >
> > > > I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
> > > > filed for what I reckon is a couple of overly pessimistic warnings.
> > >
> > >
> > > > diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> > > > index bd9fd55b0a..ebb0221140 100644
> > > > --- a/include/qemu/iov.h
> > > > +++ b/include/qemu/iov.h
> > > > @@ -46,7 +46,7 @@ static inline size_t
> > > > iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
> > > > size_t offset, const void *buf, size_t bytes)
> > > > {
> > > > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > > > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > > > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > > > memcpy(iov[0].iov_base + offset, buf, bytes);
> > > > return bytes;
> > > > @@ -59,7 +59,7 @@ static inline size_t
> > > > iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> > > > size_t offset, void *buf, size_t bytes)
> > > > {
> > > > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > > > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > > > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > > > memcpy(buf, iov[0].iov_base + offset, bytes);
> > > > return bytes;
> >
> > tbh I don't know what the right fix for this is; the gcc discussion
> > confused me as to why it thinks it can be a valid case.
>
> Even if gcc is broken in issuing a warning here, we still need to
> make it quiet so people on F26 and similarly new distros can build
> without warnings.
I agree.
> IMHO your patch is ok, or we could be alittle more explicit about
> catching just the case where you pass -1 for bytes, and have
>
> && bytes != -1
This seems bizarre to me since bytes is size_t bytes and size_t
is unsigned, so I'd have sympathy for a compiler that warned that
bytes != -1 was always true.
Dave
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Jul 20, 2017 at 06:04:44PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Thu, Jul 20, 2017 at 05:15:49PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > On Fri, Apr 07, 2017 at 03:38:47PM +0100, Dr. David Alan Gilbert wrote:
> > > > > Hi,
> > > > > Fedora 26 has gcc 7.0.1 which has the normal compliment
> > > > > of new fussy warnings; so far I've posted :
> > > > >
> > > > > tests/check-qdict: Fix missing brackets
> > > > > slirp/smb: Replace constant strings by glib string
> > > > >
> > > > > that fix one actual mistake and work around something it's being
> > > > > fussy over.
> > > > >
> > > > > But I've also got a pile of hacks, attached below that I'm
> > > > > not too sure what I'll do with them yet, but they're attached
> > > > > for anyone else trying to build. Note they're smoke-only-tested.
> > > > >
> > > > > I also have gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80346
> > > > > filed for what I reckon is a couple of overly pessimistic warnings.
> > > >
> > > >
> > > > > diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> > > > > index bd9fd55b0a..ebb0221140 100644
> > > > > --- a/include/qemu/iov.h
> > > > > +++ b/include/qemu/iov.h
> > > > > @@ -46,7 +46,7 @@ static inline size_t
> > > > > iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
> > > > > size_t offset, const void *buf, size_t bytes)
> > > > > {
> > > > > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > > > > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > > > > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > > > > memcpy(iov[0].iov_base + offset, buf, bytes);
> > > > > return bytes;
> > > > > @@ -59,7 +59,7 @@ static inline size_t
> > > > > iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> > > > > size_t offset, void *buf, size_t bytes)
> > > > > {
> > > > > - if (__builtin_constant_p(bytes) && iov_cnt &&
> > > > > + if (__builtin_constant_p(bytes) && iov_cnt && bytes <= INT_MAX &&
> > > > > offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
> > > > > memcpy(buf, iov[0].iov_base + offset, bytes);
> > > > > return bytes;
> > >
> > > tbh I don't know what the right fix for this is; the gcc discussion
> > > confused me as to why it thinks it can be a valid case.
> >
> > Even if gcc is broken in issuing a warning here, we still need to
> > make it quiet so people on F26 and similarly new distros can build
> > without warnings.
>
> I agree.
>
> > IMHO your patch is ok, or we could be alittle more explicit about
> > catching just the case where you pass -1 for bytes, and have
> >
> > && bytes != -1
>
> This seems bizarre to me since bytes is size_t bytes and size_t
> is unsigned, so I'd have sympathy for a compiler that warned that
> bytes != -1 was always true.
Could be paranoid and do "&& bytes != (size_t)-1"
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 07/20/2017 12:06 PM, Daniel P. Berrange wrote: >>> IMHO your patch is ok, or we could be alittle more explicit about >>> catching just the case where you pass -1 for bytes, and have >>> >>> && bytes != -1 >> >> This seems bizarre to me since bytes is size_t bytes and size_t >> is unsigned, so I'd have sympathy for a compiler that warned that >> bytes != -1 was always true. > > Could be paranoid and do "&& bytes != (size_t)-1" Any compiler that treats 'bytes != -1' differently from 'bytes != (size_t)-1' is broken, since those two are equivalent per C99 promotion rules (unsigned op signed produces unsigned, and conversion of signed to unsigned is well-defined). I prefer the form without a cast. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.