diff mbox series

Have g++ define _FILE_OFFSET_BITS=64 on Solaris

Message ID yddy3f88br3.fsf@CeBiTec.Uni-Bielefeld.DE
State New
Headers show
Series Have g++ define _FILE_OFFSET_BITS=64 on Solaris | expand

Commit Message

Rainer Orth June 21, 2018, 2:17 p.m. UTC
I recently found two libstdc++ testcases failing on some Solaris hosts
for 32-bit only:

FAIL: 27_io/filesystem/operations/space.cc execution test
FAIL: experimental/filesystem/operations/space.cc execution test

Both file in the same way:

terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
  what():  filesystem error: cannot get free space: Value too large for defined data type [.]

However, the test PASSes just fine on other systems.

It turns out that the tests FAIL with

statvfs(".", 0xFEFFDB64)                        Err#79 EOVERFLOW

On the failing system, the build filesystem is 3.4 TB, thus the
EOVERFLOW.

It seems g++ on Solaris doesn't fully enable largefile support: it has
-D_LARGEFILE_SOURCE=1 in gcc/config/sol2.h (TARGET_OS_CPP_BUILTINS), but
lacks -D_FILE_OFFSET_BITS=64 which is required to get the
largefile-aware functions (statvfs64 in this case).

The following patch adds that, fixing the two failures.

Bootstrapped without regressions on i386-pc-solaris2.1[01] and
sparc-sun-solaris2.1[01].

Unless someone has an idea why this might cause problems, I'll install
the patch on mainline and backport to the gcc-7 and gcc-8 branches.

	Rainer

Comments

Jonathan Wakely June 21, 2018, 2:32 p.m. UTC | #1
On 21/06/18 16:17 +0200, Rainer Orth wrote:
>I recently found two libstdc++ testcases failing on some Solaris hosts
>for 32-bit only:
>
>FAIL: 27_io/filesystem/operations/space.cc execution test
>FAIL: experimental/filesystem/operations/space.cc execution test
>
>Both file in the same way:
>
>terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
>  what():  filesystem error: cannot get free space: Value too large for defined data type [.]
>
>However, the test PASSes just fine on other systems.
>
>It turns out that the tests FAIL with
>
>statvfs(".", 0xFEFFDB64)                        Err#79 EOVERFLOW
>
>On the failing system, the build filesystem is 3.4 TB, thus the
>EOVERFLOW.
>
>It seems g++ on Solaris doesn't fully enable largefile support: it has
>-D_LARGEFILE_SOURCE=1 in gcc/config/sol2.h (TARGET_OS_CPP_BUILTINS), but
>lacks -D_FILE_OFFSET_BITS=64 which is required to get the
>largefile-aware functions (statvfs64 in this case).
>
>The following patch adds that, fixing the two failures.
>
>Bootstrapped without regressions on i386-pc-solaris2.1[01] and
>sparc-sun-solaris2.1[01].
>
>Unless someone has an idea why this might cause problems, I'll install
>the patch on mainline and backport to the gcc-7 and gcc-8 branches.

No objection to this patch, but I'll just note that we have
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81091 suggesting we
should use LFS for libstdc++ unconditionally.
Rainer Orth June 21, 2018, 2:49 p.m. UTC | #2
Hi Jonathan,

> No objection to this patch, but I'll just note that we have
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81091 suggesting we
> should use LFS for libstdc++ unconditionally.

seems like a wise move to me.  The libstdc++.so ABI didn't change on
Solaris either (that possibility had caused concern for me initially);
didn't check libstdc++fs.a though.

	Rainer
Jonathan Wakely June 21, 2018, 2:53 p.m. UTC | #3
On 21/06/18 16:49 +0200, Rainer Orth wrote:
>Hi Jonathan,
>
>> No objection to this patch, but I'll just note that we have
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81091 suggesting we
>> should use LFS for libstdc++ unconditionally.
>
>seems like a wise move to me.  The libstdc++.so ABI didn't change on
>Solaris either (that possibility had caused concern for me initially);
>didn't check libstdc++fs.a though.

