Skip to content

[libc++] Add support for picolibc and newlib in RUNTIMES_USE_LIBC #147956

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

voltur01
Copy link
Contributor

This replaces detection of picolibc in libc++ (by checking for and including picolibc.h) with using RUNTIMES_USE_LIBC build time option intriduced in #134893 RUNTIMES_USE_LIBC is extended to accept picolibc and newlib.

Detection of picolibc via the header is kept as a deprecated feature to avoid breaking builds.

libc++ is updated to use dedicated LIBCXX_LIBC_NEWLIB macro to check for newlib specific conditions instead of less informative _NEWLIB_VERSION

This replaces detection of picolibc in libc++ (by checking for and including picolibc.h) with using RUNTIMES_USE_LIBC build time option intriduced in llvm#134893 RUNTIMES_USE_LIBC is extended to accept picolibc and newlib.

Detection of picolibc via the header is kept as a deprecated feature to avoid breaking builds.

libc++ is updated to use dedicated LIBCXX_LIBC_NEWLIB macro to check for newlib specific conditions instead of less informative _NEWLIB_VERSION
@voltur01 voltur01 requested a review from a team as a code owner July 10, 2025 13:08
@llvmbot llvmbot added cmake Build system in general and CMake in particular libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. labels Jul 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 10, 2025

@llvm/pr-subscribers-libcxx

Author: Volodymyr Turanskyy (voltur01)

Changes

This replaces detection of picolibc in libc++ (by checking for and including picolibc.h) with using RUNTIMES_USE_LIBC build time option intriduced in #134893 RUNTIMES_USE_LIBC is extended to accept picolibc and newlib.

Detection of picolibc via the header is kept as a deprecated feature to avoid breaking builds.

libc++ is updated to use dedicated LIBCXX_LIBC_NEWLIB macro to check for newlib specific conditions instead of less informative _NEWLIB_VERSION


Full diff: https://github.com/llvm/llvm-project/pull/147956.diff

22 Files Affected:

  • (modified) libcxx/CMakeLists.txt (+15)
  • (modified) libcxx/include/__config_site.in (+5)
  • (modified) libcxx/include/__configuration/platform.h (+3-4)
  • (modified) libcxx/include/__cxx03/__fwd/ios.h (+1-1)
  • (modified) libcxx/include/__cxx03/__locale (+1-1)
  • (modified) libcxx/include/__cxx03/__locale_dir/locale_base_api.h (+1-1)
  • (modified) libcxx/include/__cxx03/fstream (+1-1)
  • (modified) libcxx/include/__cxx03/locale (+1-1)
  • (modified) libcxx/include/__cxx03/regex (+2-4)
  • (modified) libcxx/include/__fwd/ios.h (+1-1)
  • (modified) libcxx/include/__locale (+1-1)
  • (modified) libcxx/include/__locale_dir/messages.h (+1-1)
  • (modified) libcxx/include/fstream (+2-2)
  • (modified) libcxx/include/regex (+2-4)
  • (modified) libcxx/src/include/config_elast.h (+1-1)
  • (modified) libcxx/src/locale.cpp (+1-1)
  • (modified) libcxx/test/libcxx/system_reserved_names.gen.py (+2-2)
  • (modified) libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/generic_category.pass.cpp (+1-1)
  • (modified) libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp (+1-1)
  • (modified) libcxx/test/support/platform_support.h (+1-1)
  • (modified) libcxx/utils/ci/run-buildbot (+1)
  • (modified) runtimes/cmake/Modules/HandleLibC.cmake (+2-2)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 85514cc7547a9..5c0ea39473ec4 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -481,6 +481,21 @@ include(config-ix)
 include(HandleLibC) # Setup the C library flags
 include(HandleLibCXXABI) # Setup the ABI library flags
 
