| Message ID | 20170407143847.GM2138@work-vm |
|---|---|
| State | New |
| Headers | show |
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.
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 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).
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
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)
* 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
* 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
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.
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 */
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(-)