This is an archive of the discontinued LLVM Phabricator instance.

[NFC][libc++] Recognize int128 as an ABI affecting property
Needs ReviewPublic

Authored by daltenty on Nov 30 2021, 8:12 AM.

Details

Reviewers
ldionne
xingxue
sfertile
Group Reviewers
Restricted Project
Summary

Whether we have __int128 support in the compiler affects the resulting
library ABI, but there are a few places where we currently don't
reflect this.

Firstly, __config dynamically detects _LIBCPP_HAS_NO_INT128 in the
public headers. This could generate inconsistent results with how the
library was built, if the user uses a compiler with __int128 support
while the library was build without. This change moves
_LIBCPP_HAS_NO_INT128 to __config_site and introduces an option
LIBCXX_USE_INT128 that allows toggling __int128 usage even if it is
available in the build compiler.

Secondly, we add LIBCXX_USE_INT128 to the abi affecting properties for
abilists and update the extension of the existing lists. This will allow
us to generate consistent abis lists on platforms where some compilers
may support __int128 and others don't (e.g. AIX).

Diff Detail

Event Timeline

daltenty created this revision.Nov 30 2021, 8:12 AM
daltenty edited the summary of this revision. (Show Details)Jan 26 2022, 7:30 AM
daltenty edited the summary of this revision. (Show Details)
daltenty published this revision for review.Jan 26 2022, 7:34 AM
daltenty added reviewers: ldionne, xingxue, sfertile.
daltenty added a subscriber: jsji.
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2022, 7:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
daltenty edited the summary of this revision. (Show Details)Jan 26 2022, 7:34 AM

Gentle ping

Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 8:36 AM

I trust that you're right when you say it affects the ABI, but do you have an example (I'm sure there's many, I just can't think of one right now).

Also, can you please rebase and re-upload to poke the CI? I have a few comments but I think this makes sense.

libcxx/CMakeLists.txt
330

I don't think we should allow this to be set explicitly during CMake configuration. If it's an ABI affecting property of the compiler, I believe we should just detect it and set/unset it unconditionally.

I can't imagine building libc++ with one compiler and using it with a compiler so different that it would differ in whether it supports int128. For example, compiling libc++ with a version of Clang and using the headers with a different version of Clang clearly works, however I wouldn't dare ship a library compiled with e.g. GCC to my users. Not saying it won't work, but it seems like living on the edge -- enough that I don't think it's worth supporting. WDYT?

I trust that you're right when you say it affects the ABI, but do you have an example (I'm sure there's many, I just can't think of one right now).

The problem seems to arise primarily with the implementation of std::chrono::file_clock.

Build with compiler without int128 support:

_ZNSt3__14__fs10filesystem17__last_write_timeERKNS1_4pathENS_6chrono10time_pointINS1_16_FilesystemClockENS5_8durationIxNS_5ratioILl1ELl1000000000EEEEEEEPNS_10error_codeE
std::__1::__fs::filesystem::__last_write_time(std::__1::__fs::filesystem::path const&, std::__1::chrono::time_point<std::__1::__fs::filesystem::_FilesystemClock, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000000000l> > >, std::__1::error_code*)

Build with int128 support:

_ZNSt3__14__fs10filesystem17__last_write_timeERKNS1_4pathENS_6chrono10time_pointINS1_16_FilesystemClockENS5_8durationInNS_5ratioILl1ELl1000000000EEEEEEEPNS_10error_codeE
std::__1::__fs::filesystem::__last_write_time(std::__1::__fs::filesystem::path const&, std::__1::chrono::time_point<std::__1::__fs::filesystem::_FilesystemClock, std::__1::chrono::duration<__int128, std::__1::ratio<1l, 1000000000l> > >, std::__1::error_code*)
libcxx/CMakeLists.txt
330

The only problem I see with taking the our direction from the build compiler is it doesn't even require something as esoteric as the example of mixing compilers you mentioned. If a compiler transitions in support for the type in a new release, you'd have no way to easily move up the build compiler while still selecting compatibility with the existing ABI.

For example, on AIX clang's support for __int128 only recently landed in https://reviews.llvm.org/D111078 for 64-bit, so clang-13 (and thus Open XL 17.1) don't support it, but clang-14 will.

That said, since the interfaces involving __int128 are relatively new and limited to C++ 20 we might still have some wiggle room about which ABI to choose on AIX, but it still seems better to give the configuration choice in case we aren't the only ones with this problem going forward.

daltenty updated this revision to Diff 413193.Mar 4 2022, 8:41 PM

Rebase to trigger CI

daltenty updated this revision to Diff 413529.Mar 7 2022, 9:41 AM

Rename remaining ABI lists

ldionne added inline comments.Mar 7 2022, 10:14 AM
libcxx/CMakeLists.txt
330

Okay, I guess this makes sense. Could you also update the AIX.cmake cache accordingly? And also we should be running the check-cxx-abilist target on AIX (I assume we aren't right now). I think this patch would make more sense if we did.

Is there still interest in shipping this patch? If so, let's rebase it, otherwise let's abandon it to clear the Phabricator queue.