Well the main reason that's only a static library for now is to allow
us to make ABI incompatible changes before we declare it stable and
add those symbols to libstdc++.so forever.
Franz Sirl June 21, 2018, 3:17 p.m. UTC | #4
Am 2018-06-21 um 16:17 schrieb Rainer Orth:
> I recently found two libstdc++ testcases failing on some Solaris hosts
> for 32-bit only:
> 
> FAIL: 27_io/filesystem/operations/space.cc execution test
> FAIL: experimental/filesystem/operations/space.cc execution test
> 
> Both file in the same way:
> 
> terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
>    what():  filesystem error: cannot get free space: Value too large for defined data type [.]
> 
> However, the test PASSes just fine on other systems.
> 
> It turns out that the tests FAIL with
> 
> statvfs(".", 0xFEFFDB64)                        Err#79 EOVERFLOW
> 
> On the failing system, the build filesystem is 3.4 TB, thus the
> EOVERFLOW.
> 
> It seems g++ on Solaris doesn't fully enable largefile support: it has
> -D_LARGEFILE_SOURCE=1 in gcc/config/sol2.h (TARGET_OS_CPP_BUILTINS), but
> lacks -D_FILE_OFFSET_BITS=64 which is required to get the
> largefile-aware functions (statvfs64 in this case).
> 
> The following patch adds that, fixing the two failures.
> 
> Bootstrapped without regressions on i386-pc-solaris2.1[01] and
> sparc-sun-solaris2.1[01].
> 
> Unless someone has an idea why this might cause problems, I'll install
> the patch on mainline and backport to the gcc-7 and gcc-8 branches.

No idea about possible problems, but isn't it usually recommended to use 
either _FILE_OFFSET_BITS=64 or _LARGEFILE{64}_SOURCE=1, not both at the 
same time?

Franz
Rainer Orth June 22, 2018, 7:43 a.m. UTC | #5
Hi Jonathan,

> On 21/06/18 16:49 +0200, Rainer Orth wrote:
>>Hi Jonathan,
>>
>>> No objection to this patch, but I'll just note that we have
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81091 suggesting we
>>> should use LFS for libstdc++ unconditionally.
>>
>>seems like a wise move to me.  The libstdc++.so ABI didn't change on
>>Solaris either (that possibility had caused concern for me initially);
>>didn't check libstdc++fs.a though.
>
> Well the main reason that's only a static library for now is to allow
> us to make ABI incompatible changes before we declare it stable and
> add those symbols to libstdc++.so forever.

I suspected that much.  However, once you're reasonable confident the
interface is stable, usability would be much improved by moving into
libstdc++.so: e.g. before Solaris 11.4 sendfile lives in a separate
libsendfile.  This can easily be dealt with as a shared library
dependency, but is harder (or puts the burden on the user) for static
libstdc++fs.a.  One might thing about introducing libstdc++.spec which
could better deal with issues like this, just like libgfortran.spec.
I'd started to work on this long ago do get rid of the hardcoded -lrt in
g++, but never completed it.

	Rainer
Rainer Orth June 22, 2018, 7:51 a.m. UTC | #6
Hi Franz,

> No idea about possible problems, but isn't it usually recommended to use
> either _FILE_OFFSET_BITS=64 or _LARGEFILE{64}_SOURCE=1, not both at the
> same time?

quite the contrary: for regular largefile support, you're supposed to
use `getconf LFS_CFLAGS', i.e. -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64, while `getconf LFS64_CFLAGS'
(-D_LARGEFILE64_SOURCE) enables the transitional largefile interfaces
(e.g. explicit stat64 calls and struct stat64 instead of making stat and
struct stat largefile-aware).

For all the gory details, see the lfcompile(7), lfcompile64(7), and
lf64(7) man pages:

	https://docs.oracle.com/cd/E88353_01/html/E37853/index.html

	Rainer
