Feel free to make up a patch, test, and commit. Seems
a tiny bit like bloat to me if we don't think it's useful,
though.

Created attachment 55004 [details] [review]
Patch for nonnull attribute

This patch simply gives the __attribute__ ((__nonnull__)) support in GCC. I
don't see any way that glib could generically support the syntax which allows
specifying explicit arguments which are nonnull. This is because doing that
would require varargs macros and I am not aware of any way that
G_GNUC_NON_NULL(1, 2) (for example) could be left at the end of a function on a
platform without varargs macros.

If glib ever moves to require a compiler that supports either ISO or GNUC
varargs macros, then this patch could be modified to support the explicit
argument syntax safely.

the current non-null/null-sentinel attributes that gcc supports are not quite what would be useful for most of glib's API. the issue has been raised to the gcc team here as part of bug 344388:
 http://gcc.gnu.org/ml/gcc/2006-06/msg00338.html

*** Bug 594991 has been marked as a duplicate of this bug. ***

Note that introspection annotations has the inverse of this: (allow-none)

Created attachment 261437 [details] [review]
gmacros: Add a G_GNUC_NON_NULL attribute macro

This expands to the nonnull GCC attribute (if supported).

http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bnonnull_007d-function-attribute-2825

It requires some level of support for varargs macros from the
preprocessor, which it’s assumed any modern compiler suite supports.

Created attachment 261438 [details] [review]
gbytes: Add G_GNUC_NON_NULL attributes

Comment on attachment 261437 [details] [review]
gmacros: Add a G_GNUC_NON_NULL attribute macro

>It requires some level of support for varargs macros from the
>preprocessor, which it's assumed any modern compiler suite supports.

Unfortunately, we still support Microsoft Visual C, which is not a modern compiler suite.

Does gcc allow you to specify __attribute__((__nonnull__())) multiple times? If so, you could just have G_GNUC_NON_NULL() take only a single argument.

Failing that, you could define it as:

  #define G_GNUC_NON_NULL(indexes) __attribute__((__nonnull__ indexes))

and require users to double-parenthesize:

 gpointer        g_bytes_unref_to_data           (GBytes         *bytes,
                                                  gsize          *size)
                                                 G_GNUC_NON_NULL ((1, 2));

although that would be gross.

>+#if defined(G_HAVE_ISO_VARARGS)
>+#define G_GNUC_NON_NULL(...) __attribute__((nonnull(__VA_ARGS__)))
>+#elif defined(G_HAVE_GNUC_VARARGS)
>+#define G_GNUC_NON_NULL(params...) __attribute__((nonnull(__VA_ARGS__)))
>+#else
>+#define G_GNUC_NON_NULL(...)
>+#endif

You need to check __GNUC__ of course

Created attachment 262828 [details] [review]
gbytes: Add G_GNUC_NON_NULL attributes

Created attachment 262829 [details] [review]
gobject: Add G_GNUC_NON_NULL attributes

Add G_GNUC_NON_NULL to g_object_[un]ref() with the aim of catching
potential g_object_unref(NULL) calls through static analysis.

Created attachment 262830 [details] [review]
gcontenttype: Fix a potential g_object_unref(NULL) call

This can happen if the g_file_query_info() call fails, returning NULL.

Found with scan-build.

Created attachment 262831 [details] [review]
gdbusserver: Fix a potential g_object_unref(NULL) call

This can happen if the hash table lookup for ‘noncefile’ fails, and
hence the first ‘goto out’ is hit, at which point resolver is still
NULL.

Found with scan-build.

Created attachment 262832 [details] [review]
gsocketaddress: Add an assertion to help static analysis

The static analyser will check dynamic type assertions and assume that
if they fail, the variable can either have the wrong type, or be NULL
(which is correct). The analyser doesn’t know that other constraints in
the API ensure the variable is non-NULL.

Add a non-null assertion to help the static analyser and shut it up in
this case.

Created attachment 262833 [details] [review]
gunixfdlist: Fix a potential NULL pointer dereference

In the case that (n_fds == 0 && fds == NULL), memcpy() would be called
against a NULL src pointer. Even though the number of bytes to copy is
0, avoid the possibility of a crash by only calling if fds is non-NULL.

