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.
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/ :|