Franz Sirl June 22, 2018, 10:08 a.m. UTC | #7
Am 2018-06-22 um 09:51 schrieb Rainer Orth:
> Hi Franz,
> 
>> No idea about possible problems, but isn't it usually recommended to use
>> either _FILE_OFFSET_BITS=64 or _LARGEFILE{64}_SOURCE=1, not both at the
>> same time?
> 
> quite the contrary: for regular largefile support, you're supposed to
> use `getconf LFS_CFLAGS', i.e. -D_LARGEFILE_SOURCE
> -D_FILE_OFFSET_BITS=64, while `getconf LFS64_CFLAGS'
> (-D_LARGEFILE64_SOURCE) enables the transitional largefile interfaces
> (e.g. explicit stat64 calls and struct stat64 instead of making stat and
> struct stat largefile-aware).
> 
> For all the gory details, see the lfcompile(7), lfcompile64(7), and
> lf64(7) man pages:
> 
> 	https://docs.oracle.com/cd/E88353_01/html/E37853/index.html
> 
> 	Rainer
> 

Hi Rainer,

so you are supposed to use "-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64", 
but at least a quick glance at the Sol10 headers shows that the 
additional -D_LARGEFILE_SOURCE only makes a difference for 
fseeko/ftello. That still doesn't explain -D_LARGEFILE64_SOURCE, does 
libstdc++ really need to use _LARGEFILE64_SOURCE functions?

Re-reading lfcompile(7) again shows that you can use either 
"-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64" (for portable applications) 
or only "-D_FILE_OFFSET_BITS=64". But in the GCC case we only need it 
for C++/libstdc++ so it seems "-D_FILE_OFFSET_BITS=64" should be enough. 
The rest is up to the users application, or?
My guess is that without defining _LARGEFILE_SOURCE and 
_LARGEFILE64_SOURCE the configure check in libstdc++-v3/acinclude.m4 
just won't define _GLIBCXX_USE_LFS and everything will fall in place. 
This would leave HPUX as the last user of _GLIBCXX_USE_LFS.

Franz
Rainer Orth June 25, 2018, 1:57 p.m. UTC | #8
Hi Franz,

> so you are supposed to use "-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64",
> but at least a quick glance at the Sol10 headers shows that the additional
> -D_LARGEFILE_SOURCE only makes a difference for fseeko/ftello. That still

right, that's also explained in lfcompile(7).

> doesn't explain -D_LARGEFILE64_SOURCE, does libstdc++ really need to use
> _LARGEFILE64_SOURCE functions?

Honestly, I hadn't checked, but wondered the same thing.  However, I'm a
bit wary to simply remove them after years for fear of breaking user
existing user code.

> Re-reading lfcompile(7) again shows that you can use either
> "-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64" (for portable applications) or
> only "-D_FILE_OFFSET_BITS=64". But in the GCC case we only need it for
> C++/libstdc++ so it seems "-D_FILE_OFFSET_BITS=64" should be enough. The
> rest is up to the users application, or?

One might argue that way, but again it's a bit late to change this now
for no compelling reason.

> My guess is that without defining _LARGEFILE_SOURCE and _LARGEFILE64_SOURCE
> the configure check in libstdc++-v3/acinclude.m4 just won't define
> _GLIBCXX_USE_LFS and everything will fall in place. This would leave HPUX
> as the last user of _GLIBCXX_USE_LFS.