Found by scan-build.

Created attachment 262834 [details] [review]
gcontenttype: Fix a potential NULL pointer dereference

If the initial part of the header (‘MIME-TreeMagic’) is valid, but the
following line does not start with ‘[’ (i.e. is not a valid section
line), insert_matchlet() will be called with a NULL match pointer, and
will crash with a NULL pointer dereference.

Fix this by bailing out if a valid section line isn’t encountered before
the first insert_matchlet() call (i.e. between the header line and the
first data line).

Note that this has not been tested against a real treemagic file; the
fix is purely theoretical.

Found by scan-build.

Created attachment 262835 [details] [review]
gunixmounts: Fix a potential strcmp(NULL) call

mntent->mnt_fsname may be NULL at this point; if so, fall to the second
branch and set mount_entry->device_path = NULL.

Found by scan-build.

Created attachment 262836 [details] [review]
gio/tests: Fix a g_return_val_if_fail() in a void function

Should be g_return_if_fail() instead.

Created attachment 262837 [details] [review]
gio/tests: Add non-NULL assertions to help static analysis

These prevent some false positives from the static analyser which are
caused by it not inspecting the invariants of
g_subprocess_communicate[_utf8]_finish() (i.e. that stdout and
stdout_str will always be set unless an error was returned).

They’re also good testing anyway.

Found by scan-build.

Created attachment 262838 [details] [review]
gio/tests: Add a non-NULL assertion to help static analysis

The static analyser (correctly) considers a type check to fail if the
variable is NULL. In this case, the address must be non-NULL as no error
was thrown by g_socket_connection_get_remote_address(), but the static
analyser doesn’t know this.

Add a non-NULL assertion anyway, both to shut the analyser up, and
because it’s good extra testing.

Found by scan-build.

Created attachment 262839 [details] [review]
gio/tests: Add a dynamic type check assertion

This shuts up a static analysis false positive, and adds some extra
checking.

Found by scan-build.

Created attachment 262840 [details] [review]
gmacros: Add a G_GNUC_NON_NULL attribute macro

This expands to the nonnull GCC attribute (if supported).

http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bnonnull_007d-function-attribute-2825

This doesn’t require variadic macro support from the compiler, as it
assumes the compiler supports multiple separate instances of the nonnull
attribute, e.g.:
    void my_func (void *p1, void *p2)
        __attribute__((nonnull(1)))
        __attribute__((nonnull(2)));

GCC supports this (at least, version 4.8 does), but Clang does not.
Clang successfully parses the attributes, but ignores all except the
final one. This partial support is better than nothing, especially as
the vast majority of functions only have one non-NULL pointer argument.
This has been reported as a Clang bug:
http://llvm.org/bugs/show_bug.cgi?id=18063.

The support from older GCC versions, and from other compilers, for
specifying the nonnull attribute multiple times has not been tested. If
they’re found not to work, the restrictions on the definition of
G_GNUC_NON_NULL can be made stricter.

I said it on IRC already: I don't want to have these non-null checks sprinkled all over our headers...

(In reply to comment #22)
> I said it on IRC already: I don't want to have these non-null checks sprinkled
> all over our headers...

What are your reasons for not wanting these attributes in the headers?

I think you should seriously consider adding them, even if only for the core parts of GLib (e.g. GObject):
 • Few people look directly at the headers; they look at the documentation instead. I don’t think adding attributes will affect readability for a large number of people.
 • The headers are already full of ugly GLIB_AVAILABLE_IN* macros.
 • Even adding a few attributes can find bugs: I found 2 concrete potential g_object_unref(NULL) calls in GLib immediately after adding the attribute to g_object_unref(). I suspect this would be very useful for finding bugs in end-user programs which use GLib.
 • Having just built GTK+, scan-build has found 10 nonnull bugs, and from a cursory glance 9 of them look like real bugs.
 • From the testing I’ve done so far, there only seems to be one class of false positive: where the static analyser can’t associate GErrors being unset with variables being non-NULL (e.g. the standard case of a variable being non-NULL unless an error was thrown). Arguably, these conditions should be asserted as well as being documented, which should eliminate the false positives.

In any case, a G_GNUC_NON_NULL macro can be used by other projects, and doesn’t have to be used in GLib itself. Most of the other patches here are bug fixes, and don’t apply the new attribute, so they can be reviewed regardless.

Review of attachment 262831 [details] [review]:

Sure.

(In reply to comment #23)
> (In reply to comment #22)
> > I said it on IRC already: I don't want to have these non-null checks sprinkled
> > all over our headers...
> 
> What are your reasons for not wanting these attributes in the headers?
> 
> I think you should seriously consider adding them, even if only for the core
> parts of GLib (e.g. GObject):

It seems pretty clear to me that annotating g_object_ref()/_unref() etc. is a large win.  But going to say GTK+ and annotating say gtk_entry_get_text_area() to pick a random example is a lot less useful...

So count me as in favor of _NON_NULL for targeted cases.  We can gain experience, and maybe add it to some specific APIs in GTK+.

I'll just mention here too - we largely already have these assertions inside the g_return_if_fail() check.  It just needs to be made visible to the analysis tool somehow.  The trick is doing so...

If we could accomplish that, then things like uninitialized or already set GErrors (for functions which have g_return_val_if_fail (FALSE, error == NULL || *error == NULL)) could be caught.

Personally I don't get bit by GError misuse in my own code, since I use a rigorous coding style that prevents it, but it'd certainly help others.


Hmm.  You know, a static analyzer that was aware of GLib/GObject *and* used the introspection data could just assume that GObject* must be non-null unless it's annotated as (allow-none).  Which we have to do anyways for bindings.

A GLib-aware static analyzer could just know about the GError conventions too.

Review of attachment 262832 [details] [review]:

Ok.

Review of attachment 262833 [details] [review]:

Right.  I assume for most memcpy() implementations this is actually safe, but I guess it's not specified.

Review of attachment 262834 [details] [review]:

::: gio/gcontenttype.c
@@ +1041,3 @@
+                {
+                  g_warning ("%s: header corrupt; skipping\n", filename);
+                  break;

Might argue for a 'continue'...but eh, doesn't really matter.

Review of attachment 262835 [details] [review]:

Looks OK.

Review of attachment 262836 [details] [review]:

Yep.

Review of attachment 262837 [details] [review]:

Ok.

Review of attachment 262838 [details] [review]:

Ok.  (But...this is the kind of assertion that a GLib-aware analyzer could assume)

Review of attachment 262839 [details] [review]:

Interesting that it warns on this one.  Ok.

Review of attachment 262830 [details] [review]:

Right.

Attachment 262830 [details] pushed as 44af12a - gcontenttype: Fix a potential g_object_unref(NULL) call
Attachment 262831 [details] pushed as c729f41 - gdbusserver: Fix a potential g_object_unref(NULL) call
Attachment 262832 [details] pushed as 956c58c - gsocketaddress: Add an assertion to help static analysis
Attachment 262833 [details] pushed as aa28ced - gunixfdlist: Fix a potential NULL pointer dereference
Attachment 262834 [details] pushed as 8b9e8dc - gcontenttype: Fix a potential NULL pointer dereference
Attachment 262835 [details] pushed as 73e6b90 - gunixmounts: Fix a potential strcmp(NULL) call
Attachment 262836 [details] pushed as fe00444 - gio/tests: Fix a g_return_val_if_fail() in a void function
Attachment 262837 [details] pushed as c9ccc2a - gio/tests: Add non-NULL assertions to help static analysis
Attachment 262838 [details] pushed as 3211895 - gio/tests: Add a non-NULL assertion to help static analysis
Attachment 262839 [details] pushed as c9344fd - gio/tests: Add a dynamic type check assertion

Review of attachment 262828 [details] [review]:

::: glib/gbytes.h
@@ +78,3 @@
+                                                 gsize          *size)
+                                                G_GNUC_NON_NULL(1)
+                                                G_GNUC_NON_NULL(2);

We should rather modify this function so that size can be NULL.  We have a convention of being able to supply NULL for out parameters to ignore them and it may be useful here in the case that you already know the size.

Review of attachment 262840 [details] [review]:

::: glib/docs.c
@@ +2209,3 @@
+ * multiple times if multiple function arguments are non-%NULL. However, unlike
+ * the raw GCC <literal>nonnull</literal> attribute, this macro does
+ * <emphasis>not</emphasis> accept a list of function argument indices. This is

Wikipedia says that variadic macros are in C99 and MSVC has supported them (in the ISO style) since 2005.

I think we may as well do this properly.

For the record, I'm kinda upset with a lot of the "fixes" here.  The one about protecting the memcpy() with a conditional for the 0 case is particularly obnoxious.  The added casts and asserts are also upsetting.

Aside from that, it seems that most of the legitimate fixes here are not related to our use of non-null annotations.  Only a copy of g_object_unref() cases were found -- and those probably should have been changed to if() instead of using g_clear_object().


All of that said, I'm OK with proceeding with this on a limited basis.  I think we should apply it only to GObject, though -- but we may want to increase the number of functions we annotate (including weakrefs, data, ref_sink, notify, etc.).  In any case, we should only apply it to cases where NULL is really absolutely definitely never valid.  glibc's use of it on memcpy() is exactly the kind of "bad use" I'm talking about here.

Created attachment 263073 [details] [review]
gmacros: Add a G_GNUC_NON_NULL attribute macro

This expands to the nonnull GCC attribute (if supported).

http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bnonnull_007d-function-attribute-2825

This requires variadic macro support from the compiler, which is now
mandatory to build GLib. It seems that all the compilers we care about
have supported at least one variety of variadic macros for a while.

Created attachment 263074 [details] [review]
gobject: Add G_GNUC_NON_NULL attributes

Add G_GNUC_NON_NULL to various core GObject and signal functions with
the aim of catching calls like g_object_unref(some_NULL_variable)
through static analysis.

Created attachment 263075 [details] [review]
gstrfuncs: Add missing preconditions to g_str_match_string()

Created attachment 263076 [details] [review]
gstrfuncs: Add G_GNUC_NON_NULL attributes

Add G_GNUC_NON_NULL to various string functions with the aim of
catching calls with invalid NULL pointers through static analysis.

Comment on attachment 262828 [details] [review]
gbytes: Add G_GNUC_NON_NULL attributes

Let’s not bother with GBytes.

Here we go. I’ve annotated gobject.h, gsignal.h and gstrfuncs.h, which I think is plenty for now.

The new annotations found no additional GLib bugs, and one legitimate g_signal_handler_disconnect(NULL) call in GTK+ (patch here: https://bugzilla.gnome.org/show_bug.cgi?id=712760#c65). This is a bit disappointing, but it’s early days. I’ll try analysing some more modules tomorrow and see how things go.

Ryan and I discussed this at the hackfest yesterday, and decided on the following:
 • We don’t use attributes in GLib headers, because they’re too noisy and ugly. Instead, we rely on static analysis using GIR annotations and a tool like gnome-clang[1] which makes use of them.
 • Because of this, we don’t add G_GNUC_NON_NULL (or any other attribute). We shouldn’t be adding things to GLib which we refuse to use in GLib itself.
 • GLib will have a clear policy that using attributes in its header files is discouraged.
 • We run regular static analysis builds of GLib and other modules. Ideally, this should be a JHBuild slave. I’ll talk to people about getting one set up.

The basic reasoning here is that attributes are ugly, annotations are not, and that the warnings coming from a normal compilation process using attributes are not valuable enough to justify adding the attributes in the first place — but the warnings coming from static analysis using annotations are.

Attributes and annotations accomplish much the same task, but annotations are pretty and already exist across much of the GLib code base.

[1]: http://people.collabora.com/~pwith/gnome-clang/

Comment on attachment 263073 [details] [review]
gmacros: Add a G_GNUC_NON_NULL attribute macro

Rejected as per comment #46.

Comment on attachment 263074 [details] [review]
gobject: Add G_GNUC_NON_NULL attributes

Rejected as per comment #46.

Comment on attachment 263076 [details] [review]
gstrfuncs: Add G_GNUC_NON_NULL attributes

Rejected as per comment #46.

Created attachment 275623 [details] [review]
docs: Add a README.style documenting code style and attribute policy

The coding style is very brief, and could be expanded if we want to be
more prescriptive. The main point of adding this file is to have
somewhere useful to put GLib’s policy on compiler attributes and
annotations.

Review of attachment 275623 [details] [review]:

If we add this file I'd like to see it substantially fleshed out.

Also: just refer to the fact that we use GNU style throughout, with spaces for indentation.

GTK+ already has a pretty substantial coding style document; we could lift that for GLib's own coding style document, since the two pretty much match already.

Created attachment 275627 [details] [review]
docs: Add a README.rationale documenting major design decisions

It will be useful to document the major decisions which affect the whole
of GLib in one centralised, easily-greppable file, otherwise they will
get lost forever in Bugzilla.

This file should contain a brief explanation of the decision and its
rationale, plus a link to further discussion (e.g. on a mailing list or
bug report).

This contains an initial discussion about use of compiler attributes in
GLib.

Comment on attachment 275623 [details] [review]
docs: Add a README.style documenting code style and attribute policy

I only added README.style as a reasonably relevant place to document the annotation discussion. In-person discussion with Ryan has resulted in the decision to create README.rationale instead.

If someone else wants to add a coding style guide, they’re welcome to. I’m not bothered about it though.

(In reply to comment #46)
>  • We don’t use attributes in GLib headers, because they’re too noisy and ugly.

That's not true. We use G_GNUC_PURE, G_GNUC_MALLOC, G_GNUC_NULL_TERMINATED, G_GNUC_PRINTF, etc.

I guess maybe you mean "we use function-level attributes, but not argument-level attributes"?

(IMHO we already lost the battle against "noisy and ugly" when we added GLIB_AVAILABLE_IN_ALL to every single function.)

(In reply to comment #55)
> (In reply to comment #46)
> >  • We don’t use attributes in GLib headers, because they’re too noisy and ugly.
> 
> That's not true. We use G_GNUC_PURE, G_GNUC_MALLOC, G_GNUC_NULL_TERMINATED,
> G_GNUC_PRINTF, etc.
> 
> I guess maybe you mean "we use function-level attributes, but not
> argument-level attributes"?

Maybe more like “we don’t want further use of attributes”.

Review of attachment 263075 [details] [review]:

Thanks

Review of attachment 275627 [details] [review]:

Would appreciate a few other things mentioned in the RATIONALE file, but I guess we can add those as we go.  OK to commit this after the small mistake.

::: HACKING
@@ +35,3 @@
 
+For information about submitting patches see the README.commits file. For
+information about coding style, see the README.style file.

Missed this part?

Great, merged to master with that tiny fix to HACKING. I think we can close this bug (as WONTFIX) now. Thanks for your patience on this, Ryan.

Attachment 263075 [details] pushed as 11297fd - gstrfuncs: Add missing preconditions to g_str_match_string()
Attachment 275627 [details] pushed as 63737df - docs: Add a README.rationale documenting major design decisions

Created attachment 277375 [details] [review]
Attaching two patches I had locally so the work doesn't get lost if (for whatever reason) we decide to add this attribute in future (which I consider very unlikely). Sorry for the noise.

--

gmacros: Add a G_GNUC_RETURNS_NON_NULL attribute macro

This adds support for the GCC returns_nonnull attribure, which was added
in GCC 4.9 (but appears to be available in Fedora 19 as well).

This attribute can be used to mark functions as never returning NULL,
which can result in optimisations being applied to the call sites. It
also adds implicit documentation for the user.

The attribute currently is not supported in Clang, but there is a bug
open about adding support:
    http://llvm.org/bugs/show_bug.cgi?id=4832

Created attachment 277376 [details] [review]
gobject: Add G_GNUC_RETURNS_NON_NULL attributes

Add G_GNUC_RETURNS_NON_NULL to various GObject functions with the aim of
further documenting the nullability of return types and of enabling
optimisations in calling code.