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. The other for use in public headers, where the public gets good documentation that passing NULL is nonsense, and even gets the attribute enabled if they compile with gcc in order to help them avoid bad code. But when compiling internally, we disable the macro (similarly to how our headers use VIR_ENUM_SENTINELS to control whether the VIR_*_LAST enum members are present for internal use but absent for the public); that way, our implementations don't see the attribute and can code in the safety-net runtime check to handle bad users that disobeyed the warning, without crashing and without our NULL checks being optimized out. I'm still not sold that we want -fdelete-null-pointer-checks, but on the other hand, how much overhead does it add? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