I don't know about HP-UX, but _GLIBC_USE_LFS is defined on Linux/x86_64,
too.  To me, the meaning seems a bit confused: 27_io/fpos/14775.cc
suggests that it denotes all system with largefile support, while
acinclude.m4 (GLIBCXX_CHECK_LFS) only tests for the transitional
functions (enabled by _LARGEFILE64_SOURCE on Solaris) while ignoring the
`transparent' largefile support from _LARGEFILE_SOURCE
_FILE_OFFSET_BITS=64.

I'd rather not mess with this stuff.

	Rainer
Franz Sirl June 25, 2018, 4:15 p.m. UTC | #9
Am 2018-06-25 um 15:57 schrieb Rainer Orth:
> Hi Franz,
> 
>> so you are supposed to use "-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64",
>> but at least a quick glance at the Sol10 headers shows that the additional
>> -D_LARGEFILE_SOURCE only makes a difference for fseeko/ftello. That still
> 
> right, that's also explained in lfcompile(7).
> 
>> doesn't explain -D_LARGEFILE64_SOURCE, does libstdc++ really need to use
>> _LARGEFILE64_SOURCE functions?
> 
> Honestly, I hadn't checked, but wondered the same thing.  However, I'm a
> bit wary to simply remove them after years for fear of breaking user
> existing user code.

But adding _FILE_OFFSET_BITS=64 is the far bigger change for the user. 
Now suddenly (for 32-bit applications) off_t changes size and thus many 
applications with mixed C/C++-code simply might break. The reason is 
that now (if they didn't take care of _LARGEFILE_SOURCE themselves), for 
example fread() really does a fread64() in the C++ parts and a fread() 
(the 32-bit version) in the C parts. This situation was avoided before 
by enabling _LARGEFILE_SOURCE without _FILE_OFFSET_BITS=64.

>> Re-reading lfcompile(7) again shows that you can use either
>> "-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64" (for portable applications) or
>> only "-D_FILE_OFFSET_BITS=64". But in the GCC case we only need it for
>> C++/libstdc++ so it seems "-D_FILE_OFFSET_BITS=64" should be enough. The
>> rest is up to the users application, or?
> 
> One might argue that way, but again it's a bit late to change this now
> for no compelling reason.

The compelling reason is that with _FILE_OFFSET_BITS=64 all bets are off 
anyway IMO, see above.

>> My guess is that without defining _LARGEFILE_SOURCE and _LARGEFILE64_SOURCE
>> the configure check in libstdc++-v3/acinclude.m4 just won't define
>> _GLIBCXX_USE_LFS and everything will fall in place. This would leave HPUX
>> as the last user of _GLIBCXX_USE_LFS.
> 
> I don't know about HP-UX, but _GLIBC_USE_LFS is defined on Linux/x86_64,
> too.  To me, the meaning seems a bit confused: 27_io/fpos/14775.cc
> suggests that it denotes all system with largefile support, while
> acinclude.m4 (GLIBCXX_CHECK_LFS) only tests for the transitional
> functions (enabled by _LARGEFILE64_SOURCE on Solaris) while ignoring the
> `transparent' largefile support from _LARGEFILE_SOURCE
> _FILE_OFFSET_BITS=64.
> 
> I'd rather not mess with this stuff.

That I can fully understand ;-). Maybe a solution is to define the 
macros also for C, not only for C++. And to conditionalize 
_LARGEFILE_SOURCE on 32-bit compile and _LARGEFILE64_SOURCE on 64-bit 
compile to make it at least less confusing.

Franz
diff mbox series

Patch

# HG changeset patch
# Parent  48a63094f075d53e7bbbe0f2de0513c267ef9e96
Have g++ define _FILE_OFFSET_BITS=64 on 32-bit Solaris

diff --git a/gcc/config/sol2.h b/gcc/config/sol2.h
--- a/gcc/config/sol2.h
+++ b/gcc/config/sol2.h
@@ -113,6 +113,7 @@  along with GCC; see the file COPYING3.  
 	builtin_define ("_XOPEN_SOURCE=600");		\
 	builtin_define ("_LARGEFILE_SOURCE=1");		\
 	builtin_define ("_LARGEFILE64_SOURCE=1");	\
+	builtin_define ("_FILE_OFFSET_BITS=64");	\
 	builtin_define ("__EXTENSIONS__");		\
       }							\
     TARGET_SUB_OS_CPP_BUILTINS();			\