After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 617949 - glib trunk fails to compile on Solaris w/ Studio 12 & -D_XOPEN_SOURCE=600
glib trunk fails to compile on Solaris w/ Studio 12 & -D_XOPEN_SOURCE=600
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.29.x
Other All
: Normal blocker
: ---
Assigned To: gtkdev
gtkdev
: 655027 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-05-06 18:39 UTC by Andrew Paprocki
Modified: 2011-09-04 21:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fixes for platforms other than linux (6.10 KB, patch)
2010-08-25 13:38 UTC, Patrick Welche
none Details | Review
fix statvfs vs statfs (4.81 KB, patch)
2011-07-18 17:06 UTC, Patrick Welche
committed Details | Review
config.log result of configure on OS X (434.85 KB, application/octet-stream)
2011-08-10 19:03 UTC, jessevdk@gmail.com
  Details
The /usr/include/sys/mount.h (14.82 KB, application/octet-stream)
2011-08-10 19:04 UTC, jessevdk@gmail.com
  Details
statfs statvfs getmntinfo fchixes pat (4.78 KB, patch)
2011-08-12 09:58 UTC, Patrick Welche
reviewed Details | Review
Config.log from patch 193681 (500.59 KB, text/plain)
2011-08-12 17:43 UTC, John Ralls
  Details
MacOSX10.4u.sdk mount.h (14.50 KB, text/plain)
2011-08-12 18:19 UTC, John Ralls
  Details
force error in getmntinfo check (4.94 KB, patch)
2011-08-16 14:27 UTC, Patrick Welche
reviewed Details | Review
Patrick's patch with already-fixed section removed (4.10 KB, patch)
2011-08-20 20:07 UTC, John Ralls
none Details | Review
Patrick's patch with already-fixed section removed and spaces-before-open-parens fixed (4.11 KB, patch)
2011-08-20 21:33 UTC, John Ralls
reviewed Details | Review
use getstat{v,}fsstat for which problematic getmntinfo is a wrapper (6.48 KB, patch)
2011-09-01 14:26 UTC, Patrick Welche
none Details | Review
harrden configure string tests (5.05 KB, patch)
2011-09-01 14:31 UTC, Patrick Welche
none Details | Review
use getstat{v,}fsstat for which problematic getmntinfo is a wrapper (6.64 KB, patch)
2011-09-02 17:41 UTC, Patrick Welche
committed Details | Review

Description Andrew Paprocki 2010-05-06 18:39:49 UTC
The configure script under Solaris is hard-coded to define _XOPEN_SOURCE=2 _XOPEN_SOURCE_EXTENDED=1, which selects XPG4v2. This selection should not be done by configure and should instead be passed by the user of configure via CFLAGS. The configuration fails like this:

checking number of arguments to statfs()... unknown
configure: error: unable to determine number of arguments to statfs()

When configure is invoked as:

CC=c99 CFLAGS="-Xc -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=200112L" ./configure

The fix is to simply remove these two lines from configure.in:

     AC_DEFINE(_XOPEN_SOURCE_EXTENDED, 1, Needed to get declarations for msg_control and msg_controllen on Solaris)
     AC_DEFINE(_XOPEN_SOURCE,          2, Needed to get declarations for msg_control and msg_controllen on Solaris)

*OR* alternatively, simply replace them with this one line to use XPGv6:

     AC_DEFINE(_XOPEN_SOURCE,        600, Needed to get declarations for msg_control and msg_controllen on Solaris)
Comment 1 Patrick Welche 2010-08-24 15:12:11 UTC
gio/glocalfile.c correctly has logic to decide whether statfs or statvfs should be used.

Not sure about gio/gunixmounts.c:573: struct statfs is "protected" by HAVE_GETMNTINFO, HAVE_FSTAB_H and HAVE_SYS_MOUNT_H. I would add HAVE_SYS_STATFS_H, as getmintinfo() can return struct statvfs on some systems.

And then why use __INTERIX for the statvfs version? Maybe that should be elif HAVE_SYS_STATVFS?

It looks to me as though configure.ac has forgotten the possibility of not having a struct statfs.
Comment 2 Patrick Welche 2010-08-25 09:44:19 UTC
Practically finished sorting out a patch: maintainers: I don't know the form on how you deal with "other" modules - can I assign this bug to myself and sort it out?
Comment 3 Patrick Welche 2010-08-25 09:45:01 UTC
(PS found an issue with dtrace too while looking at configure.ac)
Comment 4 Patrick Welche 2010-08-25 13:38:34 UTC
Created attachment 168731 [details] [review]
Fixes for platforms other than linux

 - move choice of statfs vs statvfs from gio/glocalfile.c to configure.ac,
   and use choice in gio/gunixmounts.c as well.
 - configure.ac: both dtrace and sys/sdt.h need to be present to use dtrace.
   Current test uses dtrace if just sys/sdt.h is present.
 - configure.ac: macros go in m4macros
