Re: [libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]
On Thu, Jan 10, 2019 at 10:23:13AM -0600, Eric Blake wrote:
On 1/10/19 9:39 AM, Daniel P. Berrangé wrote:
> For any pointer parameter there's then two possibilities. Either NULL
> is a valid value and results in some useful behaviour, or NULL is not
> valid and libvirt will catch that and report an error.
But within the latter are two cases: libvirt catches the error
gracefully, or libvirt crashes.
>
> The latter are essentially all programmer bugs, but we wanted to be robust
> and catch them at runtime & report graceful error instead of referencing
> NULL or assert()ing. We added ATTRIBUTE_NONNULL widely to try to catch the
> latter at compile time too. This also served as documentation in the header
> files to indicate that you shouldn't be passing NULL for that parameter.
>
> We later discovered our usage of ATTRIBUTE_NONNULL was based on a
> mis-understanding of the functional impact of that annotation, and so
> #defined it to a no-op, so it merely served as documentation that you
> should not pass NULL to a method.
>
> It is still desirable for us to find a way to do all three tasks
>
> 1. document that NULL should not be passed for a param
> 2. detect mistakes passing NULL via static analysis
> 3. retain runtime graceful NULL param checking.
>
> AFAICT we have a choice either
>
> a. Replace ATTRIBUTE_NONNULL to just a plain comment /* nonnull */
> against each parameter. This covers items 1 & 2, mostly as
> we do today, while stopping coverity getting upset
>
> b. Re-enable ATTRIBUTE_NONNULL to expand to attribute(nonnull)
> but only under modern GCC, and add -fdelete-null-pointer-checks
> This covers items 1, 2 & 3, under GCC, while stopping
> coverity getting upset
Or go with two separate macros:
One for use on internal files, where when we mark something non-NULL, it
is a programming bug in libvirt for the caller for passing NULL, or a
programming bug in the implementation for using the attribute and then
checking for NULL after all. Let gcc/coverity always flag these
violations as bugs, and don't bother worrying about safety nets because
we trust our callers of internal interfaces for knowing the semantics.
I think this is the key point. I do *not* trust us to know the semantics
of our internal interfaces. Developers are fallible, so while we can
expect we'll get it right 95% of the time, we're inevitably going to
pass NULL to methods which don't want it. We must have the runtime
safety net to catch this gracefully, and ideally have static checks
to catch some of it at build time (though the latter is never going
to be perfect as it needs non-trivial control-flow data analysis).
I'm still not sold that we want -fdelete-null-pointer-checks, but
on the
other hand, how much overhead does it add?
It disables constant folding optimizations, and stops gcc from
deleting "useless" null pointer checks. It isn't doing instrumentation
like the -fsanitize args do, so I don't think it should have significant
overhead.
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 :|