GNOME Bugzilla – Bug 344388
Add G_GNUC_NULL_TERMINATED where necessary
Last modified: 2007-09-18 00:35:28 UTC
As on AMD64 (or other 64bit systems), it is important that NULL terminated varargs calls occur with a correct definition of NULL, when C++ is involved: http://www.uwog.net/news/?p=38 So here is a patch that adds G_GNUC_NULL_TERMINATED to those calls that are NULL terminated. However, the problem with this patch is that there occur new warnings that are false positive: bsebus.c: In function 'bse_bus_get_stack': bsebus.c:582: warning: not enough variable arguments to fit a sentinel bsebus.c:584: warning: not enough variable arguments to fit a sentinel bsebus.c:586: warning: not enough variable arguments to fit a sentinel The reason is that the NULL sentinel doesn't occur in the varargs part of the function, but before: line 582 reads: self->vin = bse_container_new_child_bname (BSE_CONTAINER (snet), BSE_TYPE_SUB_IPORT, "%VInput", NULL); Whereas bsecontainer.h contains: gpointer bse_container_new_child_bname (BseContainer *container, GType child_type, const gchar *base_name, const gchar *first_param_name, ...) G_GNUC_NULL_TERMINATED; so the "sentinel" is first_param_name here, which is not accepted by gcc. Should I try to update the patch so that the first_param_name argument disappears and the function prototype will be (will be needed at other places as well): gpointer bse_container_new_child_bname (BseContainer *container, GType child_type, const gchar *base_name, ...) G_GNUC_NULL_TERMINATED; This would make the false positives go away, I suppose. Will attach the patch I have so far.
Created attachment 67038 [details] [review] Current patch adding G_GNUC_NULL_TERMINATED
(In reply to comment #0) > As on AMD64 (or other 64bit systems), it is important that NULL terminated > varargs calls occur with a correct definition of NULL, when C++ is involved: > http://www.uwog.net/news/?p=38 sorry? corect varargs list termination is required on all systems. > So here is a patch that adds G_GNUC_NULL_TERMINATED to those calls that are > NULL terminated. However, the problem with this patch is that there occur new > warnings that are false positive: > > bsebus.c: In function 'bse_bus_get_stack': > bsebus.c:582: warning: not enough variable arguments to fit a sentinel > bsebus.c:584: warning: not enough variable arguments to fit a sentinel > bsebus.c:586: warning: not enough variable arguments to fit a sentinel if i understand you correctly, *all* G_GNUC_NULL_TERMINATED changes in the current patch produce new warnings that way, so it's essentially useless? some otehr changes intermixed in the patch can go in right away though, e.g. you could: - remove the gxk_led_set_colors() decl - commit the definition in birnet/birnetutils.h, provided you remove the newlines withihn #if ... #endif - commit the PRINTF() annotations, i.e. sfi/sfidl-parser.hh, bse_item_undo_open(). > The reason is that the NULL sentinel doesn't occur in the varargs part of the > function, but before: line 582 reads: > > self->vin = bse_container_new_child_bname (BSE_CONTAINER (snet), > BSE_TYPE_SUB_IPORT, "%VInput", NULL); > > Whereas bsecontainer.h contains: > > gpointer bse_container_new_child_bname (BseContainer *container, > GType child_type, > const gchar *base_name, > const gchar > *first_param_name, > ...) G_GNUC_NULL_TERMINATED; > > so the "sentinel" is first_param_name here, which is not accepted by gcc. > > Should I try to update the patch so that the first_param_name argument > disappears and the function prototype will be (will be needed at other places > as well): > > gpointer bse_container_new_child_bname (BseContainer *container, > GType child_type, > const gchar *base_name, > ...) G_GNUC_NULL_TERMINATED; > > This would make the false positives go away, I suppose. > > Will attach the patch I have so far. this is the reason we're using the NULL_TERMINATED flagging rather seldomly so far. it's a pity that null-termination has to be traded against type safety here, it'd probably good to contact the gcc guys about this to figure whether the sentinel flag could be changed/extended to support typing of the first varargs parameter.
the issue has been raised to the gcc team here: http://gcc.gnu.org/ml/gcc/2006-06/msg00338.html
(In reply to comment #2) > (In reply to comment #0) > > As on AMD64 (or other 64bit systems), it is important that NULL terminated > > varargs calls occur with a correct definition of NULL, when C++ is involved: > > http://www.uwog.net/news/?p=38 > > sorry? corect varargs list termination is required on all systems. Sure. Its just that even with correct varargs termination, on 64bit systems under unfortunate circumstances the code will still segfault (the link describes what the AbiWord people saw). > > So here is a patch that adds G_GNUC_NULL_TERMINATED to those calls that are > > NULL terminated. However, the problem with this patch is that there occur new > > warnings that are false positive: > > > > bsebus.c: In function 'bse_bus_get_stack': > > bsebus.c:582: warning: not enough variable arguments to fit a sentinel > > bsebus.c:584: warning: not enough variable arguments to fit a sentinel > > bsebus.c:586: warning: not enough variable arguments to fit a sentinel > > if i understand you correctly, *all* G_GNUC_NULL_TERMINATED changes in the > current patch produce new warnings that way, so it's essentially useless? Actually, there are some cases where no new warnings can occur, because immediate NULL termination makes no sense. For instance consider: void CxxBase::set (const gchar *first_property_name, ...) { va_list var_args; va_start (var_args, first_property_name); g_object_set_valist (gobject (), first_property_name, var_args); va_end (var_args); } in bse/bsecxxbase.cc. Although you could theoretically call this "set multiple properties" function with zero properties, it wouldn't make sense, because setting zero properties is rather useless. So there are some cases where adding G_GNUC_NULL_TERMINATED, as it is defined right now, doesn't and will never cause useless warnings. > some otehr changes intermixed in the patch can go in right away though, e.g. > you could: > - commit the definition in birnet/birnetutils.h, provided you remove the > newlines withihn #if ... #endif I didn't commit that one yet, because we may want to use a newly implemented attribute, instead of __sentinel__ (see discussion on gcc list). > - remove the gxk_led_set_colors() decl > - commit the PRINTF() annotations, i.e. sfi/sfidl-parser.hh, > bse_item_undo_open(). Committed.
(In reply to comment #4) > > if i understand you correctly, *all* G_GNUC_NULL_TERMINATED changes in the > > current patch produce new warnings that way, so it's essentially useless? > > Actually, there are some cases where no new warnings can occur, because > immediate NULL termination makes no sense. For instance consider: > > void > CxxBase::set (const gchar *first_property_name, > ...) > { > va_list var_args; > va_start (var_args, first_property_name); > g_object_set_valist (gobject (), first_property_name, var_args); > va_end (var_args); > } > > in bse/bsecxxbase.cc. Although you could theoretically call this "set multiple > properties" function with zero properties, it wouldn't make sense, because > setting zero properties is rather useless. > > So there are some cases where adding G_GNUC_NULL_TERMINATED, as it is defined > right now, doesn't and will never cause useless warnings. can you please submit a new patch for just these "valid" cases? > > some otehr changes intermixed in the patch can go in right away though, e.g. > > you could: > > - commit the definition in birnet/birnetutils.h, provided you remove the > > newlines withihn #if ... #endif > > I didn't commit that one yet, because we may want to use a newly implemented > attribute, instead of __sentinel__ (see discussion on gcc list). i'd rather not wait on that thread before taking any action. if some future gcc version implements what we need, we can still add more null-termination flags. until then, we should flag those functions where a sentinel already makes sense.
Created attachment 67435 [details] [review] Reduced patch adding G_GNUC_NULL_TERMINATED where possible Here is a new version of the patch, where I removed those G_GNUC_NULL_TERMINATED attributes that caused false positive warnings. I just removed the attribute where actual warnings occured, so it is possible that when writing new code, that calls one of the functions that I added G_GNUC_NULL_TERMINATED to, in a way that wasn't used before, a new false positive warning will occur, and the attribute on the corresponding function will need to be removed. However, I think this "lazy" strategy of removing attributes which cause false positive warnings is better than the "eager" strategy (removing all attributes that could possibly cause problems in some future use case), due to two advantages * it is rather easy to do * it allows us to check the correct NULL termination in as many cases as possible with the current code base Ok to commit?
thanks, applied: 2007-09-18 02:27:58 Tim Janik <timj@gtk.org> * beast-gtk/bstmenus.h: * beast-gtk/bstprocedure.h: * beast-gtk/bstutils.h: * beast-gtk/gxk/gxkutils.h: * tools/sfiutils.h: added G_GNUC_NULL_TERMINATED to function ellipsis, patch by Stefan Westerfeld from bug #344388. 2007-09-18 02:25:33 Tim Janik <timj@gtk.org> * bse/bsecxxbase.hh: * bse/bseengine.h: * bse/bseitem.h: added G_GNUC_NULL_TERMINATED to function ellipsis, patch by Stefan Westerfeld from bug #344388. 2007-09-18 02:27:07 Tim Janik <timj@gtk.org> * sfi/glib-extra.h: * sfi/sfiglueproxy.h: added G_GNUC_NULL_TERMINATED to function ellipsis, patch by Stefan Westerfeld from bug #344388.