Comment 5 Matthias Clasen 2010-08-25 14:23:09 UTC
Review of attachment 168731 [details] [review]:

Looks reasonable at first sight. I haven't tested it, though...
Comment 6 Patrick Welche 2010-08-26 13:11:58 UTC
It looks as though my patch addresses a small part of Bug 583330, and the original poster's problem (related to Bug 133344) seems to be addressed properly by

http://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/devel/glib2/patches/patch-ak?rev=1.6&content-type=text/x-cvsweb-markup&only_with_tag=MAIN
Comment 7 Patrick Welche 2010-08-26 13:15:02 UTC
(The patch linked in the above comment also addresses 2 other things - it's the first part of that patch which takes care of both options in comment 1)
Comment 8 Patrick Welche 2011-07-18 17:06:13 UTC
Created attachment 192205 [details] [review]
fix statvfs vs statfs
Comment 9 Patrick Welche 2011-07-18 17:07:20 UTC
So, since last year, two bits of this patch were rediscovered and applied:

dd9f8b8c Tue Sep 28 2010 sdt.h
33c7a9d8 Fri Jan  7 2011 m4macros

Here is a resubmission of the rest in the hope that it gets in this time...
Comment 10 Colin Walters 2011-07-18 18:34:42 UTC
Review of attachment 192205 [details] [review]:

Looks good to me; thank you!
Comment 11 jessevdk@gmail.com 2011-08-09 21:16:42 UTC
It seems that this patch has broken support for OS X. I'm not really familiar with what exactly the patch does, but right now I get the following error:

gunixmounts.c: In function ‘_g_get_unix_mounts’:
gunixmounts.c:608: warning: passing argument 1 of ‘getmntinfo’ from incompatible pointer type

Which on OS X takes a statfs (not statvfs), however during configure it has been determined that statvfs should be used.
Comment 12 Patrick Welche 2011-08-10 16:05:24 UTC
Any chance of a copy of your config.log, and maybe a copy of your /usr/include/sys/mount.h?
Comment 13 jessevdk@gmail.com 2011-08-10 19:03:13 UTC
Created attachment 193591 [details]
config.log result of configure on OS X

The config.log
Comment 14 jessevdk@gmail.com 2011-08-10 19:04:45 UTC
Created attachment 193592 [details]
The /usr/include/sys/mount.h

The /usr/include/sys/mount.h on OS X 10.6
Comment 15 jessevdk@gmail.com 2011-08-10 19:07:22 UTC
Please shoot me down if I'm wrong, but it seems the test to choose between statfs and statvfs when they are both available is wrong for OS X:

Excerpt from configure.ac:

# Some systems have both statfs and statvfs, pick the most "native" for these
AS_IF([test x$ac_cv_func_statfs = xyes && test x$ac_cv_func_statvfs = xyes],
   [
   # on solaris and irix, statfs doesn't even have the f_bavail field
   AS_IF([test x$ac_cv_member_struct_statfs_f_bavail = xyes],
      [ac_cv_func_statfs=no],
   # else, at least on linux, statfs is the actual syscall
      [ac_cv_func_statvfs=no])
   ])

On OS X, f_bavail is a member of statfs
Comment 16 Patrick Welche 2011-08-11 10:21:05 UTC
I'm afraid I don't have a handy macos box to try this out, so I'll have to ask you for rather basic information :-(

It seems that configure doesn't find sys/statfs.h on your system, as

/usr/bin/gcc-4.2 -c -O0 -ggdb3 -arch x86_64 -mmacosx-version-min=10.6 -Wall -I/Users/jesse/gtk/inst/include -DG_DISABLE_SINGLE_INCLUDES conftest.c >&5

fails (according to the config.log you sent).

You say that OS X uses statfs, so what headers would you use? (Not sys/statfs.h ?)
Comment 17 Patrick Welche 2011-08-11 10:22:38 UTC
Sorry, I think that was a stupid question (reading your mount.h)
Comment 18 jessevdk@gmail.com 2011-08-11 10:51:50 UTC
Indeed, statfs is defined in mount.h. The error about not finding sys/statfs.h is not very relevant (as I understand it simply tests for the availability of statfs call in some headers, including mount.h).

Noting this comment:
# on solaris and irix, statfs doesn't even have the f_bavail field

It seems like a bad heuristic to use this field to determine whether to use statfs or statvfs.
Comment 19 Patrick Welche 2011-08-11 11:35:03 UTC
Here is what I was writing while you wrote comment 18, so I assume the
answer to "is this plausible?" is yes!:

A colleague just leant me a mac, so here is what I think is happening:
sys/mount.h defines struct statfs and declares statfs()
sys/statvfs.h defines struct statvfs and declares statvfs()
As statvfs are provided as wrappers to statfs,  the call to statvfs in
glocalfile.c should work, so the choice USE_STATVFS should be OK.

The problem is then the assumption that if statvfs() is OK, getmntinfo takes a
struct statvfs.

I'll write a test based on the examples (though obviously not testing for the
OS):

