GNOME Bugzilla – Bug 649201
Fix usage of _GNU_SOURCE
Last modified: 2011-05-03 14:15:03 UTC
_GNU_SOURCE must be defined before including any other (system) header, so defining it in glib-unix.h (and hoping no one has included anything else before that) is wrong. And the "#define _USE_GNU" workaround for this problem in gnetworkingprivate.h is even wronger (and still prone to failure anyway due to single-include guards). Fix this by defining _GNU_SOURCE in config.h when building against glibc. In theory this is bad because new releases of glibc may include symbols that conflict with glib symbols, which could then cause compile failures. However, most people only see new releases of glibc when they upgrade their distro, at which point they also generally get new releases of gcc, which have new warnings/errors to clean up anyway.
Created attachment 187044 [details] [review] Fix usage of _GNU_SOURCE
(In reply to comment #0) > _GNU_SOURCE must be defined before including any other (system) > header, so defining it in glib-unix.h (and hoping no one has included > anything else before that) is wrong. But since glib-unix.h is a new header, we can document it should be included first, no? And note that glib-unix.h carefully only defines it for the duration of its system includes. > Fix this by defining _GNU_SOURCE in config.h when building against > glibc. Hmmm. I'd really rather try fixing all of the GLib code that does GNU_SOURCE tricks now to #include <glib-unix.h> first. Also, we should fix Gio's random pipe2() wrappers to use g_unix_pipe_flags!
Though there is definitely a problem in that the private <glib-unix.h> depends on the private config.h header.
Er, *public* depends on private
(In reply to comment #3) > Though there is definitely a problem in that the private <glib-unix.h> depends > on the private config.h header. It can't.
(In reply to comment #2) > And note that glib-unix.h carefully only defines it for the duration of its > system includes. Actually, that doesn't work, because _USE_GNU will have been #defined by features.h at that point, and that's the symbol that all of the other headers check. If you really wanted that effect, you'd have to do something like: #undef _GNU_SOURCE #undef _FEATURES_H #include <features.h> But really, there is no reason to do that. We want to use the full functionality of our libc. So let's do it.
Review of attachment 187044 [details] [review]: Everything except this bit looks OK: ::: glib/glib-unix.h @@ -30,3 @@ -#define _G_GNU_SOURCE_TEMPORARILY_DEFINED -#define _GNU_SOURCE -#endif But your change won't have _GNU_SOURCE defined for glib-unix.h, because your change only affects config.h, and that's a private header. So I stand by this code, and don't think it should be removed in this patch. We need a different fix for it.
Created attachment 187077 [details] [review] Generate glib-unix.h to avoid a dependency on private config.h We can't check #ifdef HAVE_CONFIG_H in a public header; switch to generating it.
(In reply to comment #7) > Review of attachment 187044 [details] [review]: > But your change won't have _GNU_SOURCE defined for glib-unix.h, because your > change only affects config.h, and that's a private header. You don't need _GNU_SOURCE to be defined there though. glib-unix.h itself doesn't use anything that is only defined under _GNU_SOURCE, and all users of glib-unix.h within glib will already have _GNU_SOURCE defined for them. And if people want to use glibc-specific functions in their non-glib glib-unix-using code, they can #define _GNU_SOURCE themselves. #defining _GNU_SOURCE for the user in glib-unix isn't really that much of a convenience, because it just changes one weird special rule ("You must #define _GNU_SOURCE before including any system header") into a different weird special rule ("You must #include <glib-unix.h> before including any system header"). Let's let people keep their headers alphabetized, and not go back to the fun days of "you have to include <sys/types.h> before <sys/socket.h>". And speaking of header problems on ancient UNIXes... (In reply to comment #8) > We can't check #ifdef HAVE_CONFIG_H in a public header; switch > to generating it. You meant HAVE_UNISTD_H, but do you even need to check anyway? unistd.h is part of POSIX. It should be there on any UNIX platform glib can compile on... (eg, bug 647341 and bug 644905 are complaining about unconditional unistd.h includes breaking Windows builds).
(In reply to comment #9) > (In reply to comment #7) > > Review of attachment 187044 [details] [review] [details]: > > But your change won't have _GNU_SOURCE defined for glib-unix.h, because your > > change only affects config.h, and that's a private header. > > You don't need _GNU_SOURCE to be defined there though. glib-unix.h itself > doesn't use anything that is only defined under _GNU_SOURCE, and all users of > glib-unix.h within glib will already have _GNU_SOURCE defined for them. Hmmm. The problem is I needed to get the prototype for e.g. pipe2() inside glib-unix.c. But you're right, with your patch I think this would no longer be necessary. > You meant HAVE_UNISTD_H, but do you even need to check anyway? unistd.h is part > of POSIX. It should be there on any UNIX platform glib can compile on... (eg, > bug 647341 and bug 644905 are complaining about unconditional unistd.h includes > breaking Windows builds). Okay, you've convinced me on this too. Let's go with your patch then, and I'll do a separate one to just remove the #ifdef HAVE_UNISTD_H bits.
Created attachment 187085 [details] [review] glib-unix.h: Unconditionally include unistd.h danw points out it's part of POSIX.
Comment on attachment 187085 [details] [review] glib-unix.h: Unconditionally include unistd.h Hm... maybe move up the #ifndef G_OS_UNIX #error "This header may only be used on UNIX" #endif ? Otherwise it will fail before you get there anyway... (Actually, I guess you'd just get two errors, at least in gcc)
(In reply to comment #12) > (From update of attachment 187085 [details] [review]) > Hm... maybe move up the > > #ifndef G_OS_UNIX > #error "This header may only be used on UNIX" > #endif > > ? Otherwise it will fail before you get there anyway... > > (Actually, I guess you'd just get two errors, at least in gcc) Seems like a separate bug.
Attachment 187085 [details] pushed as e08e70e - glib-unix.h: Unconditionally include unistd.h