Page MenuHomePhabricator

[CMake] Define _FILE_OFFSET_BITS=64 on Solaris
ClosedPublic

Authored by ro on Jul 10 2019, 6:36 AM.

Details

Summary

This is the compantion patch to https://reviews.llvm.org/D64482, needed to ensure
that builds with host compilers that don't yet predefine _FILE_OFFSET_BITS=64 on
Solaris succeed by always making the host and freshly built clang consistent.

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

Diff Detail

Repository
rL LLVM

Event Timeline

ro created this revision.Jul 10 2019, 6:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 6:36 AM
Herald added a subscriber: mgorny. · View Herald Transcript

Ping? It's been a week and together with it's clang companion this patch is needed to unbreak make check-all with gcc 9. So it would
be good to get this into LLVM 9.

rnk added a subscriber: rnk.Jul 17 2019, 1:10 PM

I would place this macro setting with the other code that sets this macro, in cmake/modules/HandleLLVMOptions.cmake.

ro added a comment.Jul 18 2019, 6:45 AM
In D64483#1590120, @rnk wrote:

I would place this macro setting with the other code that sets this macro, in cmake/modules/HandleLLVMOptions.cmake.

This doesn't work unfortunately: I still get the same link failure for Sanitizer-i386-Test with gcc 9. For the original patch I'd followed how things are done
for AIX (_XOPEN_SOURCE=700, _LARGE_FILE_API) which is similar.

ro added a comment.Jul 23 2019, 4:15 AM

How should we procede here, given the proposed alternative doesn't work? It would be good to get this and the already approved
companion patch into trunk and, if possible, into the 9.x branch.

ro added a comment.Mon, Jul 29, 6:22 AM

Ping^2? It's been two weeks now and I'd like to know how to proceed with this patch.

hans accepted this revision.Tue, Jul 30, 1:38 AM

Seems fine I guess, since this matches what's already there for AIX.

This revision is now accepted and ready to land.Tue, Jul 30, 1:38 AM
ro added a comment.Tue, Jul 30, 2:22 AM

Seems fine I guess, since this matches what's already there for AIX.

Thanks. Would this and its companion D64482 be appropriate for the 9.x branch, too? If so, what's the procedure to get it there?

hans added a comment.Tue, Jul 30, 2:24 AM

Thanks. Would this and its companion D64482 be appropriate for the 9.x branch, too? If so, what's the procedure to get it there?

Probably. They need to land on trunk first, then let me know and I'll merge them.

This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
llvm/trunk/CMakeLists.txt
837

Doesn't ${CMAKE_SYSTEM_NAME} MATCHES "SunOS" imply UNIX?

ro marked 2 inline comments as done.Tue, Jul 30, 4:38 AM
ro added inline comments.
llvm/trunk/CMakeLists.txt
837

I guess so: just copied it from the AIX clause above.

ro marked an inline comment as done.Tue, Jul 30, 4:41 AM

Thanks. Would this and its companion D64482 be appropriate for the 9.x branch, too? If so, what's the procedure to get it there?

Probably. They need to land on trunk first, then let me know and I'll merge them.

Both patches are in now. However, there's a buildbot failure
that claims to be related, though I cannot see yet how this can possibly happen.

ro added a comment.Tue, Jul 30, 6:41 AM
In D64483#1605989, @ro wrote:

Thanks. Would this and its companion D64482 be appropriate for the 9.x branch, too? If so, what's the procedure to get it there?

Probably. They need to land on trunk first, then let me know and I'll merge them.

Both patches are in now. However, there's a buildbot failure
that claims to be related, though I cannot see yet how this can possibly happen.

The next builds of both affected buildbots [[http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast] and http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux
are fine again, so this seem to have been transient failures unrelated to my patches.

From the POV, they seem good to go for a backport.

rnk added a comment.Tue, Jul 30, 1:09 PM
In D64483#1591428, @ro wrote:
In D64483#1590120, @rnk wrote:

I would place this macro setting with the other code that sets this macro, in cmake/modules/HandleLLVMOptions.cmake.

This doesn't work unfortunately: I still get the same link failure for Sanitizer-i386-Test with gcc 9. For the original patch I'd followed how things are done
for AIX (_XOPEN_SOURCE=700, _LARGE_FILE_API) which is similar.

I don't understand how that could be, but I can't investigate since I don't have solaris set up. HandleLLVMOptions.cmake is included from llvm/CMakeLists.txt right above the code you are modifying, and it calls add_definitions as well, so it should work exactly the same.

hans added a comment.Thu, Aug 1, 1:49 AM
In D64483#1606208, @ro wrote:
In D64483#1605989, @ro wrote:

Thanks. Would this and its companion D64482 be appropriate for the 9.x branch, too? If so, what's the procedure to get it there?

Probably. They need to land on trunk first, then let me know and I'll merge them.

Both patches are in now. However, there's a buildbot failure
that claims to be related, though I cannot see yet how this can possibly happen.

The next builds of both affected buildbots [[http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast] and http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux
are fine again, so this seem to have been transient failures unrelated to my patches.

From the POV, they seem good to go for a backport.

Yes, I've merged both in r367526 and r367527, respectively.

ro added a comment.Fri, Aug 2, 1:50 AM
In D64483#1606208, @ro wrote:

From the POV, they seem good to go for a backport.

Yes, I've merged both in r367526 and r367527, respectively.

Excellent, thanks.