Linux:
        no getmntinfo
BSD:
        #include <sys/types.h>
        #include <sys/statvfs.h>
        int getmntinfo(struct statvfs **mntbufp, int flags);
Mac OS X:
        #include <sys/param.h>
        #include <sys/ucred.h>
        #include <sys/mount.h>
        int getmntinfo(struct statfs **mntbufp, int flags);

Sound plausible?
Comment 20 Patrick Welche 2011-08-12 09:58:57 UTC
Created attachment 193681 [details] [review]
statfs statvfs getmntinfo fchixes pat

While writing the getmntinfo test, I fixed two other parts, I had the logic of

-# if !defined(HAVE_STRUCT_STATFS_F_BAVAIL)
-   /* on solaris and irix, statfs doesn't even have the
-      f_bavail field */
-#  define USE_STATVFS

exactly inverted, so this is the right way around:
    # on solaris and irix, statfs doesn't even have the f_bavail field
-   AS_IF([test x$ac_cv_member_struct_statfs_f_bavail = xyes],
+   AS_IF([test x$ac_cv_member_struct_statfs_f_bavail != xyes],
       [ac_cv_func_statfs=no],

I also added (typeof(statfs_buffer)=struct statvfs - don't be put off by name)
+#if defined(HAVE_STRUCT_STATVFS_F_FSTYPENAME)
+  fstype = g_strdup(statfs_buffer.f_fstypename); 
which looks the same as the statfs case, so I don't see why f_fstypename
shouldn't be used if it is available.

OK to commit?
Comment 21 Colin Walters 2011-08-12 10:57:58 UTC
Review of attachment 193681 [details] [review]:

I tested this on GNOME/Linux and it looks fine.

Let's see if we can get feedback from the OS X people.

::: gio/glocalfile.c
@@ +1004,3 @@
+#elif defined(USE_STATVFS)
+#if defined(HAVE_STRUCT_STATVFS_F_FSTYPENAME)
+  fstype = g_strdup(statfs_buffer.f_fstypename); 

We use a space between identifier and paren.

@@ +1008,1 @@
   fstype = g_strdup(statfs_buffer.f_basetype); 

Ditto.
Comment 22 Colin Walters 2011-08-12 11:01:03 UTC
John, can you try this patch?
Comment 23 Colin Walters 2011-08-12 11:01:21 UTC
*** Bug 655027 has been marked as a duplicate of this bug. ***
Comment 24 John Ralls 2011-08-12 17:43:37 UTC
Created attachment 193718 [details]
Config.log from patch 193681

Unfortunately, no:

  CC     libgio_2_0_la-gunixfdlist.lo
  CC     libgio_2_0_la-gunixfdmessage.lo
  CC     libgio_2_0_la-gunixmount.lo
  CC     libgio_2_0_la-gunixmounts.lo
gunixmounts.c: In function '_g_get_unix_mounts':
gunixmounts.c:608: warning: passing argument 1 of 'getmntinfo' from incompatible pointer type
gunixmounts.c:617: error: 'struct statvfs' has no member named 'f_mntonname'
gunixmounts.c:618: error: 'struct statvfs' has no member named 'f_mntfromname'
gunixmounts.c:619: error: 'struct statvfs' has no member named 'f_fstypename'
gunixmounts.c:621: error: 'struct statvfs' has no member named 'f_flags'
gunixmounts.c: In function '_g_get_unix_mount_points':
gunixmounts.c:1006: warning: unused variable 'len'
make[4]: *** [libgio_2_0_la-gunixmounts.lo] Error 1
Comment 25 John Ralls 2011-08-12 18:19:43 UTC
Created attachment 193720 [details]
MacOSX10.4u.sdk mount.h

The bit that's missing is that we ( the gtk-quartz maintainers) are trying to maintain backward compatibility with 10.4, and Apple changed struct statvs between 10.5 and 10.6. I'm starting a check build on 10.7, using the 10.7 SDK, but that will take a couple of hours (I have to start from bootstrap) and I'll report back when it's done. (I expect that it will succeed.)
Comment 26 John Ralls 2011-08-12 20:33:16 UTC
Never mind my ranting about 10.4 compatibility: That wasn't the problem at all. 
The problem is that the getmntinfo argument check isn't working:

configure:23228: checking type of argument to getmntinfo
configure:23255: /usr/bin/gcc-4.2 -c -O0 -ggdb3 -arch i386 -I/Developer/SDKs/MacOSX10.5.sdk/usr/include -isysroot /Developer/SDKs/MacOSX10.5.sdk -mmacosx-version-min=10.5 -Wall -I/usr/local/gtk-unstable-3/inst/include -I/Developer/SDKs/MacOSX10.5.sdk/usr/include -DG_DISABLE_SINGLE_INCLUDES conftest.c
conftest.c: In function 'main':
conftest.c:148: warning: passing argument 1 of 'getmntinfo' from incompatible pointer type
configure:23255: $? = 0
configure:23256: result: struct statvfs**

The warning isn't strong enough to make the compile fail, so even though
/usr/include/sys/mount.h:int	getmntinfo(struct statfs **, int) __DARWIN_INODE64(getmntinfo);
it succeeds and sets GETMNTINFO_STRUCT_STATVFS.

One could work around that by setting -Werror in $CFLAGS, but compilation of glib isn't free of warnings, so that's not really feasible. Is there a way to force it for only that check?
Comment 27 Patrick Welche 2011-08-16 14:27:07 UTC
Created attachment 193953 [details] [review]
force error in getmntinfo check

Sorry for wasting time trying to do "better". This version of the previous patch does exactly as you suggest and adds Werror. (Just for the test, not in general. Adding Werror then gets me an erroneous error from the uninitialised use of struct warning hence the spurious addition of =NULL to keep gcc happy.) AFAICT Werror is only correct for GCC, so I only set it in that case. I wonder what happens if you try llvm/clang on macos...
Comment 28 Patrick Welche 2011-08-16 14:28:32 UTC
(Just realised: I haven't added the space as per comment 21 :-/ (BTW you worked out which line I cut and pasted ;-) ))
Comment 29 John Ralls 2011-08-16 22:06:57 UTC
Comment on attachment 193953 [details] [review]
force error in getmntinfo check

OK, this one works.

I haven't tried building Gtk with clang yet, but I did look in the documentation. It uses -Werror just like gcc, so this should be OK there, too.
Comment 30 Patrick Welche 2011-08-18 14:24:28 UTC
I would suggest applying this patch (plus the missing space) - it fixes the problem case under consideration, which is building on macos with gcc. Colin?

I know that -Werror is necessary for, and works on GCC, and the test for GCC is easy.
It seems that -Werror is also OK for clang and pcc, but is it necessary?
I just tried cc: Sun Ceres C 5.10 SunOS_i386 2009/03/06 and it didn't understand -Werror.
It may be that other compiles don't need an extra flag to make that warning an error.
Either way, I would rather complicate if we find it necessary, but not otherwise...
Comment 31 John Ralls 2011-08-20 20:07:03 UTC
Created attachment 194308 [details] [review]
Patrick's patch with already-fixed section removed

OK, that works now, except that Mattias checked in http://git.gnome.org/browse/glib/commit/configure.ac?id=3c504e47656e7d26c0c24465f492b92f673a5cec which already does one of your changes and makes the patch not apply. This fixes that little problem so that it applies against master.

I also fixed the spaces before opening parens, though I see another violation in one of the context lines. Whoever commits can fix that up. ;-)
Comment 32 John Ralls 2011-08-20 21:33:32 UTC
Created attachment 194313 [details] [review]
Patrick's patch with already-fixed section removed and spaces-before-open-parens fixed

Darn it, didn't save the spaces-fixed patch before submitting. This one is right.
Comment 33 Colin Walters 2011-08-22 17:42:40 UTC
Review of attachment 194313 [details] [review]:

I know the details were discussed here in the bug, but I'd like to at least see some attempt to summarize the changes in the commit message.   Following https://live.gnome.org/GnomeLove/SubmittingPatches the commit body must *at least* have a link to this bug.

The point is that someone looking 5 years later for *why* things changed will be able to find out - "statfs/statvfs fixes" doesn't cover it.

::: configure.ac
@@ +1010,3 @@
 # successfully compiles, but warns to use the newer statvfs interface:
 AS_IF([test $ac_cv_header_sys_statvfs_h = yes], [AC_CHECK_FUNCS([statvfs])])
+AS_IF([test $ac_cv_header_sys_statfs_h  = yes -o $ac_cv_header_sys_mount_h = yes], [AC_CHECK_FUNCS([statfs])])

Missing shell empty string hardening here.  You need to either include $ac_cv_header_sys_statfs_h in double quotes or add an x prefix.  Search for "xyes".

(Autoconf may be guaranteed to set it, but it's better to be consistent in our use of "test".

@@ +1157,3 @@
+# Check whether getmntinfo takes struct statfs or struct statvfs as an argument
+#
+AS_IF([test $ac_cv_func_getmntinfo = yes],

Ditto here.

@@ +1159,3 @@
+AS_IF([test $ac_cv_func_getmntinfo = yes],
+[AC_MSG_CHECKING([type of argument to getmntinfo])
+ AS_IF([test x$GCC = xyes],

Note this one is safe with the "x" prefix.

@@ +1163,3 @@
+        CFLAGS="-Werror $CFLAGS"])
+ AC_COMPILE_IFELSE(
+	[AC_LANG_PROGRAM(

This is kind of hard to read - you could split it up into separate sections, but I'm ok with how it is now too.

By split it up I mean:

AC_COMPILE_IFELSE(firstprog, have_foo=yes, have_foo=no)
if test x$have_foo = xno; then
  AC_COMPILE_IFELSE(...)
fi
Comment 34 Dan Winship 2011-08-27 16:22:59 UTC
(In reply to comment #19)
> BSD:
>         int getmntinfo(struct statvfs **mntbufp, int flags);
> Mac OS X:
>         int getmntinfo(struct statfs **mntbufp, int flags);

actually, apparently it's only NetBSD that uses statvfs.

Also, getmntinfo() is apparently just a wrapper around getfsstat() (or, on NetBSD, getvfsstat()) that leaks memory and fails to be threadsafe, for your convenience.

So rather than relying on -Werror to work to get the right result, a better solution would be:

    - test for the existence of getfsstat and getvfsstat instead of
      getmntinfo

    - Change _g_get_unix_mounts() to call getfsstat()/getvfsstat() with
      NULL, allocate an array of statfs/statvfs, call
      getfsstat()/getvfsstat() again with the right bufsize, process
      the results, and then free the array.
Comment 35 Patrick Welche 2011-09-01 14:26:53 UTC
Created attachment 195388 [details] [review]
use getstat{v,}fsstat for which problematic getmntinfo is a wrapper

Implement Dan Winship's good idea to use functions whose names differ, rather than a function which can take varying parameter types.
Comment 36 Patrick Welche 2011-09-01 14:31:15 UTC
Created attachment 195389 [details] [review]
harrden configure string tests

Style change: essentially

if test $ac_cv_var = yes   ----> if test x$ac_cv_var = xyes

throughout.

(I'm ambivalent about it myself: as noted in comment 33, I "hardened" the test for GCC above, as I thought it was set to yes if found, but may have been empty otherwise. I didn't "harden" the cached variables as, as also noted in comment 33, they are guaranteed to be set.)
Comment 37 Colin Walters 2011-09-01 20:34:21 UTC
These fail for me on Linux/glibc:

| /src/glib/gio/gunixmounts.c:191:23: fatal error: sys/ucred.h: No such file or directory
Comment 38 Dan Winship 2011-09-02 15:52:37 UTC
(In reply to comment #37)
> These fail for me on Linux/glibc:
> 
> | /src/glib/gio/gunixmounts.c:191:23: fatal error: sys/ucred.h: No such file or
> directory

-#if defined(HAVE_GETMNTINFO) && defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)
+#if defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)

needs to be something like

#if (defined(HAVE_GETVFSSTAT) || defined(HAVE_GETFSSTAT)) && defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)

(it compiles on Linux with that)
Comment 39 Patrick Welche 2011-09-02 17:41:07 UTC
Created attachment 195512 [details] [review]
use getstat{v,}fsstat for which problematic getmntinfo is a wrapper

Use

#if (defined(HAVE_GETVFSSTAT) || defined(HAVE_GETFSSTAT)) && defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)

everywhere HAVE_GETMNTINFO appeared. Essentially the above combination of defines becomes a flag for the way it is done on a particular system. I suppose automake conditionals are looking appealing...

(The 2nd patch ought to apply after this, and I tested it on NetBSD and ubuntu 10.04)
Comment 40 Dan Winship 2011-09-04 21:51:37 UTC
committed with a few minor fixes (in particular, the getfsstat()
systems still use MNT_NOWAIT, not ST_NOWAIT). Thanks for the patch.