Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libcxx][libc] Hand in Hand PoC with from_chars #91651

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelrj-google
Copy link
Contributor

@michaelrj-google michaelrj-google commented May 9, 2024

Implements std::from_chars for float and double.

The implementation uses LLVM-libc to do the real parsing. Since this is the first time libc++
uses LLVM-libc there is a bit of additional infrastructure code. The patch is based on the
[RFC] Project Hand In Hand (LLVM-libc/libc++ code sharing)
https://discourse.llvm.org/t/rfc-project-hand-in-hand-llvm-libc-libc-code-sharing/77701

DO NOT MERGE, PROOF OF CONCEPT ONLY.

This patch aims to demonstrate the utility of sharing code between libc
and libc++ by using the libc float conversion code in the libc++
function from_chars. This patch adds from_chars for float and double
(long double is possible but was causing errors so was skipped here), as
well as a test to demonstrate that it works.

This is very much just a proof of concept, not intended to be committed
as-is. The from_chars code written is copied from the libc parsing code
and is not functionally complete, nor does it follow the correct coding
style.
@cjdb
Copy link
Contributor

cjdb commented May 9, 2024

Thanks for working on this! Let's see what CI has to say about its prospects, and then kick off a design conversation on Discourse?

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is quite promising! I left some comments but I think we can figure out a way to make this work.

