[libvirt] [PATCH (RFC?)] Remove usage of __attribute__((nonnull))

[Daniel P. Berrange]

On Wed, Apr 05, 2017 at 12:38:23PM +0200, Martin Kletzander wrote:
> On Wed, Apr 05, 2017 at 10:06:22AM +0100, Daniel P. Berrange wrote:
> > On Tue, Apr 04, 2017 at 09:51:47PM +0200, Martin Kletzander wrote:
> > > On Tue, Apr 04, 2017 at 04:10:59PM +0100, Daniel P. Berrange wrote:
> > > > On Tue, Mar 28, 2017 at 01:46:31PM +0200, Martin Kletzander wrote:
> > > > > The attribute (defined as ATTRIBUTE_NONNULL) was added long time
> > > > > ago (2009), but in 2012 (commit eefb881d4683) it was disabled for
> > > > > normal build, making it used only when coverity was building libvirt
> > > > > or on special request.  It was disabled because it was being misused
> > > > > and misunderstood -- the attribute is there as an optimization hint
> > > > > for the compiler, not to enhance static analysis.
> > > >
> > > > Actually the attribute does both and the primary intention of the attribute
> > > > *is* build time warnings and/or static analysis warnings:
> > > >
> > > > [quote]
> > > >  'nonnull (ARG-INDEX, ...)'
> > > >     The 'nonnull' attribute specifies that some function parameters
> > > >     should be non-null pointers.  For instance, the declaration:
> > > >
> > > >          extern void *
> > > >          my_memcpy (void *dest, const void *src, size_t len)
> > > >                  __attribute__((nonnull (1, 2)));
> > > >
> > > >     causes the compiler to check that, in calls to 'my_memcpy',
> > > >     arguments DEST and SRC are non-null.  If the compiler determines
> > > 
> > > This, however, happens only if we pass NULL directly.  There's not much
> > > of a deeper analysis involved IIRC.
> > 
> > Yep, coverity is needed to do most deeper analysis of NULL handling
> > 
> > > > The use of nonnull attribute would help the compiler report
> > > > mistakes, but the compiler only catches some easy cases at build
> > > > time.
> > > >
> > > 
> > > It would.  But for now it is only enabled if STATIC_ANALYSIS=1 and that
> > > is only set if you are running in coverity (or explicitly specify that
> > > during configure, which almost nobody does).
> > 
> > Easily solvable by enabling it in CI.
> > 
> 
> How about we enable it by default for build with new enough GCC (so that
> it reports these errors without silently dropping the non-NULL checks)?

Yes, it seems that we can use -fno-delete-null-pointer-checks to prevent
GCC from optimizing away the NULL pointer checks, while having nonnull
attributes enabled all the time.


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