| Message ID | 1431635892-31996-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Under Review |
| Headers | show |
On 05/14/2015 02:38 PM, Stefan Weil wrote: > i686-w64-mingw32-gcc 4.9.1 from Debian Jessie complains: > > hw/pci/pci.c:938:29: warning: > array subscript is above array bounds [-Warray-bounds] > > Using g_assert instead of assert fixes this warning. Is that because the mingw headers don't properly mark the expansion of the failed branch of assert() as noreturn, whereas g_assert() does, and therefore the compiler has more information about what variables must be if the rest of the function is reached? > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > hw/pci/pci.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 48f19a3..34f71dc 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -927,8 +927,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > uint64_t wmask; > pcibus_t size = memory_region_size(memory); > > - assert(region_num >= 0); > - assert(region_num < PCI_NUM_REGIONS); > + g_assert(region_num >= 0); > + g_assert(region_num < PCI_NUM_REGIONS); I must say, this is the strangest way I've ever heard of silencing a compiler warning. But if it works and my uneducated guess above about why is correct, Reviewed-by: Eric Blake <eblake@redhat.com> Meanwhile, you may want to file a bug to the mingw maintainers that their header is puny when compared to glibc assert() or to glib's g_assert, when it comes to giving gcc decent hints.
On 05/14/2015 03:06 PM, Eric Blake wrote: > On 05/14/2015 02:38 PM, Stefan Weil wrote: >> i686-w64-mingw32-gcc 4.9.1 from Debian Jessie complains: >> >> hw/pci/pci.c:938:29: warning: >> array subscript is above array bounds [-Warray-bounds] >> >> Using g_assert instead of assert fixes this warning. > > Is that because the mingw headers don't properly mark the expansion of > the failed branch of assert() as noreturn, whereas g_assert() does, and > therefore the compiler has more information about what variables must be > if the rest of the function is reached? > > > Meanwhile, you may want to file a bug to the mingw maintainers that > their header is puny when compared to glibc assert() or to glib's > g_assert, when it comes to giving gcc decent hints. Oh, I was right! glibc /usr/include/assert.h: extern void __assert_fail (const char *__assertion, const char *__file, unsigned int __line, const char *__function) __THROW __attribute__ ((__noreturn__)); # define assert(expr) \ ((expr) \ ? __ASSERT_VOID_CAST (0) \ : __assert_fail (__STRING(expr), __FILE__, __LINE__, __ASSERT_FUNCTION)) vs mingw /usr/i686-w64-mingw32/sys-root/mingw/include/assert.h: extern void __cdecl _assert (const char *_Message, const char *_File, unsigned _Line); #define assert(_Expression) \ (void) \ ((!!(_Expression)) || \ (_assert(#_Expression,__FILE__,__LINE__),0)) even though mingw has in that same file: void __cdecl _Exit(int) __MINGW_ATTRIB_NORETURN; so it is indeed a bug in the mingw headers that __MINGW_ATTRIB_NORETURN was not attached to _assert's declaration.
On 05/14/2015 03:16 PM, Eric Blake wrote: >> Meanwhile, you may want to file a bug to the mingw maintainers that >> their header is puny when compared to glibc assert() or to glib's >> g_assert, when it comes to giving gcc decent hints. > > Oh, I was right! > > so it is indeed a bug in the mingw headers that __MINGW_ATTRIB_NORETURN > was not attached to _assert's declaration. Filed a Fedora bug at https://bugzilla.redhat.com/show_bug.cgi?id=1221811 but of course it will take a while for fixed headers to propagate to all distros, so this patch is fine for qemu in the meantime.
On 05/14/2015 03:16 PM, Eric Blake wrote: > On 05/14/2015 03:06 PM, Eric Blake wrote: >> On 05/14/2015 02:38 PM, Stefan Weil wrote: >>> i686-w64-mingw32-gcc 4.9.1 from Debian Jessie complains: >>> >>> hw/pci/pci.c:938:29: warning: >>> array subscript is above array bounds [-Warray-bounds] >>> >>> Using g_assert instead of assert fixes this warning. >> >> Is that because the mingw headers don't properly mark the expansion of >> the failed branch of assert() as noreturn, whereas g_assert() does, and >> therefore the compiler has more information about what variables must be >> if the rest of the function is reached? >> > >> >> Meanwhile, you may want to file a bug to the mingw maintainers that >> their header is puny when compared to glibc assert() or to glib's >> g_assert, when it comes to giving gcc decent hints. > > Oh, I was right! > > glibc /usr/include/assert.h: > vs mingw /usr/i686-w64-mingw32/sys-root/mingw/include/assert.h: and glib's version in /usr/include/glib-2.0/glib/gtestutils.h: #define g_assert(expr) G_STMT_START { \ if G_LIKELY (expr) ; else \ g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \ #expr); \ } G_STMT_END GLIB_AVAILABLE_IN_ALL void g_assertion_message_expr (const char *domain, const char *file, int line, const char *func, const char *expr) G_GNUC_NORETURN;
Stefan Weil <sw@weilnetz.de> writes: > i686-w64-mingw32-gcc 4.9.1 from Debian Jessie complains: > > hw/pci/pci.c:938:29: warning: > array subscript is above array bounds [-Warray-bounds] > > Using g_assert instead of assert fixes this warning. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > hw/pci/pci.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 48f19a3..34f71dc 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -927,8 +927,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > uint64_t wmask; > pcibus_t size = memory_region_size(memory); > > - assert(region_num >= 0); > - assert(region_num < PCI_NUM_REGIONS); > + g_assert(region_num >= 0); > + g_assert(region_num < PCI_NUM_REGIONS); > if (size & (size-1)) { > fprintf(stderr, "ERROR: PCI region size must be pow2 " > "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size); This is obviously a bug in that version of MinGW. Have you reported it? Do we really want to work around it?
On 05/15/2015 02:00 AM, Markus Armbruster wrote: > Stefan Weil <sw@weilnetz.de> writes: > >> i686-w64-mingw32-gcc 4.9.1 from Debian Jessie complains: >> > > This is obviously a bug in that version of MinGW. Have you reported it? I don't know if Debian has a bug filed against mingw yet, but I mentioned elsewhere in the thread that I filed a bug for Fedora which is also impacted. > Do we really want to work around it? That, I'm not sure. It takes a while for updated mingw headers to propagate, and the fix is small enough that it won't hurt to leave the fix in place even after updated headers are available. But since the problem is a missing attribute in the header causing a compiler warning, it could be worked around by turning off compiler warnings until a new header is present.
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 48f19a3..34f71dc 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -927,8 +927,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, uint64_t wmask; pcibus_t size = memory_region_size(memory); - assert(region_num >= 0); - assert(region_num < PCI_NUM_REGIONS); + g_assert(region_num >= 0); + g_assert(region_num < PCI_NUM_REGIONS); if (size & (size-1)) { fprintf(stderr, "ERROR: PCI region size must be pow2 " "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size);
i686-w64-mingw32-gcc 4.9.1 from Debian Jessie complains: hw/pci/pci.c:938:29: warning: array subscript is above array bounds [-Warray-bounds] Using g_assert instead of assert fixes this warning. Signed-off-by: Stefan Weil <sw@weilnetz.de> --- hw/pci/pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)