[Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[ro]

Summary

make check-all currently fails on x86_64-pc-solaris2.11 when building with GCC 9:

Undefined                       first referenced
 symbol                             in file
_ZN11__sanitizer14internal_lseekEimi SANITIZER_TEST_OBJECTS.sanitizer_libc_test.cc.i386.o
_ZN11__sanitizer23MapWritableFileToMemoryEPvmim SANITIZER_TEST_OBJECTS.sanitizer_libc_test.cc.i386.o
ld: fatal: symbol referencing errors
clang-9: error: linker command failed with exit code 1 (use -v to see invocation)
make[3]: *** [projects/compiler-rt/lib/sanitizer_common/tests/CMakeFiles/TSanitizer-i386-Test.dir/build.make:92: projects/compiler-rt/lib/sanitizer_common/tests/Sanitizer-i386-Test] Error 1

While e.g. __sanitizer::internal_lseek is defined in sanitizer_solaris.cc, g++ 9
predefines _FILE_OFFSET_BITS=64 while clang++ currently does not.

This patch resolves this inconsistency by following the gcc lead, which allows
make check-all to finish successfully.

There's one caveat: gcc defines _LARGEFILE_SOURCE and _LARGEFILE64_SOURCE for C++ only, while clang has long been doing it for
all languages. I'd like to keep it this way because those macros do is to make
declarations of fseek/ftello (_LARGEFILE_SOURCE) resp. the 64-bit versions
of largefile functions (*64 with _LARGEFILE64_SOURCE) visible additionally.
However, _FILE_OFFSET_BITS=64 changes all affected functions to be largefile-aware.
I'd like to restrict this to C++, just like gcc does.

To avoid a similar inconsistence with host compilers that don't predefine _FILE_OFFSET_BITS=64
(e.g. clang < 9, gcc < 9), this needs a compantion patch to be submitted shortly.

Tested on x86_64-pc-solaris2.11. Ok for trunk?

[MaskRay]

There's one caveat: gcc defines _LARGEFILE_SOURCE and _LARGEFILE64_SOURCE for C++ only, while clang has long been doing it for all languages

Can you explain more about the hack https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=0f97ccfdccc033f543ccbcb220697e62e84bf01f

[ro]

In D64482#1578245, @MaskRay wrote:

There's one caveat: gcc defines _LARGEFILE_SOURCE and _LARGEFILE64_SOURCE for C++ only, while clang has long been doing it for all languages

Can you explain more about the hack https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=0f97ccfdccc033f543ccbcb220697e62e84bf01f

No hack at all ;-) See the patch submission https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01320.html for details. Apart from that,
this is the direction libstdc++/g++ mean to take in general.

[MaskRay]

In D64482#1578376, @ro wrote:

In D64482#1578245, @MaskRay wrote:

There's one caveat: gcc defines _LARGEFILE_SOURCE and _LARGEFILE64_SOURCE for C++ only, while clang has long been doing it for all languages

Can you explain more about the hack https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=0f97ccfdccc033f543ccbcb220697e62e84bf01f

No hack at all ;-) See the patch submission https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01320.html for details. Apart from that,
this is the direction libstdc++/g++ mean to take in general.

Honestly I find such enforced C/C++ difference very unfortunate... e.g.

if (Opts.CPlusPlus)
  Builder.defineMacro("_GNU_SOURCE");

Solaris seems the only exception that defines these Large File Support extension macros on the compiler driver side. Isn't it possible to do it in a common system header file?

That rationale will be better than this paragraph in the description:

make check-all currently fails on x86_64-pc-solaris2.11 when building with GCC 9:

With the current description, a casual reader (like I) would just think this patch is probably not doing things at the correct layer.

[ro]

In D64482#1579850, @MaskRay wrote:

Honestly I find such enforced C/C++ difference very unfortunate... e.g.

if (Opts.CPlusPlus)
  Builder.defineMacro("_GNU_SOURCE");

You may not like them, but there are plenty of examples in OSTargets.h (for kFreeBSD, Hurd, Linux, RTEMS, AIX, Windows, NaCl and
several more). Why take offense in the Solaris case if this is already common practice?

Solaris seems the only exception that defines these Large File Support extension macros on the compiler driver side. Isn't it possible to do it in a common system header file?

Even if it were, this would only affect future releases. The user experience of "you need to upgrade to Solaris 11.x" or install update y
to get this" seems pretty dismal to me. Besides, that ship has sailed and GCC 9 is released.

Apart from that, I strongly suspect that there are other reasons. Large file support is only relevant for 32-bit targets. However, many
OSes have switched to 64-bit only by now. And testing non-default multilibs just doesn't happen in LLVM: I haven't found a way to do
so yet, while it's quite easy to run all gcc testsuites for a common set of multilibs. Even building LLVM for Linux/i686 or Solaris/i386
proved to be highly problematic and not (regularly) tested.

That rationale will be better than this paragraph in the description:

make check-all currently fails on x86_64-pc-solaris2.11 when building with GCC 9:

With the current description, a casual reader (like I) would just think this patch is probably not doing things at the correct layer.

That description only served to show what problem as hand is being solved, not just an abstract desire for g++ compatibiliy.

[ro]

Ping? It's been a week and it would be good to get this patch and its companion into LLVM 9: it unbreaks make check-all with gcc 9
and restores g++ compatibility.

[rnk]

