[libcxx-commits] [PATCH] D114805: [NFC][libc++] Recognize int128 as an ABI affecting property

[David Tenty via Phabricator via libcxx-commits]

daltenty added a comment.

> 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*)





================
Comment at: libcxx/CMakeLists.txt:315
+check_cxx_symbol_exists(__SIZEOF_INT128__ "" LIBCXX_HAVE_INT128)
+cmake_dependent_option(LIBCXX_USE_INT128   "Use __int128 in the library" ON
+"LIBCXX_HAVE_INT128" OFF)
----------------
ldionne wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114805