This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][AIX] Switch build compiler to clang
ClosedPublic

Authored by Jake-Egan on Jun 9 2022, 8:24 PM.

Details

Reviewers
daltenty
ldionne
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rG1cf4113952ae: [libcxx][AIX] Switch build compiler to clang
Summary

This patch switches the build compiler for AIX from ibm-clang to clang. ibm-clang++_r has -pthread by default, but clang for AIX doesn't, so -pthread had to be added to the test config. A bunch of tests now pass, so the XFAIL was removed. This patch also switch the build to use the visibility support available in clang-15 to control symbols exported by the shared library (AIX traditionally uses explicit export lists for this purpose).

Diff Detail

Event Timeline

Jake-Egan created this revision.Jun 9 2022, 8:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 8:24 PM
Jake-Egan requested review of this revision.Jun 9 2022, 8:24 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 9 2022, 8:24 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Jake-Egan edited the summary of this revision. (Show Details)Jun 9 2022, 8:34 PM
Jake-Egan added a reviewer: daltenty.
Jake-Egan updated this revision to Diff 435785.Jun 9 2022, 8:40 PM
Jake-Egan edited the summary of this revision. (Show Details)

Fixed typo

Jake-Egan updated this revision to Diff 435959.Jun 10 2022, 9:59 AM

Rebuild CI

Jake-Egan updated this revision to Diff 436093.Jun 10 2022, 8:44 PM

Set ar path to hopefully fix the CI (llvm-ar not recognizing -X option)

Jake-Egan updated this revision to Diff 436130.Jun 11 2022, 5:36 AM
This comment was removed by Jake-Egan.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2022, 5:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
daltenty added inline comments.Jun 13 2022, 7:23 AM
libcxx/CMakeLists.txt
472

Would setting this in our AIX CMake cache file be sufficient? That might be preferable to hardcoding part of the tool chain in each of the top-level CMake files.

ldionne accepted this revision.Jun 13 2022, 7:53 AM
ldionne added a subscriber: ldionne.

LGTM with comments addressed.

libcxx/CMakeLists.txt
472

Yes, please do it in the CMake cache instead to avoid adding platform-specific code to the CMakeLists.txt.

libcxxabi/CMakeLists.txt
232

Same comment here.

libunwind/CMakeLists.txt
146 ↗(On Diff #436130)

And here.

This revision is now accepted and ready to land.Jun 13 2022, 7:53 AM
Jake-Egan updated this revision to Diff 436424.Jun 13 2022, 8:45 AM

Use CMake cache to set ar path instead.

Jake-Egan marked 3 inline comments as done.Jun 13 2022, 8:47 AM
daltenty accepted this revision.Jun 13 2022, 10:04 AM

LGTM, with minor nit

libcxx/include/__config
131

nit: It's probably worth a comment to explain this:

daltenty edited the summary of this revision. (Show Details)Jun 13 2022, 10:06 AM

Added requested comment.

Jake-Egan marked an inline comment as done.Jun 13 2022, 10:41 AM
Jake-Egan updated this revision to Diff 436571.Jun 13 2022, 2:38 PM

Add CACHE FILEPATH to CMAKE_AR.

This revision was automatically updated to reflect the committed changes.