To avoid a similar inconsistence with host compilers that don't predefine _FILE_OFFSET_BITS=64
(e.g. clang < 9, gcc < 9), this needs a compantion patch to be submitted shortly.

I'm curious, what's the plan for that? I suppose the user can always take things into their own hands with -D and -U.

In any case, if you want to match newer GCCs, I'm for that. lgtm

[ro]

In D64482#1588154, @rnk wrote:

To avoid a similar inconsistence with host compilers that don't predefine _FILE_OFFSET_BITS=64
(e.g. clang < 9, gcc < 9), this needs a compantion patch to be submitted shortly.

I'm curious, what's the plan for that? I suppose the user can always take things into their own hands with -D and -U.

The companion patch https://reviews.llvm.org/D64483 unconditionally predefines _FILE_OFFSET_BITS=64 on Solaris, irrespective
of compiler. It still needs approval and should go in first, otherwise we trade the failure with a gcc 9 host compiler for a failure with
older gcc and clang (and this also would break the Solaris buildbots, which currently use gcc 7).

While users could use -D/-U by themselves to select the largefile support they want, I wouldn't rely too much on that, with parts of
libstdc++ in headers and the rest in the shared object. Mixing code with and without largefile support in the same executable works,
of course, but the results may be surprising...

[MaskRay]

You may not like them, but there are plenty of examples in OSTargets.h (for kFreeBSD, Hurd, Linux, RTEMS, AIX, Windows, NaCl and several more). Why take offense in the Solaris case if this is already common practice?

I used that as an example. Defining _GNU_SOURCE was a mistake. Making it different from C was another mistake. It is very unfortunate that it is too late to fix them.

Even if it were, this would only affect future releases. The user experience of "you need to upgrade to Solaris 11.x" or install update y to get this" seems pretty dismal to me. Besides, that ship has sailed and GCC 9 is released.

Defining _LARGEFILE_SOURCE, _LARGEFILE64_SOURCE and _FILE_OFFSET_BITS on the compiler side is exclusively used by Solaris. Do you mean that newer Solaris versions may define these macros in the common headers and these macros can eventually be removed from compiler drivers?

If these are considered temporary hacks to make some application to compile on older Solaris versions, I think the comment should be expanded a bit.

Large file support is only relevant for 32-bit targets.

Yes. https://gcc.gnu.org/git/?p=gcc.git;a=blobdiff;f=gcc/config/sol2.h;h=bc4f63df03bc8e0b5bceee4e52f48240de7f993d;hp=ec4b111ba0e3cdf56970073959d228fbafe8937d;hb=0f97ccfdccc033f543ccbcb220697e62e84bf01f;hpb=ee621ce77125904f7946ba6cef345cb83e48248d and the previous commits should probably restrict the scope of the hack to 32-bit only.

[ro]

In D64482#1589174, @MaskRay wrote:

Even if it were, this would only affect future releases. The user experience of "you need to upgrade to Solaris 11.x" or install update y to get this" seems pretty dismal to me. Besides, that ship has sailed and GCC 9 is released.

Defining _LARGEFILE_SOURCE, _LARGEFILE64_SOURCE and _FILE_OFFSET_BITS on the compiler side is exclusively used by Solaris. Do you mean that newer Solaris versions may define these macros in the common headers and these macros can eventually be removed from compiler drivers?

No, certainly not: it has been this way in gcc since at least 2002 and is not going to change. Besides, HP-UX 10 and 11 do the same.

If these are considered temporary hacks to make some application to compile on older Solaris versions, I think the comment should be expanded a bit.

It's not and thus going to stay.

Large file support is only relevant for 32-bit targets.

Yes. https://gcc.gnu.org/git/?p=gcc.git;a=blobdiff;f=gcc/config/sol2.h;h=bc4f63df03bc8e0b5bceee4e52f48240de7f993d;hp=ec4b111ba0e3cdf56970073959d228fbafe8937d;hb=0f97ccfdccc033f543ccbcb220697e62e84bf01f;hpb=ee621ce77125904f7946ba6cef345cb83e48248d and the previous commits should probably restrict the scope of the hack to 32-bit only.

There's no point: _FILE_OFFSET_BITS has no effect when _LP64 is defined, so such an effort would be wasted.

lib/Basic/Targets/OSTargets.h

void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
     // Solaris headers require _XOPEN_SOURCE to be set to 600 for C99 and
     // newer, but to 500 for everything else.  feature_test.h has a check to
     // ensure that you are not using C99 with an old version of X/Open or C89
     // with a new version.
     if (Opts.C99)
       Builder.defineMacro("_XOPEN_SOURCE", "600");
     else
       Builder.defineMacro("_XOPEN_SOURCE", "500");
-    if (Opts.CPlusPlus)
+    if (Opts.CPlusPlus) {
       Builder.defineMacro("__C99FEATURES__");
+      Builder.defineMacro("_FILE_OFFSET_BITS", "64");
+    }
+    // GCC restricts the next two to C++.
     Builder.defineMacro("_LARGEFILE_SOURCE");
     Builder.defineMacro("_LARGEFILE64_SOURCE");
     Builder.defineMacro("__EXTENSIONS__");
     if (Opts.POSIXThreads)
       Builder.defineMacro("_REENTRANT");
     if (this->HasFloat128)
       Builder.defineMacro("__FLOAT128__");
   }