+# Set C library in use to define respective macro in __config_site
+# RUNTIMES_USE_LIBC was checked in HandleLibC to be one of accepted values
+if (RUNTIMES_USE_LIBC STREQUAL "llvm-libc")
+  set(LIBCXX_LIBC_LLVMLIBC 1)
+elseif (RUNTIMES_USE_LIBC STREQUAL "picolibc")
+  set(LIBCXX_LIBC_PICOLIBC 1)
+  # picolibc is derived from newlib and behaves the same in regards to libc++
+  # so setting both here:
+  # * LIBCXX_LIBC_NEWLIB is used now
+  # * LIBCXX_LIBC_PICOLIBC can be used for further customizations later
+  set(LIBCXX_LIBC_NEWLIB 1) 
+elseif (RUNTIMES_USE_LIBC STREQUAL "newlib")
+  set(LIBCXX_LIBC_NEWLIB 1)
+endif()
+
 # FIXME(EricWF): See the FIXME on LIBCXX_ENABLE_PEDANTIC.
 # Remove the -pedantic flag and -Wno-pedantic and -pedantic-errors
 # so they don't get transformed into -Wno and -errors respectively.
diff --git a/libcxx/include/__config_site.in b/libcxx/include/__config_site.in
index fc01aaf2d8746..f01400bd351ec 100644
--- a/libcxx/include/__config_site.in
+++ b/libcxx/include/__config_site.in
@@ -42,6 +42,11 @@
 // Hardening.
 #cmakedefine _LIBCPP_HARDENING_MODE_DEFAULT @_LIBCPP_HARDENING_MODE_DEFAULT@
 
+// C libraries
+#cmakedefine LIBCXX_LIBC_LLVMLIBC
+#cmakedefine LIBCXX_LIBC_PICOLIBC
+#cmakedefine LIBCXX_LIBC_NEWLIB
+
 // __USE_MINGW_ANSI_STDIO gets redefined on MinGW
 #ifdef __clang__
 #  pragma clang diagnostic push
diff --git a/libcxx/include/__configuration/platform.h b/libcxx/include/__configuration/platform.h
index f3c199dee172b..cb97c97c51ae4 100644
--- a/libcxx/include/__configuration/platform.h
+++ b/libcxx/include/__configuration/platform.h
@@ -42,11 +42,10 @@
 #  endif
 #endif
 
-// This is required in order for _NEWLIB_VERSION to be defined in places where we use it.
-// TODO: We shouldn't be including arbitrarily-named headers from libc++ since this can break valid
-//       user code. Move code paths that need _NEWLIB_VERSION to another customization mechanism.
+// TODO: Remove this deprecated behavior after LLVM 22 release
+// To build libc++ with picolibc provide RUNTIMES_USE_LIBC=picolibc
 #if __has_include(<picolibc.h>)
-#  include <picolibc.h>
+#  define LIBCXX_LIBC_NEWLIB
 #endif
 
 #ifndef __BYTE_ORDER__
diff --git a/libcxx/include/__cxx03/__fwd/ios.h b/libcxx/include/__cxx03/__fwd/ios.h
index dc03e8c6bab2f..5776ad90ea623 100644
--- a/libcxx/include/__cxx03/__fwd/ios.h
+++ b/libcxx/include/__cxx03/__fwd/ios.h
@@ -31,7 +31,7 @@ using wios = basic_ios<wchar_t>;
 template <class _CharT, class _Traits>
 class _LIBCPP_PREFERRED_NAME(ios) _LIBCPP_IF_WIDE_CHARACTERS(_LIBCPP_PREFERRED_NAME(wios)) basic_ios;
 
-#if defined(_NEWLIB_VERSION)
+#if defined(LIBCXX_LIBC_NEWLIB)
 // On newlib, off_t is 'long int'
 using streamoff = long int; // for char_traits in <string>
 #else
diff --git a/libcxx/include/__cxx03/__locale b/libcxx/include/__cxx03/__locale
index d5faa89b99fc0..d32ac05d28187 100644
--- a/libcxx/include/__cxx03/__locale
+++ b/libcxx/include/__cxx03/__locale
@@ -384,7 +384,7 @@ public:
   static const mask xdigit       = _ISXDIGIT;
   static const mask blank        = _ISBLANK;
   static const mask __regex_word = 0x8000;
-#elif defined(_NEWLIB_VERSION)
+#elif defined(LIBCXX_LIBC_NEWLIB)
   // Same type as Newlib's _ctype_ array in newlib/libc/include/ctype.h.
   typedef char mask;
   // In case char is signed, static_cast is needed to avoid warning on
diff --git a/libcxx/include/__cxx03/__locale_dir/locale_base_api.h b/libcxx/include/__cxx03/__locale_dir/locale_base_api.h
index a20f0952f52c3..ccb8201e8ab1d 100644
--- a/libcxx/include/__cxx03/__locale_dir/locale_base_api.h
+++ b/libcxx/include/__cxx03/__locale_dir/locale_base_api.h
@@ -17,7 +17,7 @@
 #  include <__cxx03/__locale_dir/locale_base_api/android.h>
 #elif defined(__sun__)
 #  include <__cxx03/__locale_dir/locale_base_api/solaris.h>
-#elif defined(_NEWLIB_VERSION)
+#elif defined(LIBCXX_LIBC_NEWLIB)
 #  include <__cxx03/__locale_dir/locale_base_api/newlib.h>
 #elif defined(__OpenBSD__)
 #  include <__cxx03/__locale_dir/locale_base_api/openbsd.h>
diff --git a/libcxx/include/__cxx03/fstream b/libcxx/include/__cxx03/fstream
index 44bdabc4602b5..f7290878bbc07 100644
--- a/libcxx/include/__cxx03/fstream
+++ b/libcxx/include/__cxx03/fstream
@@ -209,7 +209,7 @@ typedef basic_fstream<wchar_t> wfstream;
 _LIBCPP_PUSH_MACROS
 #include <__cxx03/__undef_macros>
 
-#if defined(_LIBCPP_MSVCRT) || defined(_NEWLIB_VERSION)
+#if defined(_LIBCPP_MSVCRT) || defined(LIBCXX_LIBC_NEWLIB)
 #  define _LIBCPP_HAS_NO_OFF_T_FUNCTIONS
 #endif
 
diff --git a/libcxx/include/__cxx03/locale b/libcxx/include/__cxx03/locale
index 64162f5a4ff2c..0de1380601855 100644
--- a/libcxx/include/__cxx03/locale
+++ b/libcxx/include/__cxx03/locale
@@ -220,7 +220,7 @@ template <class charT> class messages_byname;
 
 #  if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
 // Most unix variants have catopen.  These are the specific ones that don't.
-#    if !defined(__BIONIC__) && !defined(_NEWLIB_VERSION) && !defined(__EMSCRIPTEN__)
+#    if !defined(__BIONIC__) && !defined(LIBCXX_LIBC_NEWLIB) && !defined(__EMSCRIPTEN__)
 #      define _LIBCPP_HAS_CATOPEN 1
 #      include <nl_types.h>
 #    endif
diff --git a/libcxx/include/__cxx03/regex b/libcxx/include/__cxx03/regex
index b96d59d3a252a..f282c983a7392 100644
--- a/libcxx/include/__cxx03/regex
+++ b/libcxx/include/__cxx03/regex
@@ -984,7 +984,7 @@ public:
   typedef _CharT char_type;
   typedef basic_string<char_type> string_type;
   typedef locale locale_type;
-#if defined(__BIONIC__) || defined(_NEWLIB_VERSION)
+#if defined(__BIONIC__) || defined(LIBCXX_LIBC_NEWLIB)
   // Originally bionic's ctype_base used its own ctype masks because the
   // builtin ctype implementation wasn't in libc++ yet. Bionic's ctype mask
   // was only 8 bits wide and already saturated, so it used a wider type here
@@ -993,9 +993,7 @@ public:
   // implementation, but this was not updated to match. Since then Android has
   // needed to maintain a stable libc++ ABI, and this can't be changed without
   // an ABI break.
-  // We also need this workaround for newlib since _NEWLIB_VERSION is not
-  // defined yet inside __config, so we can't set the
-  // _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE macro. Additionally, newlib is
+  // We also need this workaround for newlib since newlib is
   // often used for space constrained environments, so it makes sense not to
   // duplicate the ctype table.
   typedef uint16_t char_class_type;
diff --git a/libcxx/include/__fwd/ios.h b/libcxx/include/__fwd/ios.h
index 831624f4b1c57..4db2c5da3861d 100644
--- a/libcxx/include/__fwd/ios.h
+++ b/libcxx/include/__fwd/ios.h
@@ -31,7 +31,7 @@ using wios = basic_ios<wchar_t>;
 template <class _CharT, class _Traits>
 class _LIBCPP_PREFERRED_NAME(ios) _LIBCPP_IF_WIDE_CHARACTERS(_LIBCPP_PREFERRED_NAME(wios)) basic_ios;
 
-#if defined(_NEWLIB_VERSION)
+#if defined(LIBCXX_LIBC_NEWLIB)
 // On newlib, off_t is 'long int'
 using streamoff = long int; // for char_traits in <string>
 #else
diff --git a/libcxx/include/__locale b/libcxx/include/__locale
index 757a53951f66e..50422b5af9963 100644
--- a/libcxx/include/__locale
+++ b/libcxx/include/__locale
@@ -389,7 +389,7 @@ public:
   static const mask xdigit       = _ISXDIGIT;
   static const mask blank        = _ISBLANK;
   static const mask __regex_word = 0x8000;
-#  elif defined(_NEWLIB_VERSION)
+#  elif defined(LIBCXX_LIBC_NEWLIB)
   // Same type as Newlib's _ctype_ array in newlib/libc/include/ctype.h.
   typedef char mask;
   // In case char is signed, static_cast is needed to avoid warning on
diff --git a/libcxx/include/__locale_dir/messages.h b/libcxx/include/__locale_dir/messages.h
index c04bf04025ff0..4ebedc9de6d45 100644
--- a/libcxx/include/__locale_dir/messages.h
+++ b/libcxx/include/__locale_dir/messages.h
@@ -22,7 +22,7 @@
 
 #  if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
 // Most unix variants have catopen.  These are the specific ones that don't.
-#    if !defined(__BIONIC__) && !defined(_NEWLIB_VERSION) && !defined(__EMSCRIPTEN__)
+#    if !defined(__BIONIC__) && !defined(LIBCXX_LIBC_NEWLIB) && !defined(__EMSCRIPTEN__)
 #      define _LIBCPP_HAS_CATOPEN 1
 #      include <nl_types.h>
 #    else
diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index c86f709bedb80..9b6b0267267e6 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -964,7 +964,7 @@ template <class _CharT, class _Traits>
 int basic_filebuf<_CharT, _Traits>::__fseek(FILE* __file, pos_type __offset, int __whence) {
 #    if defined(_LIBCPP_MSVCRT_LIKE)
   return _fseeki64(__file, __offset, __whence);
-#    elif defined(_NEWLIB_VERSION)
+#    elif defined(LIBCXX_LIBC_NEWLIB)
   return fseek(__file, __offset, __whence);
 #    else
   return ::fseeko(__file, __offset, __whence);
@@ -975,7 +975,7 @@ template <class _CharT, class _Traits>
 typename basic_filebuf<_CharT, _Traits>::pos_type basic_filebuf<_CharT, _Traits>::__ftell(FILE* __file) {
 #    if defined(_LIBCPP_MSVCRT_LIKE)
   return _ftelli64(__file);
-#    elif defined(_NEWLIB_VERSION)
+#    elif defined(LIBCXX_LIBC_NEWLIB)
   return ftell(__file);
 #    else
   return ftello(__file);
diff --git a/libcxx/include/regex b/libcxx/include/regex
index bbc21e244dd17..29ba9f6a06725 100644
--- a/libcxx/include/regex
+++ b/libcxx/include/regex
@@ -1004,7 +1004,7 @@ public:
   typedef _CharT char_type;
   typedef basic_string<char_type> string_type;
   typedef locale locale_type;
-#    if defined(__BIONIC__) || defined(_NEWLIB_VERSION)
+#    if defined(__BIONIC__) || defined(LIBCXX_LIBC_NEWLIB)
   // Originally bionic's ctype_base used its own ctype masks because the
   // builtin ctype implementation wasn't in libc++ yet. Bionic's ctype mask
   // was only 8 bits wide and already saturated, so it used a wider type here
@@ -1013,9 +1013,7 @@ public:
   // implementation, but this was not updated to match. Since then Android has
   // needed to maintain a stable libc++ ABI, and this can't be changed without
   // an ABI break.
-  // We also need this workaround for newlib since _NEWLIB_VERSION is not
-  // defined yet inside __config, so we can't set the
-  // _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE macro. Additionally, newlib is
+  // We also need this workaround for newlib since newlib is
   // often used for space constrained environments, so it makes sense not to
   // duplicate the ctype table.
   typedef uint16_t char_class_type;
diff --git a/libcxx/src/include/config_elast.h b/libcxx/src/include/config_elast.h
index 7edff2d9375d4..388286e41330f 100644
--- a/libcxx/src/include/config_elast.h
+++ b/libcxx/src/include/config_elast.h
@@ -23,7 +23,7 @@
 #  define _LIBCPP_ELAST ELAST
 #elif defined(__LLVM_LIBC__)
 // No _LIBCPP_ELAST needed for LLVM libc
-#elif defined(_NEWLIB_VERSION)
+#elif defined(LIBCXX_LIBC_NEWLIB)
 #  define _LIBCPP_ELAST __ELASTERROR
 #elif defined(__NuttX__)
 // No _LIBCPP_ELAST needed on NuttX
diff --git a/libcxx/src/locale.cpp b/libcxx/src/locale.cpp
index da735865c322c..da4d16c7963f9 100644
--- a/libcxx/src/locale.cpp
+++ b/libcxx/src/locale.cpp
@@ -933,7 +933,7 @@ const ctype<char>::mask* ctype<char>::classic_table() noexcept {
   return __pctype_func();
 #  elif defined(__EMSCRIPTEN__)
   return *__ctype_b_loc();
-#  elif defined(_NEWLIB_VERSION)
+#  elif defined(LIBCXX_LIBC_NEWLIB)
   // Newlib has a 257-entry table in ctype_.c, where (char)0 starts at [1].
   return _ctype_ + 1;
 #  elif defined(_AIX)
diff --git a/libcxx/test/libcxx/system_reserved_names.gen.py b/libcxx/test/libcxx/system_reserved_names.gen.py
index a4f2928eda332..5e4809d20fd82 100644
--- a/libcxx/test/libcxx/system_reserved_names.gen.py
+++ b/libcxx/test/libcxx/system_reserved_names.gen.py
@@ -78,7 +78,7 @@
 
 // Test that libc++ doesn't use names that collide with FreeBSD system macros.
 // newlib and picolibc also define these macros
-#if !defined(__FreeBSD__) && !defined(_NEWLIB_VERSION)
+#if !defined(__FreeBSD__) && !defined(LIBCXX_LIBC_NEWLIB)
 #  define __null_sentinel SYSTEM_RESERVED_NAME
 #  define __generic SYSTEM_RESERVED_NAME
 #endif
@@ -117,7 +117,7 @@
 #endif
 
 // Newlib & picolibc use __input as a parameter name of a64l & l64a
-#ifndef _NEWLIB_VERSION
+#ifndef LIBCXX_LIBC_NEWLIB
 # define __input SYSTEM_RESERVED_NAME
 #endif
 #define __output SYSTEM_RESERVED_NAME
diff --git a/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/generic_category.pass.cpp b/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/generic_category.pass.cpp
index 5425203304014..b4fa377b7f101 100644
--- a/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/generic_category.pass.cpp
+++ b/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/generic_category.pass.cpp
@@ -48,7 +48,7 @@ int main(int, char**)
         // responds with an empty message, which we probably want to
         // treat as a failure code otherwise, but we can detect that
         // with the preprocessor.
-#if defined(_NEWLIB_VERSION)
+#if defined(LIBCXX_LIBC_NEWLIB)
         const bool is_newlib = true;
 #else
         const bool is_newlib = false;
diff --git a/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp b/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp
index 255cbe75e2fa9..ca3a57006f0d4 100644
--- a/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp
+++ b/libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp
@@ -59,7 +59,7 @@ int main(int, char**) {
     // responds with an empty message, which we probably want to
     // treat as a failure code otherwise, but we can detect that
     // with the preprocessor.
-#if defined(_NEWLIB_VERSION)
+#if defined(LIBCXX_LIBC_NEWLIB)
     const bool is_newlib = true;
 #else
     const bool is_newlib = false;
diff --git a/libcxx/test/support/platform_support.h b/libcxx/test/support/platform_support.h
index 99e60f60c5998..655641e858468 100644
--- a/libcxx/test/support/platform_support.h
+++ b/libcxx/test/support/platform_support.h
@@ -48,7 +48,7 @@
 # include <string.h> // strverscmp
 #endif
 
-#if defined(_NEWLIB_VERSION) && defined(__STRICT_ANSI__)
+#if defined(LIBCXX_LIBC_NEWLIB) && defined(__STRICT_ANSI__)
 // Newlib provides this, but in the header it's under __STRICT_ANSI__
 extern "C" {
   int mkstemp(char*);
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index d8b23be9a0323..9bd63d175a74c 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -230,6 +230,7 @@ function test-armv7m-picolibc() {
         -DLIBUNWIND_TEST_CONFIG="armv7m-picolibc-libunwind.cfg.in" \
         -DCMAKE_C_FLAGS="${flags}" \
         -DCMAKE_CXX_FLAGS="${flags}" \
+        -DRUNTIMES_USE_LIBC=picolibc \
         "${@}"
 
     step "Installing compiler-rt"
diff --git a/runtimes/cmake/Modules/HandleLibC.cmake b/runtimes/cmake/Modules/HandleLibC.cmake
index 51fbf04df7e3b..23f735aee6c02 100644
--- a/runtimes/cmake/Modules/HandleLibC.cmake
+++ b/runtimes/cmake/Modules/HandleLibC.cmake
@@ -10,10 +10,10 @@
 
 include_guard(GLOBAL)
 
-set(RUNTIMES_SUPPORTED_C_LIBRARIES system llvm-libc)
+set(RUNTIMES_SUPPORTED_C_LIBRARIES system llvm-libc picolibc newlib)
 set(RUNTIMES_USE_LIBC "system" CACHE STRING "Specify C library to use. Supported values are ${RUNTIMES_SUPPORTED_C_LIBRARIES}.")
 if (NOT "${RUNTIMES_USE_LIBC}" IN_LIST RUNTIMES_SUPPORTED_C_LIBRARIES)
-  message(FATAL_ERROR "Unsupported C library: '${RUNTIMES_CXX_ABI}'. Supported values are ${RUNTIMES_SUPPORTED_C_LIBRARIES}.")
+  message(FATAL_ERROR "Unsupported C library: '${RUNTIMES_USE_LIBC}'. Supported values are ${RUNTIMES_SUPPORTED_C_LIBRARIES}.")
 endif()
 
 # Link against a system-provided libc

Copy link
Contributor

@pratlucas pratlucas left a comment

Choose a reason for hiding this comment

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

Just a small comment, but LGTM otherwise.

Copy link
Contributor

@pratlucas pratlucas left a comment

Choose a reason for hiding this comment

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

LGTM, but as per the contribution policy this PR probably needs approval from someone from the libc++ review group.

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.

Thanks for the patch! I have some comments but this makes sense to me.

@@ -481,6 +481,21 @@ include(config-ix)
include(HandleLibC) # Setup the C library flags
include(HandleLibCXXABI) # Setup the ABI library flags

# Set C library in use to define respective macro in __config_site
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest we do this instead (around line 753 where we handle other __config_site variables):

if (RUNTIMES_USE_LIBC STREQUAL "llvm-libc")
  config_define(_LIBCPP_LIBC_LLVMLIBC 1)
elseif (RUNTIMES_USE_LIBC STREQUAL "picolibc")
  config_define(_LIBCPP_LIBC_PICOLIBC 1)
elseif (RUNTIMES_USE_LIBC STREQUAL "newlib")
  config_define(_LIBCPP_LIBC_NEWLIB 1)
endif()

That's more consistent with what we do for the other __config_site macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for review!

Good point, moved and made use of config_define() for consistency.

Comment on lines 490 to 494
# picolibc is derived from newlib and behaves the same in regards to libc++
# so setting both here:
# * LIBCXX_LIBC_NEWLIB is used now
# * LIBCXX_LIBC_PICOLIBC can be used for further customizations later
set(LIBCXX_LIBC_NEWLIB 1)
Copy link
Member

Choose a reason for hiding this comment

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

Do you absolutely need this right now (if so, why)? I think it would be cleaner to have a 1-1 mapping between the macros we define and the libc we're using, otherwise things become confusing when reading the source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was for completeness only - removed now.

@@ -481,6 +481,21 @@ include(config-ix)
include(HandleLibC) # Setup the C library flags
Copy link
Member

Choose a reason for hiding this comment

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

General comment on the whole patch: Let's use _LIBCPP_foo instead of LIBCXX_foo for the macro names. That's what we do consistently everywhere, and we must use a reserved identifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, done.

Comment on lines 46 to 48
#cmakedefine LIBCXX_LIBC_LLVMLIBC
#cmakedefine LIBCXX_LIBC_PICOLIBC
#cmakedefine LIBCXX_LIBC_NEWLIB
Copy link
Member

Choose a reason for hiding this comment

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

I'd use #cmakedefine01 here and then I'd check #if _LIBCPP_LIBC_NEWLIB instead of #if defined(_LIBCPP_LIBC_NEWLIB). This can prevent mistakes when coupled with -Wundef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good suggestion, done - the checks are much cleaner now.

// user code. Move code paths that need _NEWLIB_VERSION to another customization mechanism.
#if __has_include(<picolibc.h>)
# include <picolibc.h>
// TODO: Remove this deprecated behavior after LLVM 22 release
Copy link
Member

Choose a reason for hiding this comment

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

Since this is something that vendors should change, I'm not sure there's a benefit from deprecating this before we remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I do not expect many people using it anyway - removed.

- Use _LIBCXX_... reserved namespace macro names.
- Move library definition code closer to the rest of __config_site definitions in CMake.
- Remove unused LIBCXX_LIBC_NEWLIB definition.
- Use #cmakedefine01 and thus avoid defined() in conditional statements.
- Remove deprecation check.
@arichardson
Copy link
Member

It looks to me like this will break existing builds that don't define the macro. I think for at least a while there should be a CMake check for picoline.h (and the newlib equivalent) to set these macros correctly and emit a warning to update the build config?

@voltur01
Copy link
Contributor Author

Thank you for the review @arichardson! There is a way to provide a warning like in this commit a876988 however I am not sure how many people are actually building with picolibc and will benefit from it.

I think this is up to maintainers to decide if the warning should be included or not.

@arichardson
Copy link
Member

Thank you for the review @arichardson! There is a way to provide a warning like in this commit a876988 however I am not sure how many people are actually building with picolibc and will benefit from it.

I think this is up to maintainers to decide if the warning should be included or not.

Yeah I'm not sure how important this is. I am using the existing implicit support but I can easily update my build scripts.
I think it would be quite nice to detect this at CMake time and give a warning, but probably no need to add the checks in the headers themselves.
IMO this is a question for the libc++ maintainers

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

Successfully merging this pull request may close these issues.

6 participants