libcxx/include/__charconv/from_chars_floating_point.h Outdated Show resolved Hide resolved
libcxx/include/__charconv/from_chars_floating_point.h Outdated Show resolved Hide resolved
libcxx/include/charconv Show resolved Hide resolved
libcxx/src/charconv.cpp Outdated Show resolved Hide resolved
@@ -197,7 +198,7 @@ split_list(LIBCXX_LINK_FLAGS)
# Build the shared library.
if (LIBCXX_ENABLE_SHARED)
add_library(cxx_shared SHARED ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
target_include_directories(cxx_shared PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
target_include_directories(cxx_shared PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/../../libc) #TODO: Do this properly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do something like this instead:

#
# runtimes/cmake/Modules/FindLibcUtils.cmake
#

# This file sets up an interface target called e.g. libc::shared-utilities that we can link against from any of the runtimes
# to get the functionality implemented in it. Concretely, we'd use it basically from libcxx/src/CMakeLists.txt as:

include(FindLibcUtils)
target_link_libraries(cxx_shared PRIVATE libc::shared-utilities) # This probably/hopefully only adds a `-I` flag.

# Possible implementation of this file:

add_library(libc::shared-utilities INTERFACE)
target_include_directories(libc::shared-utilities INTERFACE <path-to-shared-utilities>)
target_compile_features(libc::shared-utilities INTERFACE cxx_std_11) # for example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a file that does this, it's not the cleanest solution but it's the best I've got for the moment.

@mordante
Copy link
Member

Thanks for the review @ldionne. I've just spoken with @michaelrj-google and I'll work on the libc++ part of this patch.

Copy link

github-actions bot commented Aug 5, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor Author

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for working on this with me, the code you've added looks good so far

libcxx/src/include/from_chars_floating_point.h Outdated Show resolved Hide resolved
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still some work to be done on this patch, but I really like where this is going. I think the difficult part (the boundary between libc++ and libc) is very clean with this approach so I am quite happy with the patch.

libcxx/src/CMakeLists.txt Outdated Show resolved Hide resolved
runtimes/cmake/Modules/FindLibcUtils.cmake Outdated Show resolved Hide resolved
runtimes/cmake/Modules/FindLibcUtils.cmake Outdated Show resolved Hide resolved
libcxx/src/charconv.cpp Outdated Show resolved Hide resolved

const char* src = __ptr; // rename to match the libc code copied for this section.
ptrdiff_t length = __last - src;
_LIBCPP_ASSERT_INTERNAL(length > 0, "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a nicer error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an error message, though it may still need improvement.

runtimes/cmake/Modules/FindLibcUtils.cmake Outdated Show resolved Hide resolved
runtimes/cmake/Modules/FindLibcUtils.cmake Outdated Show resolved Hide resolved
@@ -317,6 +317,8 @@ auto all_unsigned = type_list<
>();
auto integrals = concat(all_signed, all_unsigned);

auto all_floats = type_list< float, double >(); //TODO: Add long double
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refactor this to use test/support/type_algorithms.h instead. Doesn't have to be in this patch.

libcxx/src/CMakeLists.txt Outdated Show resolved Hide resolved
@ldionne ldionne added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 15, 2024
@ldionne ldionne marked this pull request as ready for review August 15, 2024 18:55
@ldionne ldionne requested a review from a team as a code owner August 15, 2024 18:55
@llvmbot llvmbot added cmake Build system in general and CMake in particular libc labels Aug 15, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2024

@llvm/pr-subscribers-libc

@llvm/pr-subscribers-libcxx

Author: Michael Jones (michaelrj-google)

Changes

This patch aims to demonstrate the utility of sharing code between libc
and libc++ by using the libc float conversion code in the libc++
function from_chars. This patch adds from_chars for floating point types,
as well as setting up a mechanism for further inclusion of libc code
in libc++ in the future.


Patch is 78.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91651.diff

24 Files Affected:

  • (added) libc/shared/str_to_float.h (+29)
  • (modified) libcxx/include/CMakeLists.txt (+1)
  • (added) libcxx/include/__charconv/from_chars_floating_point.h (+56)
  • (modified) libcxx/include/__configuration/availability.h (+13)
  • (modified) libcxx/include/charconv (+7)
  • (modified) libcxx/include/module.modulemap (+10-9)
  • (modified) libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist (+2)
  • (modified) libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist (+2)
  • (modified) libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist (+2)
  • (modified) libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist (+2)
  • (modified) libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist (+2)
  • (modified) libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist (+2)
  • (modified) libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist (+2)
  • (modified) libcxx/src/CMakeLists.txt (+5-2)
  • (modified) libcxx/src/charconv.cpp (+11)
  • (added) libcxx/src/include/from_chars_floating_point.h (+337)
  • (added) libcxx/test/std/utilities/charconv/charconv.from.chars/float.pass.cpp (+1024)
  • (modified) libcxx/test/std/utilities/charconv/charconv.msvc/float_from_chars_test_cases.hpp (-1)
  • (modified) libcxx/test/std/utilities/charconv/charconv.msvc/test.cpp (+37-13)
  • (modified) libcxx/test/std/utilities/charconv/charconv.msvc/test.pass.cpp (+3)
  • (modified) libcxx/test/support/charconv_test_helpers.h (+2)
  • (modified) libcxx/test/support/msvc_stdlib_force_include.h (+1)
  • (modified) libcxx/test/support/test_macros.h (+4)
  • (added) runtimes/cmake/Modules/FindLibcUtils.cmake (+8)
diff --git a/libc/shared/str_to_float.h b/libc/shared/str_to_float.h
new file mode 100644
index 00000000000000..da70db11f6b82b
--- /dev/null
+++ b/libc/shared/str_to_float.h
@@ -0,0 +1,29 @@
+//===-- String to float conversion utils ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SHARED_STR_TO_FLOAT_H
+#define LLVM_LIBC_SHARED_STR_TO_FLOAT_H
+
+#include "src/__support/str_to_float.h"
+
+namespace LIBC_NAMESPACE::shared {
+
+// WARNING: This is a proof of concept. In future the interface point for libcxx
+// won't be using libc internal classes.
+
+template <class T>
+inline internal::FloatConvertReturn<T> decimal_exp_to_float(
+    internal::ExpandedFloat<T> init_num, bool truncated,
+    internal::RoundDirection round, const char *__restrict num_start,
+    const size_t num_len = cpp::numeric_limits<size_t>::max()) {
+  return internal::decimal_exp_to_float(init_num, truncated, round, num_start,
+                                        num_len);
+}
+} // namespace LIBC_NAMESPACE::shared
+
+#endif // LLVM_LIBC_SHARED_STR_TO_FLOAT_H
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 32579272858a8e..43f9e5071f3b02 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -235,6 +235,7 @@ set(files
   __bit/rotate.h
   __bit_reference
   __charconv/chars_format.h
+  __charconv/from_chars_floating_point.h
   __charconv/from_chars_integral.h
   __charconv/from_chars_result.h
   __charconv/tables.h
diff --git a/libcxx/include/__charconv/from_chars_floating_point.h b/libcxx/include/__charconv/from_chars_floating_point.h
new file mode 100644
index 00000000000000..33ecf7012cfb6a
--- /dev/null
+++ b/libcxx/include/__charconv/from_chars_floating_point.h
@@ -0,0 +1,56 @@
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___CHARCONV_FROM_CHARS_FLOATING_POINT_H
+#define _LIBCPP___CHARCONV_FROM_CHARS_FLOATING_POINT_H
+
+#include <__assert>
+#include <__charconv/chars_format.h>
+#include <__charconv/from_chars_result.h>
+#include <__charconv/traits.h>
+#include <__config>
+#include <__system_error/errc.h>
+#include <__type_traits/enable_if.h>
+#include <__type_traits/integral_constant.h>
+#include <__type_traits/is_floating_point.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER >= 17
+
+_LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_EXPORTED_FROM_ABI from_chars_result
+__from_chars_floating_point(const char* __first, const char* __last, float& __value, chars_format __fmt);
+
+_LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_EXPORTED_FROM_ABI from_chars_result
+__from_chars_floating_point(const char* __first, const char* __last, double& __value, chars_format __fmt);
+
+_LIBCPP_HIDE_FROM_ABI inline from_chars_result
+from_chars(const char* __first, const char* __last, float& __value, chars_format __fmt = chars_format::general) {
+  return std::__from_chars_floating_point(__first, __last, __value, __fmt);
+}
+
+_LIBCPP_HIDE_FROM_ABI inline from_chars_result
+from_chars(const char* __first, const char* __last, double& __value, chars_format __fmt = chars_format::general) {
+  return std::__from_chars_floating_point(__first, __last, __value, __fmt);
+}
+
+#endif // _LIBCPP_STD_VER >= 17
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___CHARCONV_FROM_CHARS_FLOATING_POINT_H
diff --git a/libcxx/include/__configuration/availability.h b/libcxx/include/__configuration/availability.h
index ab483a07c9c137..edd9df87ebca99 100644
--- a/libcxx/include/__configuration/availability.h
+++ b/libcxx/include/__configuration/availability.h
@@ -87,6 +87,9 @@
 // in all versions of the library are available.
 #if defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
 
+#  define _LIBCPP_INTRODUCED_IN_LLVM_20 1
+#  define _LIBCPP_INTRODUCED_IN_LLVM_20_ATTRIBUTE /* nothing */
+
 #  define _LIBCPP_INTRODUCED_IN_LLVM_19 1
 #  define _LIBCPP_INTRODUCED_IN_LLVM_19_ATTRIBUTE /* nothing */
 
@@ -132,6 +135,11 @@
 
 // clang-format off
 
+// LLVM 20
+// TODO: Fill this in
+#  define _LIBCPP_INTRODUCED_IN_LLVM_20 0
+#  define _LIBCPP_INTRODUCED_IN_LLVM_20_ATTRIBUTE __attribute__((unavailable))
+
 // LLVM 19
 // TODO: Fill this in
 #  define _LIBCPP_INTRODUCED_IN_LLVM_19 0
@@ -375,6 +383,11 @@
 #define _LIBCPP_AVAILABILITY_HAS_BAD_EXPECTED_ACCESS_KEY_FUNCTION _LIBCPP_INTRODUCED_IN_LLVM_19
 #define _LIBCPP_AVAILABILITY_BAD_EXPECTED_ACCESS_KEY_FUNCTION _LIBCPP_INTRODUCED_IN_LLVM_19_ATTRIBUTE
 
+// This controls the availability of floating-point std::from_chars functions.
+// These overloads were added later than the integer overloads.
+#define _LIBCPP_AVAILABILITY_HAS_FROM_CHARS_FLOATING_POINT _LIBCPP_INTRODUCED_IN_LLVM_20
+#define _LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_INTRODUCED_IN_LLVM_20_ATTRIBUTE
+
 // Define availability attributes that depend on _LIBCPP_HAS_NO_EXCEPTIONS.
 // Those are defined in terms of the availability attributes above, and
 // should not be vendor-specific.
diff --git a/libcxx/include/charconv b/libcxx/include/charconv
index a2e270e9316dc7..dfef8194a2a67b 100644
--- a/libcxx/include/charconv
+++ b/libcxx/include/charconv
@@ -65,6 +65,12 @@ namespace std {
   constexpr from_chars_result from_chars(const char* first, const char* last,
                                see below& value, int base = 10);                         // constexpr since C++23
 
+  constexpr from_chars_result from_chars(const char* first, const char* last,
+                               float& value, chars_format fmt);
+
+  constexpr from_chars_result from_chars(const char* first, const char* last,
+                               double& value, chars_format fmt);
+
 } // namespace std
 
 */
@@ -73,6 +79,7 @@ namespace std {
 
 #if _LIBCPP_STD_VER >= 17
 #  include <__charconv/chars_format.h>
+#  include <__charconv/from_chars_floating_point.h>
 #  include <__charconv/from_chars_integral.h>
 #  include <__charconv/from_chars_result.h>
 #  include <__charconv/tables.h>
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 13d0dce34d97e3..6c0ee59e2cc121 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -1075,18 +1075,19 @@ module std_private_bit_invert_if      [system] { header "__bit/invert_if.h" }
 module std_private_bit_popcount       [system] { header "__bit/popcount.h" }
 module std_private_bit_rotate         [system] { header "__bit/rotate.h" }
 
-module std_private_charconv_chars_format            [system] { header "__charconv/chars_format.h" }
-module std_private_charconv_from_chars_integral     [system] { header "__charconv/from_chars_integral.h" }
-module std_private_charconv_from_chars_result       [system] { header "__charconv/from_chars_result.h" }
-module std_private_charconv_tables                  [system] { header "__charconv/tables.h" }
-module std_private_charconv_to_chars                [system] { header "__charconv/to_chars.h" }
-module std_private_charconv_to_chars_base_10        [system] { header "__charconv/to_chars_base_10.h" }
-module std_private_charconv_to_chars_floating_point [system] { header "__charconv/to_chars_floating_point.h" }
-module std_private_charconv_to_chars_integral       [system] {
+module std_private_charconv_chars_format              [system] { header "__charconv/chars_format.h" }
+module std_private_charconv_from_chars_integral       [system] { header "__charconv/from_chars_integral.h" }
+module std_private_charconv_from_chars_result         [system] { header "__charconv/from_chars_result.h" }
+module std_private_charconv_tables                    [system] { header "__charconv/tables.h" }
+module std_private_charconv_to_chars                  [system] { header "__charconv/to_chars.h" }
+module std_private_charconv_to_chars_base_10          [system] { header "__charconv/to_chars_base_10.h" }
+module std_private_charconv_to_chars_floating_point   [system] { header "__charconv/to_chars_floating_point.h" }
+module std_private_charconv_from_chars_floating_point [system] { header "__charconv/from_chars_floating_point.h" }
+module std_private_charconv_to_chars_integral         [system] {
   header "__charconv/to_chars_integral.h"
   export std_private_charconv_traits
 }
-module std_private_charconv_to_chars_result         [system] {
+module std_private_charconv_to_chars_result           [system] {
   header "__charconv/to_chars_result.h"
   export *
 }
diff --git a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index 917388f86811fe..7e227263ae10ae 100644
--- a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1587,6 +1587,8 @@
 {'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIxNS_22__cxx_atomic_base_implIxEEEE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
index 033d9f9987fa80..e0aca153ca0a2a 100644
--- a/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -572,6 +572,8 @@
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIiNS_22__cxx_atomic_base_implIiEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
index 332d8abeb03e3a..d3c3012c443b0a 100644
--- a/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -572,6 +572,8 @@
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIlNS_22__cxx_atomic_base_implIlEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
 {'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index defe235a283c21..a9dd2a5851f7f5 100644
--- a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1587,6 +1587,8 @@
 {'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIxNS_22__cxx_atomic_base_implIxEEEE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist
index 6b77cda1e2866d..55c69232375b07 100644
--- a/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1220,6 +1220,8 @@
 {'is_defined': True, 'name': '_ZNSt6__ndk123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIiNS_22__cxx_atomic_base_implIiEEEE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt6__ndk123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt6__ndk125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt6__ndk131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt6__ndk132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt6__ndk134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
index 3458b333dd6a9b..356440b6cfd152 100644
--- a/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1235,6 +1235,8 @@
 {'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIlNS_22__cxx_atomic_base_implIlEEEE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
index bdf90ba25c7fd9..0fc3be345fa8d3 100644
--- a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1233,6 +1233,8 @@
 {'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIiNS_22__cxx_atomic_base_implIiEEEE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index fe9d2666fa4caa..6183f42252d82b 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -31,6 +31,7 @@ set(LIBCXX_SOURCES
   include/ryu/f2s.h
   include/ryu/ryu.h
   include/to_chars_floating_point.h
+  include/from_chars_floating_point.h
   legacy_pointer_safety.cpp
   memory.cpp
   memory_resource.cpp
@@ -176,11 +177,13 @@ endif()
 split_list(LIBCXX_COMPILE_FLAGS)
 split_list(LIBCXX_LINK_FLAGS)
 
+include(FindLibcUtils)
+
 # Build the shared library.
 if (LIBCXX_ENABLE_SHARED)
   add_library(cxx_shared SHARED ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
   target_include_directories(cxx_shared PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
-  target_link_libraries(cxx_shared PUBLIC cxx-headers libcxx-...
[truncated]

@@ -121,6 +121,7 @@ inline constexpr FloatFromCharsTestCase float_from_chars_test_cases[] = {
"2222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222"
"2222222222222222222e-45",
chars_format::scientific, 1006, errc{}, 0x0.000004p-126f},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we should not edit this file unless strictly required. This file is copied from the MSVC STL implementation. Avoiding unrelated changes makes it easier to keep the files in sync.

libc/shared/str_to_float.h Outdated Show resolved Hide resolved
Copy link
Contributor Author

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've expanded the libc/shared interface a bit to include the other pieces that are being used in libc++. Now everything on the libc++ side should be coming from the shared namespace.

libc/shared/str_to_float.h Outdated Show resolved Hide resolved
if (__offset + 1 < __n && // an exponent always needs at least one digit.
std::tolower(__first[__offset]) == 'p') {
++__offset; // assumes a valid exponent.
LIBC_NAMESPACE::shared::StrToNumResult<int32_t> __e =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
LIBC_NAMESPACE::shared::StrToNumResult<int32_t> __e =
auto __e =

Comment on lines 365 to 375
int64_t temp_exponent = static_cast<int64_t>(exponent) + static_cast<int64_t>(add_to_exponent);

// If the result is in the valid range, then we use it. The valid range is
// also within the int32 range, so this prevents overflow issues.
if (temp_exponent > LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT) {
exponent = LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT;
} else if (temp_exponent < -LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT) {
exponent = -LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT;
} else {
exponent = static_cast<int32_t>(temp_exponent);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use __merge_exponents

@@ -40,7 +40,7 @@ Paper Status

.. note::

.. [#note-P0067] P0067: ``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``.
.. [#note-P0067] P0067: ``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment would benefit a rewrite, to note what is actually still open. E.g.

Suggested change
.. [#note-P0067] P0067: ``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0.
.. [#note-P0067] P0067: for integer types, ``std::(to|from)_chars`` has been available since version 7.0; for ``float`` and ``double``, ``std::to_chars`` since version 14.0 and ``std::from_chars`` since version 20.1. Support is complete except for ``long double``, which currently uses the implementation for ``double``.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment before, but forgot to publish it :-/
to_chars has float, double, and long double, where long double is double
from_chars only has float and double support, and no long double support at all.
(Adding proper long double support is on my todo list and using LLVM-libc that will be easier, however a lot of work will be to write proper tests for long double. So I feel your suggestion is not correct.

Feel free to propose different wording if you have a better suggestion.

libcxx/include/__charconv/from_chars_floating_point.h Outdated Show resolved Hide resolved
libcxx/lib/abi/CHANGELOG.TXT Show resolved Hide resolved
runtimes/cmake/Modules/FindLibcCommonUtils.cmake Outdated Show resolved Hide resolved
@@ -0,0 +1,1557 @@
//===----------------------------------------------------------------------===//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this test to floating_point.pass.cpp

libcxx/test/support/test_macros.h Outdated Show resolved Hide resolved
libcxx/test/support/test_macros.h Outdated Show resolved Hide resolved
libcxx/test/support/msvc_stdlib_force_include.h Outdated Show resolved Hide resolved
@@ -78,4 +78,5 @@
"","","","","",""
"`LWG3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Adopted Yet","|Complete|","16.0",""
"`LWG4139 <https://wg21.link/LWG4139>`__","§[time.zone.leap] recursive constraint in <=>","Not Adopted Yet","|Complete|","20.0",""
"`LWG3456 <https://wg21.link/LWG3343>`__","Pattern used by std::from_chars is underspecified (option B)",,"Not Yet Adopted","|Complete|","20.0",""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the wrong LWG issue number.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is a copy-paste error; the number is correct, the link is not updated.

libcxx/include/__charconv/from_chars_floating_point.h Outdated Show resolved Hide resolved
Comment on lines 23 to 24
Symbol added: _ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE
Symbol added: _ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These symbols seem a bit wrong: it looks like they are coming from the Android CI jobs, is that possible? Android uses a special namespace instead of __1, so it would be better to take the name generated from another Linux job since that represents better the name of the symbols being added.

libcxx/test/support/msvc_stdlib_force_include.h Outdated Show resolved Hide resolved
Copy link
Contributor Author

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the libc++ changes are done I want to make a small libc change (adding comments to the functions being shared marking them as needing special handling) but other than that this looks good to me

@@ -71,7 +71,7 @@
"`P0394R4 <https://wg21.link/P0394R4>`__","Hotel Parallelifornia: terminate() for Parallel Algorithms Exception Handling","2016-06 (Oulu)","|Complete|","17.0",""
"","","","","",""
"`P0003R5 <https://wg21.link/P0003R5>`__","Removing Deprecated Exception Specifications from C++17","2016-11 (Issaquah)","|Complete|","5.0",""
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``."
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reattaching my suggestion (clarify what's done resp. still open) after the table format changed to include notes.

Suggested change
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0."
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","For integer types, ``std::(to|from)_chars`` has been available since version 7.0; for ``float`` and ``double``, ``std::to_chars`` since version 14.0 and ``std::from_chars`` since version 20.1. Support is complete except for ``long double``, which currently uses the implementation for ``double``.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few comments about the ABI boundary. I'm not overly attached to those, but I do think that avoiding the external template instantiation declaration and the explicit template instantiation in the .cpp file results in simpler code for the reader -- it's not always obvious how that stuff interacts with attributes.

@h-vetinari 's comment on the status page should also be addressed.

Thanks a lot to everyone involved in this patch! It's exciting to see this kind of collaboration between different subprojects.

Comment on lines +37 to +45
template <class _Fp>
_LIBCPP_EXPORTED_FROM_ABI [[gnu::pure]] __from_chars_result<_Fp> __from_chars_floating_point(
[[clang::noescape]] const char* __first, [[clang::noescape]] const char* __last, chars_format __fmt);

extern template __from_chars_result<float> __from_chars_floating_point(
[[clang::noescape]] const char* __first, [[clang::noescape]] const char* __last, chars_format __fmt);

extern template __from_chars_result<double> __from_chars_floating_point(
[[clang::noescape]] const char* __first, [[clang::noescape]] const char* __last, chars_format __fmt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but FWIW it would be strictly simpler to use

_LIBCPP_EXPORTED_FROM_ABI __from_chars_result<float> __from_chars_float(...);
_LIBCPP_EXPORTED_FROM_ABI __from_chars_result<double> __from_chars_double(...);

That way, no need to think about whether the EXPORTED_FROM_ABI attribute is inherited by the external template declaration. It also simplifies the .cpp file since you don't need any explicit template instantiation. It's less "cute", but it's simpler overall.


_LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_HIDE_FROM_ABI inline from_chars_result
from_chars(const char* __first, const char* __last, float& __value, chars_format __fmt = chars_format::general) {
return __from_chars(__first, __last, __value, __fmt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be std::__from_chars, but personally I'd inline the body of __from_chars here to call std::__from_chars_float and std::__from_chars_double directly.

@michaelrj-google
Copy link
Contributor Author

Everything LGTM from the libc side. I will wait to merge until @mordante is done with his edits and approves. Once that's done I will try to merge either this friday morning or monday morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel cmake Build system in general and CMake in particular libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants