[libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Erik Skultety]

On Tue, Jan 10, 2017 at 01:55:02PM +0000, Daniel P. Berrange wrote:
> On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
> > On Tue, Jan 10, 2017 at 10:00:31AM +0000, Daniel P. Berrange wrote:
> > > On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote:
> > > > On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:
> > > > > On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
> > > > > > On Mon, Jan 09, 2017 at 04:58:49PM +0000, Daniel P. Berrange wrote:
> > > > > > > For those who don't already know, GCC and CLang both implement a C language
> > > > > > > extension that enables automatic free'ing of resources when variables go
> > > > > > > out of scope. This is done by annotating the variable with the "cleanup"
> > > > > > > attribute, pointing to a function the compiler will wire up a call to when
> > > > > > > unwinding the stack. Since the annotation points to an arbitrary user
> > > > > > > defined function, you're not limited to simple free() like semantics. The
> > > > > > > cleanup function could unlock a mutex, or decrement a reference count, etc
> > > > > > >
> > > > > > > This annotation is used extensively by systemd, and libguestfs, amongst
> > > > > > > other projects. This obviously doesn't bring full garbage collection to
> > > > > > > C, but it does enable the code to be simplified. By removing the need to
> > > > > > > put in many free() (or equiv) calls to cleanup state, the "interesting"
> > > > > > > logic in the code stands out more, not being obscured by cleanup calls
> > > > > > > and goto jumps.
> > > > > > >
> > > > > > > I'm wondering what people think of making use of this in libvirt ?
> > > > > > >
> > > > > > > To my mind the only real reason to *not* use it, would be to maintain
> > > > > > > code portability to non-GCC/non-CLang compilers. OS-X, *BSD and *Linux
> > > > > > > all use GCC or CLang or both, so its a non-issue there. So the only place
> > > > > > > this could cause pain is people building libvirt on Win32, who are using
> > > > > > > the Microsoft compilers instead og GCC.
> > > > > > >
> > > > 
> > > > Only reason I see for not using it is the "temporary" mess it will
> > > > cause.  Yes, we can change to that incrementally, but it will take some
> > > > time and effort and it will never be all of the code that uses it.
> > > > Don't get me wrong, I would love using more builtin compiler features
> > > > and shortening the code here and there.  I'm just worried this
> > > > particular one might be more disrupting than useful.  Most of us are
> > > > pretty used to the code flow we already have and there's nothing you
> > > > can't achieve without the cleanup attribute.
> > > > 
> > > > And yes, I used quotation marks around the word temporary intentionally.
> > > 
> > > Yes, that's why I thought of it as something that would make for a GSoc
> > > project - have someone do a full conversion of particular areas of code.
> > > eg convert all of util/ or convert the domain XML parser, etc. Basically
> > > if we did it, I think we'd want to have entire files converted at once.
> > > Only converting individual methods ad-hoc would be quite messy.
> > > 
> > 
> > Yes, I know, but that still doesn't mean all will be converted,
> > unfortunately.
> > 
> > > > > > > IMHO, it is perfectly valid for us to declare that MSVC is unsupported
> > > > > > > with Libvirt and users must use GCC to build on Windows, either natively
> > > > > > > via cygwin, or cross-build from Linux hosts.
> > > > 
> > > > I would love to know if anyone actually tried doing that lately.  Given
> > > > how often we are broken with mingw and we only foind out thanks to our
> > > > test suite (and sometomes the fixing takes more than a release cycle), I
> > > > think nobody does that and from what I know, it might not even work.
> > > 
> > > We have mingw in the CI system for a while now and its generally fixed
> > > as quickly as native arch builds are fixed these days.
> > > 
> > 
> > Yes.  Now.  But there was a build-breaker for several months that nobody
> > cared about.  Pity the builds are truncated so I can't track it back
> > properly.  My point is that I don't remember anyone asking about it
> > during the whole time, just us trying to come up with fixes.
> > 
> > > 
> > > > > > (2) You must not write code like:
> > > > > >
> > > > > >    fn ()
> > > > > >    {
> > > > > >      CLEANUP_FREE char *v; // uninitialized
> > > > > >
> > > > > >      if (some error condition) {
> > > > > >        return -1;
> > > > > >      }
> > > > > >      ...
> > > > > >    }
> > > > > >
> > > > > > because that will call free (v) on the uninitialized variable.
> > > > > > Sometimes GCC can spot this.  In libguestfs we tend to initialize
> > > > > > every CLEANUP_* variable to either an explicit value or NULL.  GCC
> > > > > > optimizes away calls to free (NULL).
> > > > >
> > > > 
> > > > I'm trying to initialize all variables, always, so I don't see this as a
> > > > problem, but there are some of us that (I have the feeling) are trying
> > > > to initialize as few as possible, so this (although it's a different
> > > > story) might still be a problem for someone.
> > > 
> > > We pretty much have the same problem already with 'goto cleanup' - you
> > > have to make sure everything is initialized sanely before the first
> > > "goto cleanup". So I think we're safe in this respect already and
> > > the cleanup attributes wouldn't make it any more complex.
> > > 
> > 
> > Yeah, but with __attribute__((cleanup)), you need to make sure
> > everything is properly initialized immediatelly as opposed to before
> > first cleanup.  I know it sounds easy, and it is.  And I love doing that
> > even without __attribute__((cleanup)), I just see the potential for
> > error.  Hopefully we'd be able to do a syntax-check rule for checking
> > uninitialized variables with __attribute__((cleanup)).
> 
> Yeah, it'd be easy to write a rule to manadate NULL initialization
> for all pointers vars with cleanup attached.
> 
> Personally, I'm in favour of explicit initialization of every single
> stack variable no matter whether it happens to be required by the
> current code structure or not.

Yes, ^^^this would be absolutely great.

Erik

> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list