[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Rainer Orth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367305: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris 
(authored by ro, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64482?vs=208935=212310#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482

Files:
  cfe/trunk/lib/Basic/Targets/OSTargets.h


Index: cfe/trunk/lib/Basic/Targets/OSTargets.h
===
--- cfe/trunk/lib/Basic/Targets/OSTargets.h
+++ cfe/trunk/lib/Basic/Targets/OSTargets.h
@@ -622,8 +622,11 @@
   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__");


Index: cfe/trunk/lib/Basic/Targets/OSTargets.h
===
--- cfe/trunk/lib/Basic/Targets/OSTargets.h
+++ cfe/trunk/lib/Basic/Targets/OSTargets.h
@@ -622,8 +622,11 @@
   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__");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> 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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

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...


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

> 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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Rainer Orth via Phabricator via cfe-commits]

ro added reviewers: mehdi_amini, rnk.
ro added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> 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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64482/new/

https://reviews.llvm.org/D64482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64482: [Driver] Define _FILE_OFFSET_BITS=64 on Solaris

[Rainer Orth via Phabricator via cfe-commits]

ro created this revision.
ro added reviewers: fedor.sergeev, rsmith.
ro added a project: clang.

`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?


Repository:
  rC Clang

https://reviews.llvm.org/D64482

Files:
  lib/Basic/Targets/OSTargets.h


Index: lib/Basic/Targets/OSTargets.h
===
--- lib/Basic/Targets/OSTargets.h
+++ lib/Basic/Targets/OSTargets.h
@@ -618,8 +618,11 @@
   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__");


Index: lib/Basic/Targets/OSTargets.h
===
--- lib/Basic/Targets/OSTargets.h
+++ lib/Basic/Targets/OSTargets.h
@@ -618,8 +618,11 @@
   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__");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits