Re: [PULL 00/10] Bitmaps patches
On 3/17/20 9:00 AM, Peter Maydell wrote: block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400) Pull request --- Hi; this fails to compile with clang: As pointed out here, my recommendation is for John to send a v2 pull request with one more patch added: https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg05969.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PULL 00/10] Bitmaps patches
On 3/17/20 10:11 AM, Daniel P. Berrangé wrote: On Tue, Mar 17, 2020 at 03:07:34PM +, Peter Maydell wrote: On Tue, 17 Mar 2020 at 15:05, Daniel P. Berrangé wrote: On Tue, Mar 17, 2020 at 03:00:48PM +, Peter Maydell wrote: On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé wrote: I don't feel like -Wno-unused-function looses anything significant, as the GCC builds will still be reporting unused functions which will catch majority of cases. The most interesting difference is that clang will catch unused static inline functions which gcc does not. That's mostly just about dead code cruft detection IIUC. That code won't make it into the binary if it isn't used. Indeed, but it's nice to have the dead code cruft detection. You can always mark the function as __attribute__((unused)) if you really mean that it might be present but not used. The *BSDs seem to track latest glib pretty quickly. So if we got the unused attribute into upstream glib, we would probably have about 6-9 months before we get a build platform with the fixed glib included where we can conditionally re-enable the unused-function warning. Looks like glib commit https://github.com/GNOME/glib/commit/b41bff1f (2.57.2) added G_GNUC_UNUSED to all of the functions declared during G_DEFINE_AUTOPTR_CLEANUP_FUNC. Which version of glib is on the NetBSD machine? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PULL 00/10] Bitmaps patches
On Tue, Mar 17, 2020 at 03:07:34PM +, Peter Maydell wrote: > On Tue, 17 Mar 2020 at 15:05, Daniel P. Berrangé wrote: > > > > On Tue, Mar 17, 2020 at 03:00:48PM +, Peter Maydell wrote: > > > On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé > > > wrote: > > > > I don't feel like -Wno-unused-function looses anything significant, as > > > > the GCC builds will still be reporting unused functions which will > > > > catch majority of cases. > > > > > > The most interesting difference is that clang will catch unused > > > static inline functions which gcc does not. > > > > That's mostly just about dead code cruft detection IIUC. That code won't > > make it into the binary if it isn't used. > > Indeed, but it's nice to have the dead code cruft detection. You > can always mark the function as __attribute__((unused)) if you really > mean that it might be present but not used. The *BSDs seem to track latest glib pretty quickly. So if we got the unused attribute into upstream glib, we would probably have about 6-9 months before we get a build platform with the fixed glib included where we can conditionally re-enable the unused-function warning. 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 :|
Re: [PULL 00/10] Bitmaps patches
On Tue, 17 Mar 2020 at 15:05, Daniel P. Berrangé wrote: > > On Tue, Mar 17, 2020 at 03:00:48PM +, Peter Maydell wrote: > > On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé > > wrote: > > > I don't feel like -Wno-unused-function looses anything significant, as > > > the GCC builds will still be reporting unused functions which will > > > catch majority of cases. > > > > The most interesting difference is that clang will catch unused > > static inline functions which gcc does not. > > That's mostly just about dead code cruft detection IIUC. That code won't > make it into the binary if it isn't used. Indeed, but it's nice to have the dead code cruft detection. You can always mark the function as __attribute__((unused)) if you really mean that it might be present but not used. thanks -- PMM
Re: [PULL 00/10] Bitmaps patches
On Tue, Mar 17, 2020 at 03:00:48PM +, Peter Maydell wrote: > On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé wrote: > > I don't feel like -Wno-unused-function looses anything significant, as > > the GCC builds will still be reporting unused functions which will > > catch majority of cases. > > The most interesting difference is that clang will catch unused > static inline functions which gcc does not. That's mostly just about dead code cruft detection IIUC. That code won't make it into the binary if it isn't used. 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 :|
Re: [PULL 00/10] Bitmaps patches
On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé wrote: > I don't feel like -Wno-unused-function looses anything significant, as > the GCC builds will still be reporting unused functions which will > catch majority of cases. The most interesting difference is that clang will catch unused static inline functions which gcc does not. thanks -- PMM
Re: [PULL 00/10] Bitmaps patches
On Tue, Mar 17, 2020 at 09:40:00AM -0500, Eric Blake wrote:
> On 3/17/20 9:00 AM, Peter Maydell wrote:
> > On Tue, 17 Mar 2020 at 04:38, John Snow wrote:
>
> > >
> > >block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12
> > > 16:36:46 -0400)
> > >
> > >
> > > Pull request
> > >
> > > ---
> >
> > Hi; this fails to compile with clang:
> >
> > /home/petmay01/linaro/qemu-for-merges/nbd/server.c:1937:1: error:
> > unused function 'glib_listautoptr_cleanup_NBDExtentArray'
> > [-Werror,-Wunused-function]
> > G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
> > ^
> > /usr/include/glib-2.0/glib/gmacros.h:462:22: note: expanded from macro
> > 'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
> >static inline void _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList
> > **_l) { g_list_free_full (*_l, (GDestroyNotify) func); } \
> > ^
> > /usr/include/glib-2.0/glib/gmacros.h:443:48: note: expanded from macro
> > '_GLIB_AUTOPTR_LIST_FUNC_NAME'
> > #define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName)
> > glib_listautoptr_cleanup_##TypeName
> > ^
> > :49:1: note: expanded from here
> > glib_listautoptr_cleanup_NBDExtentArray
> > ^
>
> Should we add -Wno-unused-function to CFLAGS when dealing with a version of
> clang that complains about that version of glib's headers? Is it fixed in a
> newer version of glib, where we could just backport a newer definition of
> G_DEFINE_AUTOPTR_CLEANUP_FUNC() that adds whatever annotations are needed to
> shut the compiler up?
>
> On IRC, danpb pointed me to libvirt's solution:
> https://libvirt.org/git/?p=libvirt.git;a=commit;h=44e7f029
>
> Maybe we just write our own macro wrapper around
> G_DEFINE_AUTOPTR_CLEANUP_FUNC which takes care of adding necessary
> annotations and use that instead (and our macro name might be shorter...)
My preference is to stick with regular glib functions/macros whereever
practical, rather than inventing QEMU replacements which add a knowledge
burden for contributors. That's why we moved to -Wno-unused-function in
libvirt.
I don't feel like -Wno-unused-function looses anything significant, as
the GCC builds will still be reporting unused functions which will
catch majority of cases.
Possibly we could figure out a patch for glib upstream that uses pragma
push/pop to squelch the warning ? They are quite receptive to patches
IME.
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 :|
Re: [PULL 00/10] Bitmaps patches
On 3/17/20 9:00 AM, Peter Maydell wrote:
On Tue, 17 Mar 2020 at 04:38, John Snow wrote:
block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46
-0400)
Pull request
---
Hi; this fails to compile with clang:
/home/petmay01/linaro/qemu-for-merges/nbd/server.c:1937:1: error:
unused function 'glib_listautoptr_cleanup_NBDExtentArray'
[-Werror,-Wunused-function]
G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
^
/usr/include/glib-2.0/glib/gmacros.h:462:22: note: expanded from macro
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
static inline void _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList
**_l) { g_list_free_full (*_l, (GDestroyNotify) func); } \
^
/usr/include/glib-2.0/glib/gmacros.h:443:48: note: expanded from macro
'_GLIB_AUTOPTR_LIST_FUNC_NAME'
#define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName)
glib_listautoptr_cleanup_##TypeName
^
:49:1: note: expanded from here
glib_listautoptr_cleanup_NBDExtentArray
^
Should we add -Wno-unused-function to CFLAGS when dealing with a version
of clang that complains about that version of glib's headers? Is it
fixed in a newer version of glib, where we could just backport a newer
definition of G_DEFINE_AUTOPTR_CLEANUP_FUNC() that adds whatever
annotations are needed to shut the compiler up?
On IRC, danpb pointed me to libvirt's solution:
https://libvirt.org/git/?p=libvirt.git;a=commit;h=44e7f029
Maybe we just write our own macro wrapper around
G_DEFINE_AUTOPTR_CLEANUP_FUNC which takes care of adding necessary
annotations and use that instead (and our macro name might be shorter...)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PULL 00/10] Bitmaps patches
On Tue, 17 Mar 2020 at 04:38, John Snow wrote:
>
> The following changes since commit 6e8a73e911f066527e775e04b98f31ebd19db600:
>
> Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request'
> into staging (2020-03-11 14:41:27 +)
>
> are available in the Git repository at:
>
> https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to 34b456d485a4df3a88116fb5ef0c418f2f12990d:
>
> block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46
> -0400)
>
>
> Pull request
>
> ---
Hi; this fails to compile with clang:
/home/petmay01/linaro/qemu-for-merges/nbd/server.c:1937:1: error:
unused function 'glib_listautoptr_cleanup_NBDExtentArray'
[-Werror,-Wunused-function]
G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
^
/usr/include/glib-2.0/glib/gmacros.h:462:22: note: expanded from macro
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
static inline void _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList
**_l) { g_list_free_full (*_l, (GDestroyNotify) func); } \
^
/usr/include/glib-2.0/glib/gmacros.h:443:48: note: expanded from macro
'_GLIB_AUTOPTR_LIST_FUNC_NAME'
#define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName)
glib_listautoptr_cleanup_##TypeName
^
:49:1: note: expanded from here
glib_listautoptr_cleanup_NBDExtentArray
^
thanks
-- PMM
[PULL 00/10] Bitmaps patches
The following changes since commit 6e8a73e911f066527e775e04b98f31ebd19db600: Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2020-03-11 14:41:27 +) are available in the Git repository at: https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request for you to fetch changes up to 34b456d485a4df3a88116fb5ef0c418f2f12990d: block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400) Pull request Vladimir Sementsov-Ogievskiy (10): hbitmap: assert that we don't create bitmap larger than INT64_MAX hbitmap: move hbitmap_iter_next_word to hbitmap.c hbitmap: unpublish hbitmap_iter_skip_words hbitmap: drop meta bitmaps as they are unused block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t block/dirty-bitmap: add _next_dirty API block/dirty-bitmap: improve _next_dirty_area API nbd/server: introduce NBDExtentArray nbd/server: use bdrv_dirty_bitmap_next_dirty_area block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty include/block/dirty-bitmap.h | 9 +- include/qemu/hbitmap.h | 95 +++ block/dirty-bitmap.c | 16 +- block/qcow2-bitmap.c | 15 +- nbd/server.c | 251 ++-- tests/test-hbitmap.c | 316 +-- util/hbitmap.c | 134 +-- 7 files changed, 375 insertions(+), 461 deletions(-) -- 2.21.1
